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

Separation & improved book-keeping of entity -> visualizer assignment #4612

Merged
merged 20 commits into from
Dec 22, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Dec 22, 2023

What

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 IndicatorMatchingEntitiesand 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

After:
image

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

  • 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 📺 re_viewer affects re_viewer itself 🟦 blueprint The data that defines our UI exclude from changelog PRs with this won't show up in CHANGELOG.md labels Dec 22, 2023
@Wumpf Wumpf requested a review from jleibs December 22, 2023 10:21
Wumpf and others added 3 commits December 22, 2023 11:37
Co-authored-by: Jeremy Leibs <jleibs@users.noreply.github.com>
Co-authored-by: Jeremy Leibs <jleibs@users.noreply.github.com>
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Really happy with how this all turned out. So much cleaner. Thanks for the thorough walkthrough.

@Wumpf Wumpf merged commit 95874ba into main Dec 22, 2023
40 checks passed
@Wumpf Wumpf deleted the andreas/separate-stages-of-entity-visualizer-mappings branch December 22, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI 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.

None yet

2 participants