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

New debug option to show an actual timeline for the Blueprint #4609

Merged
merged 23 commits into from
Jan 19, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Dec 22, 2023

What

Lots of goodies here:

  • Add a new toggle which opens a second timeline showing a view of the blueprint store.
  • Introduce a Blueprint Timeline and log all blueprint events as temporal on the blueprint timeline.
  • Make it possible to query data using the blueprint timeline.
  • And last but not least, use the blueprint time control to drive all our viewport queries when we're in the blueprint inspection mode.

In debug, Ctrl-Shift-I now opens this inspector, or it's available via rerun debug menu.

image

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

@jleibs jleibs added the 🟦 blueprint The data that defines our UI label Dec 22, 2023
@jleibs jleibs force-pushed the jleibs/blueprint_timeline branch from 803fc04 to 810de30 Compare January 16, 2024 20:21
@jleibs jleibs changed the title Make the Blueprint debug timeline as awesome as it should be New debug option to show an actual timeline for the Blueprint Jan 16, 2024
@jleibs jleibs force-pushed the jleibs/blueprint_timeline branch from d34d59e to 7a5e641 Compare January 16, 2024 20:53
@jleibs jleibs added the 🧑‍💻 dev experience developer experience (excluding CI) label Jan 16, 2024
@jleibs jleibs marked this pull request as ready for review January 16, 2024 21:59
@emilk emilk self-requested a review January 17, 2024 08:00
Copy link
Member

@emilk emilk left a 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).

crates/re_viewer/src/app.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/app_state.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/app_state.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/rerun_menu.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/rerun_menu.rs Outdated Show resolved Hide resolved
Comment on lines 36 to 38
/// Show a timeline of the blueprint data.
#[cfg(debug_assertions)]
ShowBlueprintTimeline(bool),
Copy link
Member

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

Suggested change
/// 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(
Copy link
Member

@emilk emilk Jan 17, 2024

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?

Copy link
Member Author

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:

  1. As you suggest: adding it to the Selection context and then plumbing the information down into every data_ui element.
  2. 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.

.query_latest_component::<DepthMeter>(entity_path, &ctx.current_query())
.query_latest_component::<DepthMeter>(
entity_path,
&ctx.current_query_for_entity_path(entity_path),
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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 .

crates/re_time_panel/src/lib.rs Outdated Show resolved Hide resolved
"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.",
Copy link
Member

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.

Copy link
Member Author

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.

@jleibs jleibs force-pushed the jleibs/blueprint_timeline branch from f188ae1 to 23ee728 Compare January 18, 2024 18:34
@jleibs
Copy link
Member Author

jleibs commented Jan 18, 2024

@emilk based on feedback I ended up making a number of changes:

  • Rather than switching what the panel views, I actually open the blueprint as a second panel.
  • Plumbed a bunch of store/query contexts through data_ui elements so that those elements can be explicit about the store/query context of the data_ui rather than doing their own guessing/hard-coding internally.
  • I consolidated the logic for guessing the relevant store and query from the entity_path into a single helper function now scoped to selection_panel. We can get rid of this once Entities or Selections carry a bit more context, but this seemed like a reasonable compromise.

@@ -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,
Copy link
Member

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 :)

Copy link
Member Author

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.

@jleibs jleibs merged commit 2921f3c into main Jan 19, 2024
41 checks passed
@jleibs jleibs deleted the jleibs/blueprint_timeline branch January 19, 2024 13:46
jleibs added a commit that referenced this pull request Jan 19, 2024
#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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI 🧑‍💻 dev experience developer experience (excluding CI) include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Improvements: Blueprint-specific timeline view
2 participants