-
Notifications
You must be signed in to change notification settings - Fork 329
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
New debug option to show an actual timeline for the Blueprint #4609
Conversation
803fc04
to
810de30
Compare
d34d59e
to
7a5e641
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.
I like the functionality, but I wonder if we can improve the implementation.
I understand this is a debug-only feature (for now!) and doesn't have to be perfect, but debug-features has a tendency to become official features over time, and if we can come up with a better design now, it would be good.
I'm in particular not a huge fan of current_query_for_entity_path
. It is only sometimes used (current_query()
remains), and it uses not only the given entity path, but the global modifier show_blueprint_timeline
. This means changing which store to view in the streams panel can also change how the currently selected entity is shown in the selection panel.
I don't have an immediate better suggestion though. Baking in the StoreKind
or StoreId
into each selected Item
seems like the clean way to go, but it is a rather big refactor (and outside the scope of this PR).
/// Show a timeline of the blueprint data. | ||
#[cfg(debug_assertions)] | ||
ShowBlueprintTimeline(bool), |
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 find this misleading. It sounds like it opens a new panel with the time-stream of the blueprint data, but what it actually does is switch what store the streams panel shows.
I think this would be more clear if the bool
was changed to a stream_panel_store: StoreKind
, and this to
/// Show a timeline of the blueprint data. | |
#[cfg(debug_assertions)] | |
ShowBlueprintTimeline(bool), | |
/// Select what store is shown in the streams panel: the recording or the blueprint. | |
#[cfg(debug_assertions)] | |
SetActiveStreamPanelStore(StoreKind), |
/// If the blueprint timeline is visible, and the `entity_path` is on the blueprint | ||
/// timeline, then the blueprint timeline is used. Otherwise, return the regular | ||
/// timeline. | ||
pub fn current_query_for_entity_path( |
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 feels quite inelegant.
I guess the alternative would be for the current Item
selection to know what store it is on, i.e. that we don't select only an EntityPath
, but an StoreId + EntityPath
pair. I guess that's more work though, but did you consider it?
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, the need for this bit made me sad. I considered 2 options and both were further reaching than I wanted to tackle here:
- As you suggest: adding it to the Selection context and then plumbing the information down into every data_ui element.
- Introduce a GloballyQualifiedEntityPath (or equivalent name) which tracks StoreId + Path, and migrating many of our usages of EntityPath (specifically Selections) to use GQEP instead.
(2) is actually be preference at the moment because GQEP's with blueprint store-ids would also be helpful with the implementation of generalized hierarchical component-overrides that combine both log-centric (e.g. log-recursive) and blueprint-sourced overrides.
crates/re_data_ui/src/image.rs
Outdated
.query_latest_component::<DepthMeter>(entity_path, &ctx.current_query()) | ||
.query_latest_component::<DepthMeter>( | ||
entity_path, | ||
&ctx.current_query_for_entity_path(entity_path), |
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.
Will a tensor entity ever be on the blueprint timeline? I thought that was only applicable to stuff like containers and space view entities?
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 guess I'm a bit confused why some current_query()
has become current_query_for_entity_path
, but not all.
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.
Will a tensor entity ever be on the blueprint timeline?
Yes -- data overrides will be stored in the blueprint. So if you set an override, and then select the blueprint entity representing that override you'll hit this code path.
I guess I'm a bit confused why some current_query() has become current_query_for_entity_path, but not all.
I scoped my search & replace to the data_ui elements which is the only context in which it should theoretically matter .
"Show Blueprint in the Time Panel", | ||
).on_hover_text("Show the Blueprint data in the Time Panel tree view. This is useful for debugging the internal blueprint state."); | ||
&mut app_options.show_blueprint_timeline, | ||
"Show a timeline of the blueprint data.", |
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.
In the PR description this is called "blueprint inspection mode" which makes it sound more like a global mode, while "Show a timeline of the blueprint data." sounds like a change in one of the panels.
I think one problem is that while show_blueprint_timeline
mostly changes the streams panel, it also does other things, like change how current_query_for_entity_path
works.
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 "blueprint inspection mode" is probably the better name and goes along with the explanation that while in this mode, the blueprint can't be edited.
f188ae1
to
23ee728
Compare
@emilk based on feedback I ended up making a number of changes:
|
@@ -28,6 +28,7 @@ impl EntityDataUi for re_types::components::TensorData { | |||
verbosity: UiVerbosity, | |||
entity_path: &re_log_types::EntityPath, | |||
query: &re_data_store::LatestAtQuery, | |||
store: &re_data_store::DataStore, |
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.
NOTE: this is a personal style preference, so feel free to ignore.
When deciding parameters order, I usually go with "environmental first, specific last". ctx
is basically always the same, so it is always first. ui
is likewise part of the environment (its where we happen to add widgets). I would therefore have put store
before verbosity
.
This helps keep the argument order somewhat consistent across the code base (ctx, ui, store, entity_path, component_path, time, …). Of course, there is a lot of room for interpretation here.
Also, it'd be a pain in the ass to change this now, so feel free not to change anything - I just wanted to spread what I consider a good practice :)
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, you've mentioned that before and I had the thought in my head while writing this code. If I was adding both query and store, I would have been more inclined to add it right after ctx at the beginning. But I found it easier to keep query & store together as a pair for this.
Open to doing parameter order reshuffling in another refactor-only PR and moving them both to the front. I'm also inclined to introduce some kind of "DataQueryContext" that combines those things just to cut down on all the arguments, which might also eventually include things like caches and override stacks.
#4867) ### What This is the basic pattern we expect to see for all of these view-configuration type tasks: - Add a new archetype / component - Add logic to the TimeSeries space-view-class implementation that both reads and writes the components directly from the blueprint store. This generally seems to be much easier to think about than dealing with the EntityProperties struct as this was handled previously as it's exclusively a contact between the TimeSeries archetype and the TimeSeriesSpaceView implementation. Open question: - Should we suffix all our view archetypes with something like `TimeSeriesView`? Needs #4609 to land first since I developed there before rebasing to main. Will be nice to come back and clean this up again once #3384 is done. ### 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/4867/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4867/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/4867/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/4867) - [Docs preview](https://rerun.io/preview/d76ed7714f29407bc1d4a62779bfa2894a10fa90/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/d76ed7714f29407bc1d4a62779bfa2894a10fa90/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
What
Lots of goodies here:
In debug, Ctrl-Shift-I now opens this inspector, or it's available via rerun debug menu.
Checklist
main
build: app.rerun.ionightly
build: app.rerun.io