Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When no heuristics match, still pick a fallback view if possible #3342

Closed
jleibs opened this issue Sep 15, 2023 · 3 comments · Fixed by #4874
Closed

When no heuristics match, still pick a fallback view if possible #3342

jleibs opened this issue Sep 15, 2023 · 3 comments · Fixed by #4874
Labels
enhancement New feature or request 📺 re_viewer affects re_viewer itself
Milestone

Comments

@jleibs
Copy link
Member

jleibs commented Sep 15, 2023

As of #3323 our space view part systems now use indicator components to filter the heuristics. This means if we just log via the new as_components APIs, we don't get any views. Patching in indicators is still a bit painful, so it would be nice if we had sane fallback behavior.

@jleibs jleibs added enhancement New feature or request 📺 re_viewer affects re_viewer itself labels Sep 15, 2023
@emilk
Copy link
Member

emilk commented Sep 25, 2023

Let's do it for 0.9 if it is easy

@Wumpf Wumpf assigned Wumpf and unassigned Wumpf Sep 25, 2023
@Wumpf
Copy link
Member

Wumpf commented Sep 27, 2023

The basics of implementing this are easy as expected. Instead of discarding entities based on heuristics from the EntitiesPerSystemPerClass list immediately we can do something like this:

pub enum ViewPartHeuristicFilterResult {
    /// The entity path has all required components for the part system and passes the heuristic filter.
    ///
    /// This means that the PartSystem will with very high likelihood display something meaningful for this entity.
    Passes,

    /// The entity path has all *required* components for the part system, but fails the heuristic filter.
    ///
    /// This typically means that it's either lacking an indicator component or
    /// has some property that make not possible to be displayed by the given part system.
    Fails,
}

pub type EntitiesPerSystem =
    IntMap<ViewSystemName, IntMap<EntityPath, ViewPartHeuristicFilterResult>>;

pub type EntitiesPerSystemPerClass = IntMap<SpaceViewClassName, EntitiesPerSystem>;

We then can use the heuristic result to spawn Space Views where the heuristic passes or, if no heuristic passes, spawn the first one that still fulfills the requirements but not the heuristics. (Would be nice to have a better heuristic than "the first")
For this we'd likely forward this new piece of information to SpaceViewClass::auto_spawn_heuristic

The tricky part that requires a bit deeper changes is how to add entities to an existing space view: The problem is that we do this in a localized manner right now. Each Space View looks at the list of entities its systems would be interested in and then decides whether to add it. Previously, this list was already filtered by the heuristic. With the above altered datastructure however we need to make a choice depending on whether another Space View was better suited for an entity that didn't pass those heuristics. This means that the decision which incoming entities to add to which existing Space View becomes a central one and not one we can solve by looking at one (existing) Space View at a time.

➡️ In the near future the heuristic pass needs pass need to be structured like this (not too unsimilar to now):

  • compute EntitiesPerSystemPerClass with heuristic information (note that this is affected by the timeline position and incoming data)
  • if enabled, add new space views, using an enhanced SpaceViewClass::auto_spawn_heuristic. TODO: Ideally we rewrite this step to not yet populate entities so this can be done in the next step?
  • for all entities that aren't in any space view yet at all determine where they should go among the lists of Space Views with the auto add entities flag enabled

The change for the last step is a bit too much and too dangerous for 0.9. Delayed to 0.10

@Wumpf Wumpf removed their assignment Sep 27, 2023
@emilk
Copy link
Member

emilk commented Oct 9, 2023

Maybe related:

Wumpf added a commit that referenced this issue Jan 28, 2024
…with many entities (#4874)

### What

What space views get spawned is no longer determined by a centralized
heuristic, but instead by one distributed to each space view class. Each
of those heuristics can make much better use of existing information
provided by store subscribers.

* Fixes most of #4388
   *  a few things on the tracking issue still left to investigate
* Fixes #3342 
* more "obsoleted" arguably: We originally had a hack that 1d tensor
logging would display a bar chart. That's not actually desirable. There
is still the open issue for the _query_ now to not rely exclusively on
indicators, but automatic view spawning is encouraged to do so. (Overall
the workings and fault lines of these systems have shifted a lot since
the ticket was filed.)
* Fixes #4695
* Looks like this now:
![image](https://github.com/rerun-io/rerun/assets/1220815/a77e7768-0d4f-4045-b5a2-980e49955c31)
* Fixes #4689
* Fixes #3079

Significant speedup for scenes with many entities (100+):

`opf --no-frames`
before: `App::update` 39.0ms, of which `default_created_space_views`
10.4ms
after: `App::update` 27.4ms, of which `default_created_space_views`
0.17ms

`python/tests/many_entities (1000 individual points)`
before: `App:::update` 151ms, of which `default_created_space_views`
124ms
after: `App::update` 22.6ms, of which `default_created_space_views`
0.06ms
(_still pretty bad, but less so and for different reasons now!_)

(numbers from my macbook on a release build. Actual numbers might differ
by now, these are from last week. )


### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4874/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4874/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4874/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4874)
- [Docs
preview](https://rerun.io/preview/ce72c01b3379c20b43d3cdbbe44c37dce2a94f5c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/ce72c01b3379c20b43d3cdbbe44c37dce2a94f5c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 📺 re_viewer affects re_viewer itself
Projects
None yet
3 participants