-
Notifications
You must be signed in to change notification settings - Fork 373
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
Support modifying the plot style by introducing a generic framework for overriding components #4914
Conversation
ce587dc
to
907c520
Compare
37223e7
to
46d0a54
Compare
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.
absolutely marvelous and not as complex as I feared it would be.
Comments are mostly starts of discussions, some of which we should have over Zoom or Slack instead, some of which need to be postponed.
The only thing that I believe is actually wrong is how initial overrides are determined (see comments). But since this won't affect the time series space view, I'll approve anyways (:
fn edit_color_ui( | ||
ctx: &ViewerContext<'_>, | ||
ui: &mut egui::Ui, | ||
_verbosity: UiVerbosity, |
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.
I wonder if ui verbosity ever makes sense for editors. Aren't they always only available at full verbosity?
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.
I think it will eventually. I suspect there will be both a Small/Inline editor for some things, when it's appropriate, but I could imagine a way of expanding that to a more "full" multiline editor that we won't always want to show in the selection panel by default.
let override_color = lookup_override::<Color>(data_result, ctx).map(|c| { | ||
let arr = c.to_array(); | ||
egui::Color32::from_rgba_unmultiplied(arr[0], arr[1], arr[2], arr[3]) | ||
}); | ||
|
||
let override_label = | ||
lookup_override::<Text>(data_result, ctx).map(|t| t.to_string()); | ||
|
||
let override_scattered = | ||
lookup_override::<ScalarScattering>(data_result, ctx).map(|s| s.0); | ||
|
||
let override_radius = lookup_override::<Radius>(data_result, ctx).map(|r| r.0); |
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.
let's discuss after this PR how we can make this pattern nice, easy and fast. Overall doesn't look as bad as I thought it would (:
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.
Yeah, it's not awful, esp since we restrict things to splats at the moment, but I'd still love to have it automagically happen inside one of the archetype-view-style generic queries.
} | ||
}) | ||
.or_else(|| { | ||
system_for_initial_value.initial_override_value( |
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.
this doesn't check out for me: The system for the initial value may not have brought up the component that we're on right now.
I think what you iterate instead is pairs of component-visualizer where you somehow removed duplicated components, i.e. decided which visualizer would provide the initial override value :/
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.
Aha! Yes, good catch. Changed to build a map of Component -> Visualizer and use the right now.
return; | ||
}; | ||
|
||
initial_data.compute_size_bytes(); |
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.
why is this needed?
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.
🤷 it's always needed when we write data to the store or else we get errors about using data that hasn't been sized yet.
What
This tackles the problem of allowing SeriesStyle components to be overridden by building out a significant chunk of a general component override framework.
The new UI is limited to TimeSeriesScalar view at the moment because it still requires special Visualizer-side handling of the component data.
High level idea
When selecting an entity within a space-view, we now have the ability to override any component that is both queried by the visualizer, and for which we have a registered component-editor.
This override value is initialized based on looking for, in order:
Notable changes:
update_overrides
process, we now checks the appropriate location in the blueprint override tree and insert the path to any overridden components into the DataResult.Screenshot
Checklist
main
build: app.rerun.ionightly
build: app.rerun.io