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

Allow systems to specify custom filter for the applicable set beyond required_components #4604

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Allow systems to specify custom filter for the applicable set beyond required_components #4604

merged 6 commits into from
Dec 21, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Dec 20, 2023 •
edited by github-actions bot
Loading

What

Part of general query effort, mostly

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

  • 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 🚜 refactor Change the code, not the functionality 🟦 blueprint The data that defines our UI exclude from changelog PRs with this won't show up in CHANGELOG.md labels Dec 20, 2023
jleibs
jleibs approved these changes Dec 20, 2023
View reviewed changes
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.

Nice -- I like how easy it is to add something like this now!

crates/re_space_view_bar_chart/src/view_part_system.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/parts/images.rs Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member Author

Wumpf commented Dec 21, 2023

linkinator failure was temp docker unavailablility

@Wumpf Wumpf merged commit a85a268 into main Dec 21, 2023
39 of 40 checks passed
@Wumpf Wumpf deleted the andreas/more-intelligent-applicable-entities branch December 21, 2023 14:46
@Wumpf Wumpf mentioned this pull request Dec 21, 2023
Merged
4 tasks
Wumpf added a commit that referenced this pull request 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleibs jleibs jleibs approved these changes

Assignees
No one assigned
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 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Wumpf @jleibs