-
Notifications
You must be signed in to change notification settings - Fork 300
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
Store indicator component mapping as part of Visualizer's store subscription #4608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Definitely seems worth computing with the subscriber.
let entites_with_matching_indicator = ctx | ||
.entities_with_matching_indicator_per_visualizer | ||
.get(&id) | ||
.unwrap_or(&empty_entity_set); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an empty set, do we still need to run: visualizer.heuristic_filter
, or can we just return false directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm practically yes. But technically heuristic_filter
decides whehter to use entites_with_matching_indicator
in the first place 🤔
I'm a bit undecided. leaving it for now
@@ -35,6 +37,10 @@ pub struct ViewerContext<'a> { | |||
/// Mapping from class and system to entities for the store | |||
pub entities_per_system_per_class: &'a EntitiesPerSystemPerClass, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this needs a more specific name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is why it is renamed in the next pr 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Definitely seems worth computing with the subscriber.
…ponent-store-subscription
What
identify_entities_per_system_per_class
goes from 1ms to 0.5ms onopf --no-frames
Checklist
main
build: app.rerun.ionightly
build: app.rerun.io