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

Improve Visible History to support more general time queries #4123

Merged
merged 15 commits into from Nov 6, 2023

Conversation

abey79
Copy link
Contributor

@abey79 abey79 commented Nov 2, 2023

What

image

Scope

  • Improve the "Visible History" feature to build more general time queries:
    • Control both start/end time boundaries
    • relative/absolute/full mode for each boundaries
    • explicit active/inactive state/checkbox
  • Improve usability:
    • Proper scaling and bounds for the 'DragValue` widgets
    • Proper cascading behaviour from data group(s) to entity: a child's "visible history" settings will override the parent's if the feature is active.
    • Do not display Visible History UI in places where it's not supported

Out-of-scope

  • Add support for Visible History to space views other than 2D/3D
  • Add Visible History UI to 3D/2D space view themselves (for consistency with what precedes and to address some corner cases)
  • Add (read-only) shading in the time panel
  • Fix the numerous issues/missing features listed in:

TODO

  • fine-tune the UI layout
  • discuss/implement heuristics to decide whether or not to display the UI
  • add information notices (e.g. "this applies to ALL sequence timelines")

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

@abey79 abey79 added ui concerns graphical user interface include in changelog labels Nov 2, 2023
@abey79 abey79 marked this pull request as ready for review November 2, 2023 17:21
Comment on lines 502 to 537
static VISIBLE_HISTORY_COMPONENT_NAMES: once_cell::sync::Lazy<Vec<ComponentName>> =
once_cell::sync::Lazy::new(|| {
[
ComponentName::from("rerun.components.Position2D"),
ComponentName::from("rerun.components.Position3D"),
ComponentName::from("rerun.components.LineStrip2D"),
ComponentName::from("rerun.components.LineStrip3D"),
ComponentName::from("rerun.components.TensorData"),
ComponentName::from("rerun.components.Vector3D"),
ComponentName::from("rerun.components.HalfSizes2D"),
ComponentName::from("rerun.components.HalfSizes3D"),
]
.into()
});

fn should_display_visible_history(
ctx: &mut ViewerContext<'_>,
entity_path: Option<&EntityPath>,
) -> bool {
if let Some(entity_path) = entity_path {
let store = ctx.store_db.store();
let component_names = store.all_components(ctx.rec_cfg.time_ctrl.timeline(), entity_path);
if let Some(component_names) = component_names {
if !component_names
.iter()
.any(|name| VISIBLE_HISTORY_COMPONENT_NAMES.contains(name))
{
return false;
}
} else {
return false;
}
}

true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really how it should be done?

@teh-cmc teh-cmc self-requested a review November 3, 2023 08:02
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.

I've tried it on the web build but it doesn't seem to do anything?

I might be doing something wrong, but it's hard to tell without any on-hover pop-ups explaining what each button/label/etc does.

23-11-03_09.05.38.patched.mp4

@abey79
Copy link
Contributor Author

abey79 commented Nov 3, 2023

@teh-cmc looks like I've introduced a last minute bug 🤦🏻 Onto fixing it.

@Wumpf Wumpf self-requested a review November 3, 2023 15:45
@Wumpf
Copy link
Member

Wumpf commented Nov 3, 2023

would be nice to make the number pickers not allow nonsensical values like so:
image

@Wumpf
Copy link
Member

Wumpf commented Nov 3, 2023

I don't quite get why you made the Relative/Infinite things checkboxes since they clearly exclude each other. Also not being relative implying absolute took me a lil bit.
-> A dropdown box or radio buttons seem to be more appropriate

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

great feature overall, leveraging abilities we already have ⭐
code lgtm for the most part, the reason I put request changes is the ui concerns prior to the code review

crates/re_data_store/src/entity_properties.rs Outdated Show resolved Hide resolved
crates/re_data_store/src/entity_properties.rs Outdated Show resolved Hide resolved
crates/re_data_store/src/entity_properties.rs Outdated Show resolved Hide resolved
@@ -17,14 +17,15 @@ pub fn query_archetype_with_history<'a, A: Archetype + 'a, const N: usize>(
re_log_types::TimeType::Sequence => history.sequences,
};

if visible_history == 0 {
if !history.enabled || visible_history == VisibleHistory::OFF {
Copy link
Member

Choose a reason for hiding this comment

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

to clarify, when can it happen that one is off but the other is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How the visible history is stored is not fully normalised: there is a slight duplication:

  • the enabled/disabled bool
  • the fact that {from: Relative(0), to: Relative(0)} (that's what VisibleHistory::OFF is) disable the feature for all practical purposes.

I've introduced that "enabled" bool to allow the user to disable the feature without losing their configured boundaries (ie. that checkbox next to the Visible History label). Overall, better UX I believe, but the cost is this check.

BTW, the test against VisibleHistory::OFF could actually be dropped and the result would be the same. So it can be seen as an optimisation to avoid a range query when we know none is needed :)

crates/re_ui/src/lib.rs Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
abey79 and others added 3 commits November 3, 2023 17:19
Co-authored-by: Andreas Reich <andreas@rerun.io>
Co-authored-by: Andreas Reich <andreas@rerun.io>
Co-authored-by: Andreas Reich <andreas@rerun.io>
@abey79
Copy link
Contributor Author

abey79 commented Nov 3, 2023

A dropdown box or radio buttons seem to be more appropriate

Makes sense, I'll try that.

@abey79
Copy link
Contributor Author

abey79 commented Nov 4, 2023

would be nice to make the number pickers not allow nonsensical values like so: image

I attempted to do that with bf65bf6. It's kind of tricky though. Please give it a good try 🙏🏻

@Wumpf Wumpf self-requested a review November 6, 2023 12:46
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

love those improvements. worked great when I played around with it

@abey79 abey79 merged commit ce2493c into main Nov 6, 2023
37 checks passed
@abey79 abey79 deleted the antoine/better-visible-history branch November 6, 2023 14:56
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
Development

Successfully merging this pull request may close these issues.

From visual history to a general time query
4 participants