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.

Sign up for GitHub

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

Jump to bottom

Re-engineer the space-view heuristics engine #4388

Closed
4 of 5 tasks
jleibs opened this issue Nov 29, 2023 · 0 comments · Fixed by #4937
Closed
4 of 5 tasks

Re-engineer the space-view heuristics engine #4388

jleibs opened this issue Nov 29, 2023 · 0 comments · Fixed by #4937
Assignees
@Wumpf @jleibs
Labels
📺 re_viewer affects re_viewer itself 🎄 tracking issue issue that tracks a bunch of subissues

Comments

@jleibs
Copy link
Member

jleibs commented Nov 29, 2023 •
edited by Wumpf
Loading

Tracking issue to capture all the things we want to make sure get fixed:

Steps towards this goal:

  • store applicability of entities per visualizer and update only on change
  • centralize spatial topology into a store subscriber
  • move "desired space views"-heuristic into a trait method of space view class and have an independent "consolidation" logic piece in the viewport. heuristic results should be availble to the ui regardless (in order to allow showing suggestions)
  • execute "desired space views"-heuristic only when necessary -- turns out it's fast enough to skip this!
  • Remove the old SpaceInfo everywhere
@jleibs jleibs added 🎄 tracking issue issue that tracks a bunch of subissues 📺 re_viewer affects re_viewer itself labels Nov 29, 2023
@jleibs jleibs added this to the 0.12 milestone Nov 29, 2023
@jleibs jleibs mentioned this issue Nov 29, 2023
Merged
4 tasks
This was referenced Dec 12, 2023
Closed
Open
@Wumpf Wumpf mentioned this issue Dec 20, 2023
Merged
4 tasks
Wumpf added a commit that referenced this issue Dec 21, 2023
@Wumpf
Allow systems to specify custom filter for the applicable set beyond …
a85a268 
…required_components (#4604)

### What

Part of general query effort, mostly
* [#4388](#4388)

Allows visualizers to specify `VisualizerAdditionalApplicabilityFilter`
which can be used to answer global visualizer applicability questions
like "is this an image tensor". Therefore moving these kind of checks
out of the `heuristic_filter`.

Right now this has a negligible (presumed positive) impact on
performance. However, there's two strong motivating factors here:
* `heuristic_filter` is right now incorrectly applied on a per space
view _class_ bases. It currently is used to extract the "visualizable
set" which actually **has** to be determined per space view _instance_
since it is dependent on things like the space's origin. Once
`heuristic_filter` runs per instance (or much much worse, per instance
candidate during heuristic!) the now moved checks become a huge burden.
This is about to happen in a follow-up PR!
* previously, we did a latest-at-query for things that should be
resolved with a store diff. Therefore, this change makes the
entity->visualizer mapping more correct & deterministic since missing an
update because of rendering pacing is no longer possible!

### 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/4604/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4604/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/4604/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/4604)
- [Docs
preview](https://rerun.io/preview/a5b72a18a7c05938ec66e510aea455fc42b3608b/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/a5b72a18a7c05938ec66e510aea455fc42b3608b/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf Wumpf mentioned this issue Dec 21, 2023
Merged
4 tasks
Wumpf added a commit that referenced this issue Dec 22, 2023
@Wumpf
Store indicator component mapping as part of Visualizer's store subsc…
e70e1dd 
…ription (#4608)

### What
 
* Kinda part of #4388
* Continuing the spirit of #4604
    * Exact same reasoning and motiviation applies! 
* This time this actually has clear positive perf implications:
Evaluation of `identify_entities_per_system_per_class` goes from 1ms to
0.5ms on `opf --no-frames`
* but again in the "heuristic evaluation is part of query"-world this is
a _huge_ speedup


### 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/4608/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4608/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/4608/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/4608)
- [Docs
preview](https://rerun.io/preview/43ecf8119748c2370d859cade05837fd62f9ffe9/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/43ecf8119748c2370d859cade05837fd62f9ffe9/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf Wumpf mentioned this issue Dec 22, 2023
Merged
4 tasks
Wumpf added a commit that referenced this issue Dec 22, 2023
@Wumpf @jleibs
Separation & improved book-keeping of entity -> visualizer assignment (…
95874ba 
…#4612)

### What

* Part of:
   * #4377 
   * #4388

Previously, we assumed that the `heuristic_filter` on applicable
entities (== entities that fullfill all required components and some
additional global properties as determined by store subscriber) applies
on a per space view _class_ basis.
However, this is not entirely accurate and misses that "visualizability"
is determined on a per-space-view-instance basis: The most prominent
example of this is that a 2D primitive may or may not be visualizable in
a 3D view depending on where the 3D origin is.
Another, more forward looking problem was had in this area is that
`heuristic_filter` of visualizers should be applied as part of the
_query_ since a query determines whether visualizers are picked
automaticall/all/single.

This PR separates all these concerns more clearly and introduces types
wrapping lists of entities to make it harder to confuse which set
represents what:

* `ApplicableEntities`
   * global set, already produced by a store subscriber
* `VisualizableEntities`
* produced per space view instance. Space view instances can compute a
context (replaces `HeuristicContext`!) to guide their systems. This is
an `Any` which allows visualizer systems to react to different "parent"
space views!
* `IndicatorMatchingEntities`
   * global set, already produced by a store subscriber
* to be used as base for the "ActiveEntities" for each visualizer. For
simplicity the "ActiveEntities" itself is right now not a type. Instead,
the query uses `IndicatorMatchingEntities`and passed in
`VisualizableEntities` on the fly to determine this final set. In the
**future** it:
* may opt out to do so because a query warrants a specific visualizer(s)
* may opt out to do so because a query warrants all possible visualizers
       * visualizers and/or classes may modify the heuristic set


Great care was taken to not regress performance of the (still per frame
applied) heuristics. However, since we're doing a lot more work now
there's still a bit of a performance hit:

Before:

![image](https://github.com/rerun-io/rerun/assets/1220815/e6fe0acc-535b-40c4-be7a-e5e66a752834)

After:

![image](https://github.com/rerun-io/rerun/assets/1220815/14d91e6f-2f9c-4de0-92af-03ea4fff0b74)

I decided that any further improvements in this area should be done in
the context of not recomputing heuristics every frame and instead react
to relevant changes. (#4388)


### 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/4612/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4612/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/4612/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/4612)
- [Docs
preview](https://rerun.io/preview/7b0485ba1d74511929facf0c06a42f6c95eb5881/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/7b0485ba1d74511929facf0c06a42f6c95eb5881/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Jeremy Leibs <jleibs@users.noreply.github.com>
@emilk emilk removed this from the 0.12 milestone Jan 2, 2024
This was referenced Jan 5, 2024
Closed
Closed
Merged
Wumpf added a commit that referenced this issue Jan 8, 2024
@Wumpf
2D visualizers are no longer applicable to paths that are behind a pi…
0f7216e 
…nhole camera other than the space's origin (#4728)

### What

Fixes broken heuristics for structure_from_motion.

This is a bit of a hacky addition. We need to solve this with a proper
`SpatialTopology` data structure that can tell us efficiently if we're
inside a 2d/3d space and how far it reaches.

See also
* #4388


<img width="446" alt="image"
src="https://github.com/rerun-io/rerun/assets/1220815/08f3dee5-1823-4055-a339-95cb778e4f01">

picture showing that a 2D space at `camera` is no longer suggested since
it wouldn't be able to show anything: `camera/image` has a pinhole
projection, therefore `camera` -> `camera/pinhole` is a 3D->2D
projection but by having a 2D space at `camera` we implied `camera` to
have an eye pinhole.


### 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/4728/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4728/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/4728/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/4728)
- [Docs
preview](https://rerun.io/preview/74a06017c2380916ec46cf5639a82e4a3bf4189a/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/74a06017c2380916ec46cf5639a82e4a3bf4189a/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf Wumpf mentioned this issue Jan 17, 2024
Merged
4 tasks
Wumpf added a commit that referenced this issue Jan 18, 2024
@Wumpf @jleibs
More rigorious evaluation of visualizable entities in spatial views u…
083a851 
…sing new `SpatialTopology` store subscriber (#4836)

### What

* Important part of #4388

Introduces a `SpatialTopology` support structure which is built
incrementally using a store subscription.
It is geared towards replacing the existing `SpaceInfo` (todo!).

The first usecase for this is a more rigorous determination of
visualizable entities for spatial views: this now allows us to much more
accurately reason about what entities are visualizable and how to treat
corner cases (/invalid cases) like logged pinhole under a pinhole.

Since this makes the evaluation of visualizable entities / creation of
their context a bit more complex in some scenes, this comes with a bit
of a perf regression during heuristic eval. However, since the topology
changes rarely and deterministically, this also brings a big opportunity
for only evaluating the more complex context objects when needed and
even sharing them accross different space view (candidates). This is not
included in this pr since this comes with the open question of where to
store such a secondary cache (likely on topology) and how to flush it
(probably best for any new entity or new relevant component).


### 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/4836/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4836/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/4836/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/4836)
- [Docs
preview](https://rerun.io/preview/4e908b26a2323f145b06a7938a44cc178019d3bf/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/4e908b26a2323f145b06a7938a44cc178019d3bf/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Jeremy Leibs <jleibs@users.noreply.github.com>
@Wumpf Wumpf mentioned this issue Jan 19, 2024
Merged
4 tasks
@Wumpf Wumpf mentioned this issue Jan 28, 2024
Closed
Wumpf added a commit that referenced this issue Jan 28, 2024
@Wumpf
Improved automatic view creation heuristic, major speedup for scenes …
15cd60d 
…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)
This was referenced Jan 29, 2024
Merged
Merged
Wumpf added a commit that referenced this issue Jan 30, 2024
@Wumpf
DisconnectedSpace now only applies to spatial space views (#4935)
2b3e31b 
### What


* Part of #4388
* Fixes #4465

This PR only addresses the documentation. `main` is actually in a
slightly mixed state right now: Visualizability is already determined
strictly with `DisconnectedSpace` only applying to spatial views, but we
still have the legacy `SpaceInfo` used in some places that applies this
concept to everything. This will be removed in a a follow-up.

### 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/4935/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4935/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/4935/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/4935)
- [Docs
preview](https://rerun.io/preview/4eec4acd74c6d5a8f132bd8490895ae45e90aac7/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/4eec4acd74c6d5a8f132bd8490895ae45e90aac7/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Wumpf added a commit that referenced this issue Jan 30, 2024
@Wumpf
Remove legacy SpaceInfo (#4937)
fb8d5d9 
### What

* Fixes #4388
   * this is the last piece 🥳  
* Related to
   * #4935
   * #4826

`SpaceInfo` used to be used to determine space connectivity, this is by
now done by `SpatialTopology`

Not a "pure" refactor: Slight changes in behavior
* Adding a space view via the 'plus' top left uses the default-created
list, not the now removed "all" list (which was poorly defined)
* Similarly, suggestions for root now use the default-created list
* adding entities via the add-ui no longer uses any kind of transform
analysis (it relied on SpaceInfo)
   * Does not fix  #4826
* .. since it still won't allow adding things that aren't visualizable
at all



### 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/4937/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4937/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/4937/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/4937)
- [Docs
preview](https://rerun.io/preview/d27d81f094e026a2cecfd42ac25dc133d9ed26f6/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d27d81f094e026a2cecfd42ac25dc133d9ed26f6/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
Assignees

@Wumpf Wumpf

@jleibs jleibs

Labels
📺 re_viewer affects re_viewer itself 🎄 tracking issue issue that tracks a bunch of subissues
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove legacy SpaceInfo rerun-io/rerun
3 participants
@emilk @Wumpf @jleibs