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

Make Visible History UI more ergonomic and show inherited values #4222

Merged
merged 14 commits into from
Nov 16, 2023

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Nov 14, 2023

What

Update the Visible History UI:

Side effects on the code base:

  • slight tweak on the querying (latest at is used whenever visible time range boundaries resolve to the same absolute time)
  • added individual_properties to DataResult, to allow UI to display/edit it
  • added support to erase property overrides in DataResult::save_override
  • added a new collapsing header widget in re_ui, to match our style

Playground RRD: test_visible_history.rrd.zip (same as the output of tests/python/visible_history_playground/main.py).

Screenshots

New "Visible Time Range" UI in the blueprint section:

image

When Override is selected:

image

When operating on "large" time (e.g. log_time):

image

Same when using the Default/Inherited value:

image

Older screenshots which aged like milk, for posterity.

On (supported) space views, the choice is "Default vs. Override":

image

On groups and entities, the choice is "Inherited vs. Override":

image

On timeseries space views, the default reflects the fact that the default behaviour is, in fact, different from 2D/3D space views

image

When overriding, the text becomes "editable":

image

Same, when inherited:

image

With timelines with "large" times, an offset is extracted and explicitly displayed, while the unit in the time editor adapts to the actual span:

image

Same inherited:

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

- fixed cascading behaviour of VH entity props
- added many timelines to example
@abey79 abey79 added ui concerns graphical user interface include in changelog labels Nov 14, 2023
@abey79 abey79 marked this pull request as draft November 14, 2023 13:23
@abey79 abey79 marked this pull request as ready for review November 14, 2023 15:45
@teh-cmc teh-cmc self-requested a review November 15, 2023 08:08
@teh-cmc
Copy link
Member

teh-cmc commented Nov 15, 2023

Played with it quite a bit, this is frankly amazing; such a giant leap in UX 😱

My only complaint UX-wise is when using absolute offsets:
image

I would expect the timestamp part to simply tell me where that offset puts me, not force me to do crazy mental gymnastics.

In fact the same applies to all other modes (relative/absolute/extremums), and isn't even specific to temporal timelines:
image
image

I'd expect not only to be shown an offsets, but also a grayed out value indicating what the final value is with that offset applied.

@teh-cmc
Copy link
Member

teh-cmc commented Nov 15, 2023

Also the thingy goes crazy if the min temporal value matches the max temporal value for some reason:
image
image

@nikolausWest
Copy link
Member

I think we should have "Visible Time Range" be a sub-tree to "Blueprint" rather than at the same hierarchical level while still being collapsible
Screenshot 2023-11-15 at 09 28 56

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.

Most of the edge-case handling code goes way above my head but LGTM otherwise.

As I've pointed out in earlier comments there are a couple UI hiccups but in any case this is such a massive improvement overall that I wouldn't consider them blockers. Your call!

crates/re_viewer/src/ui/visible_history.rs Outdated Show resolved Hide resolved
@nikolausWest
Copy link
Member

I find the distinction here between default and inherited a bit confusing. Particularly since it seems to default to "inherited" for 2D/3D views and "default" for timeli
Screenshot 2023-11-15 at 09 42 44
Screenshot 2023-11-15 at 09 42 14
ne.

It's also quite confusing that it says inherited but

  1. not where it's from
  2. it may actually not even be inheriting anything.

One option solution be to make the choice be between default and override and then write out "Inherited from " if the value comes from a parent

@nikolausWest
Copy link
Member

nikolausWest commented Nov 15, 2023

Screenshot 2023-11-15 at 09 51 21

I understand why you chose this behavior but I definitely find it conceptually confusing although probably practical. My prediction is we will make this configurable in the future. In the meantime I think this needs a more detailed description on hover. In the case where default means you'll do a "latest at" query instead of a range query, spelling that out is probably useful.

@nikolausWest
Copy link
Member

I think this state needs some extra explanation:
Screenshot 2023-11-15 at 09 55 00

