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

More rigorious evaluation of visualizable entities in spatial views using new SpatialTopology store subscriber #4836

Merged
merged 17 commits into from
Jan 18, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jan 17, 2024

What

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

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf Wumpf added enhancement New feature or request 📺 re_viewer affects re_viewer itself exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 17, 2024
@Wumpf Wumpf requested a review from jleibs January 17, 2024 10:03
crates/re_space_view_spatial/src/space_view_2d.rs Outdated Show resolved Hide resolved
};

// Entities _at_ pinholes are a special case: we display both 3d and 2d visualizers for them.
entities_in_main_3d_space.insert(child_space.origin.clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change it yet but this continues to make me think these entities should just be in primary_space.entities in the first place so we don't have to mutate the set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we can totally throw the responsibility for this onto the space topology, just need to add a few more unit tests that it does the right thing under updates.
Nice thing is that unlike the old SpaceInfo we know what we're dealing with and who's the "target audience"

crates/re_space_view_spatial/src/spatial_topology.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/spatial_topology.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/spatial_topology.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf merged commit 083a851 into main Jan 18, 2024
40 of 41 checks passed
@Wumpf Wumpf deleted the andreas/spatial-topology branch January 18, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exclude from changelog PRs with this won't show up in CHANGELOG.md 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants