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

Keep global registry of all visualizers & context systems instead of isolating per space view class #4540

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Dec 14, 2023

What

This will allow us to store additional state with each registry entry: This will be used to register a VisualizerEntitySubscriber and store a handle to it for every visualizer.

All the "interesting" changes are in crates/re_viewer_context/src/space_view/space_view_class_registry.rs, everything else is just ripples.

Note that this pr sneakily starts the renaming of ViewPart(System) to just Visualizer

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):
    • Full build: app.rerun.io
    • Partial build: app.rerun.io - Useful for quick testing when changes do not affect examples in any way
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

This will allow us to store additional state with each registry entry: This will be used to register a `VisualizerEntitySubscriber` and store a handle to it for every visualizer
Note that this pr sneakily starts the renaming of `ViewPart(System)` to just `Visualizer`
@Wumpf Wumpf added 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels Dec 14, 2023
Copy link

Size changes

Name main 4540/merge Change
plots.rrd 204.80 kiB 194.56 kiB -5.00%

@teh-cmc teh-cmc self-requested a review December 15, 2023 08:34
@teh-cmc
Copy link
Member

teh-cmc commented Dec 15, 2023

Putting a massive renaming change in the same commit as everything else was not very nice of you @Wumpf 🥲

EDIT: Ha nevermind after having a closer look it's more tricky than that 😶

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Im too far removed from all of this to really discuss the semantics of it all, but understood the general idea and didn't see anything suprising so... 🟢

struct SpaceViewClassRegistryEntry {
class: Box<dyn DynSpaceViewClass>,
systems: SpaceViewSystemRegistry,
context_systems: HashSet<ViewSystemIdentifier>,
Copy link
Member

Choose a reason for hiding this comment

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

then I guess they are... contextualizers? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

hehe :D. Was actually wondering if it's still VisualizerSystem in this context, to not make system stick out too much

@Wumpf
Copy link
Member Author

Wumpf commented Dec 15, 2023

Putting a massive renaming change

ah yeah it's not the massive change that is going to happen. But still kinda guilty as I started chipping away on it.

@Wumpf Wumpf merged commit 490fc63 into main Dec 15, 2023
40 of 41 checks passed
@Wumpf Wumpf deleted the andreas/global-spaceview-system-registration branch December 15, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants