Add support for Visible History to time series space views #4179

abey79 · 2023-11-08T10:25:26Z

What

Add support for Visible History to Timeseries space views
- Needed to introduce DataStore::entity_min_time() to maintain the consistent "beginning of x axis" behaviour.
Changes the way the Visible History feature is organised:
- Now all space view (blueprint) contain a "root entity properties" structure that is cascaded to the enclosed groups and entities.
- The visible history part of that root entity props is editable for all supported space view classes (now 2d, 3d, and timeseries).
- The rest is unchanged: this means that contrary to the plan it's also possible to edit the visible history on a per-entity basis in timeseries space views as well.
Closes Allow to set a sliding window for time series plots (aka support the Visible History feature) #2547

Follow-up TODO (before 0.11)

Known limitations

default value for EntityProperty is not ideal for entities in timeseries space view (2x Relative(0))
Add support to skip the "latest_at" semantics with range queries #4192

TODO

freeze the plot display bounds while the time cursor is dragged
setting must be serialised to blueprint

Export-1699438955049.mp4

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 demo.rerun.io (if applicable)
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

nikolausWest · 2023-11-09T09:05:02Z

I don't really get why we would have a toggle for "Visual History". Shouldn't we just pre-fill the options that correspond to the default behavior for all the views? Then we can just make it editable for the views / data that support it.

For instance if we can't support visual history, we would just set the range to 0, 0 and have it not be editable. The time series plots would default to -♾️,+♾️

Fwiw, I think renaming this to something like "Included time range" or "visible time range" would make it more clear but I won't die on that hill.

… to use this instead of the ad hoc state in the timeseries space view.

nikolausWest · 2023-11-09T11:14:46Z

I think something like this would be easier to understand:

abey79 · 2023-11-09T11:23:45Z

I don't really get why we would have a toggle for "Visual History".

Two motivations I can think of:

Disabling the feature would be, in general, cumbersome (2x set to "Relative" and enter "0").
Make the cascading behaviour explicit. Now a child will override the parent (group, space view) if its Visible History is enabled (regardless of actual settings). If we remove that, then the cascading logic becomes much less obvious.

abey79 · 2023-11-09T11:36:03Z

I think something like this would be easier to understand

I like the reordering of controls. I'm not sure about merging the concepts of "infinite" with "absolute". "First + 10s" feels less useful than "t = 63s" (assuming the timelines first logged data would be at 53s, which could be accidental).

- keep plot bounds from changing while the time cursor is dragged - correct min time computation - don't draw the time cursor out of the plot area

# Conflicts: # crates/re_arrow_store/tests/correctness.rs

abey79 · 2023-11-09T19:36:58Z

crates/re_arrow_store/tests/correctness.rs

+    let positions = build_some_positions2d(3);
+    let colors = build_some_colors(3);


@teh-cmc I think this doesn't respect your prescription of using only stuff from re_log_types::example_components or re_types_core, but it's done all over the place in this file already 🤷🏻

Mistakes were made in the past, let's not repeat them 🙃

Done 👍🏻

crates/re_viewport/src/space_view.rs

teh-cmc

Played around with it a bit and it works great!

Please remove the use of positions and colors from the store tests before merging.

crates/re_viewer/src/ui/selection_panel.rs

crates/re_space_view_time_series/src/space_view_class.rs

crates/re_space_view_time_series/src/view_part_system.rs

teh-cmc · 2023-11-10T08:30:41Z

crates/re_viewer/src/ui/selection_panel.rs

@@ -309,13 +321,13 @@ fn blueprint_ui(
                        // TODO(emilk): show the values of this specific instance (e.g. point in the point cloud)!
                    } else {
                        // splat - the whole entity
-                        let space_view_class_name = *space_view.class_name();
+                        let space_view_class = *space_view.class_name();


Why rename here? The previous one was better imo.

~~Yeah I went back and forth with these and this one clear got forgotten.~~

Edit: actually no, space_view_class was used everywhere in this file before, I just made this one consistent. This naming choice (which also wasn't my preferred) wins by 42 to 15 throughout the codebase, so best keep it that way for now imo.

crates/re_viewport/src/space_view.rs

Add support for Visible History to timeseries space views

9a84d12

abey79 added ui concerns graphical user interface include in changelog labels Nov 8, 2023

abey79 added 2 commits November 9, 2023 11:14

Introduce root entity props in the space view blueprints and refactor…

8d63961

… to use this instead of the ad hoc state in the timeseries space view.

Misc fixes for lint and diff clean-up

1a04d1d

Several improvements:

d838b47

- keep plot bounds from changing while the time cursor is dragged - correct min time computation - don't draw the time cursor out of the plot area

abey79 mentioned this pull request Nov 9, 2023

Tracking issue: Visible History feature #4107

Open

21 tasks

abey79 added 3 commits November 9, 2023 20:09

Clarifying comment

d3c6df1

Merge branch 'main' into antoine/visible-history-timeseries

015e3f7

# Conflicts: # crates/re_arrow_store/tests/correctness.rs

Post-merge fixes

210b681

abey79 commented Nov 9, 2023

View reviewed changes

[Ugly] hard-code better defaults for time series space views

33e6c16

abey79 commented Nov 9, 2023

View reviewed changes

crates/re_viewport/src/space_view.rs Outdated Show resolved Hide resolved

abey79 marked this pull request as ready for review November 9, 2023 20:02

abey79 requested a review from teh-cmc November 9, 2023 20:15

teh-cmc approved these changes Nov 10, 2023

View reviewed changes

abey79 mentioned this pull request Nov 10, 2023

Support per-space-view-class property defaults #4194

Open

Review comments

8c2be67

abey79 merged commit a4cf644 into main Nov 10, 2023
37 checks passed

abey79 deleted the antoine/visible-history-timeseries branch November 10, 2023 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Visible History to time series space views #4179

Add support for Visible History to time series space views #4179

abey79 commented Nov 8, 2023 •

edited

nikolausWest commented Nov 9, 2023 •

edited

nikolausWest commented Nov 9, 2023

abey79 commented Nov 9, 2023

abey79 commented Nov 9, 2023

abey79 Nov 9, 2023

teh-cmc Nov 10, 2023

abey79 Nov 10, 2023

teh-cmc left a comment

teh-cmc Nov 10, 2023

abey79 Nov 10, 2023 •

edited

		let positions = build_some_positions2d(3);
		let colors = build_some_colors(3);

Add support for Visible History to time series space views #4179

Add support for Visible History to time series space views #4179

Conversation

abey79 commented Nov 8, 2023 • edited

What

Follow-up TODO (before 0.11)

Known limitations

TODO

Checklist

nikolausWest commented Nov 9, 2023 • edited

nikolausWest commented Nov 9, 2023

abey79 commented Nov 9, 2023

abey79 commented Nov 9, 2023

abey79 Nov 9, 2023

Choose a reason for hiding this comment

teh-cmc Nov 10, 2023

Choose a reason for hiding this comment

abey79 Nov 10, 2023

Choose a reason for hiding this comment

teh-cmc left a comment

Choose a reason for hiding this comment

teh-cmc Nov 10, 2023

Choose a reason for hiding this comment

abey79 Nov 10, 2023 • edited

Choose a reason for hiding this comment

abey79 commented Nov 8, 2023 •

edited

nikolausWest commented Nov 9, 2023 •

edited

abey79 Nov 10, 2023 •

edited