It's not immediately obvious that what this actually means is that you will not do a range query but instead do a latest at query for the current time point

@nikolausWest
Copy link
Member

Not for this PR but it's pretty obvious that visualizing what the time range is will make this a lot easier to understand

@abey79
Copy link
Member Author

abey79 commented Nov 15, 2023

Screenshot 2023-11-15 at 09 51 21 I understand why you chose this behavior but I definitely find it conceptually confusing although probably practical. My prediction is we will make this configurable in the future. In the meantime I think this needs a more detailed description on hover. In the case where default means you'll do a "latest at" query instead of a range query, spelling that out is probably useful.

Oh, I definitely did not make that choice! That's how the entire blueprint data model is currently implemented (ie. entity properties are saved per item per timeline type, as opposed to per timeline). The UI just reflects this state of affair. Should the blueprint data model change, I'll be glad to remove this note.

@abey79
Copy link
Member Author

abey79 commented Nov 15, 2023

I'd expect not only to be shown an offsets, but also a grayed out value indicating what the final value is with that offset applied.

I was thinking of doing something like that indeed, but it somehow overlaps with #4141. I might punt on that one in that PR, then move to the display, then we can reconsider if we need further context.

I think we should have "Visible Time Range" be a sub-tree to "Blueprint" rather than at the same hierarchical level while still being collapsible

OK.

Not for this PR but it's pretty obvious that visualizing what the time range is will make this a lot easier to understand

I think this state needs some extra explanation: Screenshot 2023-11-15 at 09 55 00

It's not immediately obvious that what this actually means is that you will not do a range query but instead do a latest at query for the current time point

In general, "from A to B" always means "from A to B if there is anything, else whatever was logged last before A". I could add a note somewhere along the lines of "When no data exists in the interval, the data last logged before the interval is shown".

@abey79
Copy link
Member Author

abey79 commented Nov 15, 2023

I find the distinction here between default and inherited a bit confusing. Particularly since it seems to default to "inherited" for 2D/3D views and "default" for timeli Screenshot 2023-11-15 at 09 42 44 Screenshot 2023-11-15 at 09 42 14 ne.

It's also quite confusing that it says inherited but

  1. not where it's from
  2. it may actually not even be inheriting anything.

One option solution be to make the choice be between default and override and then write out "Inherited from " if the value comes from a parent

I sense bi-directional misunderstanding here—let's have a chat about this one post-standup (or whenever).

@abey79
Copy link
Member Author

abey79 commented Nov 15, 2023

Also the thingy goes crazy if the min temporal value matches the max temporal value for some reason.

Fixed.

@nikolausWest
Copy link
Member

nikolausWest commented Nov 16, 2023

To fix in this or a follow up PR: initialize the override with the defaults so you don't get this jumping behavior:

Maintain.defaults.mp4

@nikolausWest
Copy link
Member

Bug after

  1. Set override visual history
  2. Drag time cursor in the plot
Bug.after.dragging.time.cursor.in.plot.mp4

@abey79
Copy link
Member Author

abey79 commented Nov 16, 2023

To fix in this or a follow up PR: initialize the override with the defaults so you don't get this jumping behavior:

That makes sense, I'll look into it.

Bug after

  1. Set override visual history
  2. Drag time cursor in the plot

This is somewhat tricky and has to do with egui plots "auto-bounds" behaviour. It gets disengaged when interacting with the plot (like when you move the time cursor), which is why the view then stops tracking the plot. It would be the same if you had manually zoomed in/out or scrolled. BTW, you can re-engage auto-bounds by double-clicking in the plot area.

I'll see what I can do, but that's definitely out of scope for this PR. I opened an issue:

Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

This looks good. Going to land so I can handle merging with my stuff as well.

@jleibs jleibs merged commit f586c04 into main Nov 16, 2023
39 checks passed
@jleibs jleibs deleted the antoine/refreshed-visible-history-ui branch November 16, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
4 participants