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.

Sign up for GitHub

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

Automatically select user timeline if no timeline was explicitly selected yet #2986

Merged
merged 3 commits into from
Aug 16, 2023
+65 −39
Merged

Automatically select user timeline if no timeline was explicitly selected yet #2986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cd470e
Automatically select user timeline if no timeline was explicitly sele…
Wumpf Aug 15, 2023
17cb433
clarify code that sets default timeline
Wumpf Aug 15, 2023
5e49b00
implement deref for EditableAutoValue
Wumpf Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions crates/re_data_store/src/editable_auto_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ where
T: std::fmt::Debug + Clone + Default + PartialEq + serde::Serialize,
for<'de2> T: serde::Deserialize<'de2>,
{
#[inline]
fn default() -> Self {
EditableAutoValue::Auto(T::default())
}
Expand All @@ -30,18 +31,21 @@ where
T: std::fmt::Debug + Clone + Default + PartialEq + serde::Serialize,
for<'de2> T: serde::Deserialize<'de2>,
{
#[inline]
pub fn is_auto(&self) -> bool {
matches!(self, EditableAutoValue::Auto(_))
}

/// Gets the value, disregarding if it was user edited or determined by a heuristic.
#[inline]
pub fn get(&self) -> &T {
match self {
EditableAutoValue::Auto(v) | EditableAutoValue::UserEdited(v) => v,
}
}

/// Returns other if self is auto, self otherwise.
#[inline]
pub fn or<'a>(&'a self, other: &'a EditableAutoValue<T>) -> &'a EditableAutoValue<T> {
if self.is_auto() {
other
Expand All @@ -52,6 +56,7 @@ where

/// Determine whether this `EditableAutoValue` has user-edits relative to another `EditableAutoValue`
/// If both values are `Auto`, then it is not considered edited.
#[inline]
pub fn has_edits(&self, other: &Self) -> bool {
match (self, other) {
(EditableAutoValue::UserEdited(s), EditableAutoValue::UserEdited(o)) => s != o,
Expand All @@ -60,3 +65,16 @@ where
}
}
}

impl<T> std::ops::Deref for EditableAutoValue<T>
where
T: std::fmt::Debug + Clone + Default + PartialEq + serde::Serialize,
for<'de2> T: serde::Deserialize<'de2>,
{
type Target = T;

#[inline]
fn deref(&self) -> &Self::Target {
self.get()
}
}
86 changes: 47 additions & 39 deletions crates/re_viewer_context/src/time_control.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::collections::{BTreeMap, BTreeSet};

use re_data_store::TimesPerTimeline;
use re_data_store::{EditableAutoValue, TimesPerTimeline};
use re_log_types::{Duration, TimeInt, TimeRange, TimeRangeF, TimeReal, TimeType, Timeline};

use crate::NeedsRepaint;
Expand Down Expand Up @@ -83,7 +83,7 @@ pub enum PlayState {
#[serde(default)]
pub struct TimeControl {
/// Name of the timeline (e.g. "log_time").
timeline: Timeline,
timeline: EditableAutoValue<Timeline>,

states: BTreeMap<Timeline, TimeState>,

Expand Down Expand Up @@ -133,7 +133,7 @@ impl TimeControl {
// then the user decides to switch timelines.
// When they do so, it might be the case that they switch to a timeline they've
// never interacted with before, in which case we don't even have a time state yet.
self.states.entry(self.timeline).or_insert_with(|| {
self.states.entry(*self.timeline).or_insert_with(|| {
TimeState::new(if self.following {
full_range.max
} else {
Expand All @@ -147,7 +147,7 @@ impl TimeControl {

let state = self
.states
.entry(self.timeline)
.entry(*self.timeline)
.or_insert_with(|| TimeState::new(full_range.min));

if self.looping == Looping::Off && full_range.max <= state.time {
Expand Down Expand Up @@ -190,7 +190,7 @@ impl TimeControl {
}
PlayState::Following => {
// Set the time to the max:
match self.states.entry(self.timeline) {
match self.states.entry(*self.timeline) {
std::collections::btree_map::Entry::Vacant(entry) => {
entry.insert(TimeState::new(full_range.max));
}
Expand Down Expand Up @@ -241,24 +241,24 @@ impl TimeControl {
self.following = false;

// Start from beginning if we are at the end:
if let Some(time_points) = times_per_timeline.get(&self.timeline) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(time_points) = times_per_timeline.get(self.timeline.get()) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
if max(time_points) <= state.time {
state.time = min(time_points).into();
}
} else {
self.states
.insert(self.timeline, TimeState::new(min(time_points)));
.insert(*self.timeline, TimeState::new(min(time_points)));
}
}
}
PlayState::Following => {
self.playing = true;
self.following = true;

if let Some(time_points) = times_per_timeline.get(&self.timeline) {
if let Some(time_points) = times_per_timeline.get(self.timeline.get()) {
// Set the time to the max:
match self.states.entry(self.timeline) {
match self.states.entry(*self.timeline) {
std::collections::btree_map::Entry::Vacant(entry) => {
entry.insert(TimeState::new(max(time_points)));
}
Expand Down Expand Up @@ -308,8 +308,8 @@ impl TimeControl {
}

pub fn restart(&mut self, times_per_timeline: &TimesPerTimeline) {
if let Some(time_points) = times_per_timeline.get(&self.timeline) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(time_points) = times_per_timeline.get(self.timeline.get()) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
state.time = min(time_points).into();
self.following = false;
}
Expand Down Expand Up @@ -344,8 +344,8 @@ impl TimeControl {
// the beginning in play mode.

// Start from beginning if we are at the end:
if let Some(time_points) = times_per_timeline.get(&self.timeline) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(time_points) = times_per_timeline.get(self.timeline.get()) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
if max(time_points) <= state.time {
state.time = min(time_points).into();
self.playing = true;
Expand Down Expand Up @@ -375,33 +375,39 @@ impl TimeControl {

/// playback fps
pub fn fps(&self) -> Option<f32> {
self.states.get(&self.timeline).map(|state| state.fps)
self.states.get(self.timeline()).map(|state| state.fps)
}

/// playback fps
pub fn set_fps(&mut self, fps: f32) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
state.fps = fps;
}
}

/// Make sure the selected timeline is a valid one
pub fn select_a_valid_timeline(&mut self, times_per_timeline: &TimesPerTimeline) {
for timeline in times_per_timeline.timelines() {
if &self.timeline == timeline {
return; // it's valid
fn is_timeline_valid(selected: &Timeline, times_per_timeline: &TimesPerTimeline) -> bool {
for timeline in times_per_timeline.timelines() {
if selected == timeline {
return true; // it's valid
}
}
false
}
if let Some(timeline) = default_timeline(times_per_timeline.timelines()) {
self.timeline = *timeline;
} else {
self.timeline = Default::default();

// If the timeline is auto refresh it every frame, otherwise only pick a new one if invalid.
if self.timeline.is_auto() || !is_timeline_valid(self.timeline(), times_per_timeline) {
self.timeline = EditableAutoValue::Auto(
default_timeline(times_per_timeline.timelines()).map_or(Default::default(), |t| *t),
Copy link
Member

Choose a reason for hiding this comment

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

The alphabetical order behavior is a bit unexpected (though I agree still a net improvement).

Since times_per_timeline persists with the EntityDb, internally tracking insertion order as part of TimesPerTimeline::insert() wouldn't be all that painful though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I think you're right. I'll try to go the extra step tomorrow

Copy link
Member Author

Choose a reason for hiding this comment

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

poked it a bit, but changing this BTreeMap of timelines right now is a bit too annoying for the moment and then it gets less viable for a patch release

);
}
}

/// The currently selected timeline
#[inline]
pub fn timeline(&self) -> &Timeline {
&self.timeline
self.timeline.get()
}

/// The time type of the currently selected timeline
Expand All @@ -410,12 +416,12 @@ impl TimeControl {
}

pub fn set_timeline(&mut self, timeline: Timeline) {
self.timeline = timeline;
self.timeline = EditableAutoValue::UserEdited(timeline);
}

/// The current time.
pub fn time(&self) -> Option<TimeReal> {
self.states.get(&self.timeline).map(|state| state.time)
self.states.get(self.timeline()).map(|state| state.time)
}

/// The current time.
Expand All @@ -431,43 +437,43 @@ impl TimeControl {
/// Query for latest value at the currently selected time on the currently selected timeline.
pub fn current_query(&self) -> re_arrow_store::LatestAtQuery {
re_arrow_store::LatestAtQuery::new(
self.timeline,
*self.timeline,
self.time().map_or(TimeInt::MAX, |t| t.floor()),
)
}

/// The current loop range, iff selection looping is turned on.
pub fn active_loop_selection(&self) -> Option<TimeRangeF> {
if self.looping == Looping::Selection {
self.states.get(&self.timeline)?.loop_selection
self.states.get(self.timeline())?.loop_selection
} else {
None
}
}

/// The full range of times for the current timeline
pub fn full_range(&self, times_per_timeline: &TimesPerTimeline) -> Option<TimeRange> {
times_per_timeline.get(&self.timeline).map(range)
times_per_timeline.get(self.timeline()).map(range)
}

/// The selected slice of time that is called the "loop selection".
///
/// This can still return `Some` even if looping is currently off.
pub fn loop_selection(&self) -> Option<TimeRangeF> {
self.states.get(&self.timeline)?.loop_selection
self.states.get(self.timeline())?.loop_selection
}

/// Set the current loop selection without enabling looping.
pub fn set_loop_selection(&mut self, selection: TimeRangeF) {
self.states
.entry(self.timeline)
.entry(*self.timeline)
.or_insert_with(|| TimeState::new(selection.min))
.loop_selection = Some(selection);
}

/// Remove the current loop selection.
pub fn remove_loop_selection(&mut self) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
state.loop_selection = None;
}
if self.looping() == Looping::Selection {
Expand All @@ -477,47 +483,49 @@ impl TimeControl {

/// Is the current time in the selection range (if any), or at the current time mark?
pub fn is_time_selected(&self, timeline: &Timeline, needle: TimeInt) -> bool {
if timeline != &self.timeline {
if timeline != self.timeline() {
return false;
}

if let Some(state) = self.states.get(&self.timeline) {
if let Some(state) = self.states.get(self.timeline()) {
state.time.floor() == needle
} else {
false
}
}

pub fn set_timeline_and_time(&mut self, timeline: Timeline, time: impl Into<TimeReal>) {
self.timeline = timeline;
self.timeline = EditableAutoValue::UserEdited(timeline);
self.set_time(time);
}

pub fn set_time(&mut self, time: impl Into<TimeReal>) {
let time = time.into();

self.states
.entry(self.timeline)
.entry(*self.timeline)
.or_insert_with(|| TimeState::new(time))
.time = time;
}

/// The range of time we are currently zoomed in on.
pub fn time_view(&self) -> Option<TimeView> {
self.states.get(&self.timeline).and_then(|state| state.view)
self.states
.get(self.timeline())
.and_then(|state| state.view)
}

/// The range of time we are currently zoomed in on.
pub fn set_time_view(&mut self, view: TimeView) {
self.states
.entry(self.timeline)
.entry(*self.timeline)
.or_insert_with(|| TimeState::new(view.min))
.view = Some(view);
}

/// The range of time we are currently zoomed in on.
pub fn reset_time_view(&mut self) {
if let Some(state) = self.states.get_mut(&self.timeline) {
if let Some(state) = self.states.get_mut(self.timeline.get()) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing all &foo to foo.get(), just implement Deref for EditableAutoValue!

Copy link
Member Author

Choose a reason for hiding this comment

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

🤯

Copy link
Member Author

Choose a reason for hiding this comment

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

for those cases I still need get() due to the way Deref works in rust: *self.timeline gives the underlying Timeline by value. But with Deref in place *self.timeline.get() becomes *self.timeline and self.timeline.get().typ() becomes self.timeline.typ()

Copy link
Member Author

Choose a reason for hiding this comment

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

could ofc also do &*self.timeline.get() if that's preferred

Copy link
Member

Choose a reason for hiding this comment

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

I mean do &*self.timeline

state.view = None;
}
}
Expand Down