From f532950a01f83165fbb9d01d4c1ffa12995aecff Mon Sep 17 00:00:00 2001 From: Antoine Beyeler <49431240+abey79@users.noreply.github.com> Date: Mon, 2 Sep 2024 20:38:30 +0200 Subject: [PATCH] Fix `re_types_core::datatypes::TimeInt::MIN` and avoid using it for `TimeDragValue` (#7334) ### What This PR does two things: 1. fix `re_types_core::datatypes::TimeIn::MIN`, which was different from `re_log_types::TimeInt::MIN` (`i64::MIN` instead of `i64::MIN+1`, the former being reserved for `static`). 2. use `re_log_types::TimeInt` for `TimeDragValue`. Background for (2): the visible time range feature in selection panel heavily relies on `re_types_core::datatypes::TimeInt` because it uses blueprint codegen'd structures. Historically, this leaked into `TimeDragValue`, which originated there. Now `TimeDragValue` is used in several other places (which is why it was moved to `re_viewer_context`), so it best uses the "correct" `TimeInt` for its API. ### 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 examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7334?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7334?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 * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/7334) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --- .../src/datatypes/time_int_ext.rs | 3 +- .../re_chunk_store_ui/src/chunk_list_mode.rs | 5 ++- .../src/visible_time_range_ui.rs | 26 ++++++++------ .../src/query_kind_ui.rs | 18 ++++------ .../re_viewer_context/src/time_drag_value.rs | 36 +++++++++++++------ 5 files changed, 53 insertions(+), 35 deletions(-) diff --git a/crates/store/re_types_core/src/datatypes/time_int_ext.rs b/crates/store/re_types_core/src/datatypes/time_int_ext.rs index 6d46ec4f3f64..4affd17ae1e9 100644 --- a/crates/store/re_types_core/src/datatypes/time_int_ext.rs +++ b/crates/store/re_types_core/src/datatypes/time_int_ext.rs @@ -1,7 +1,8 @@ use super::TimeInt; impl TimeInt { - pub const MIN: Self = Self(i64::MIN); + // matches `re_log_types::TimeInt::MIN` + pub const MIN: Self = Self(i64::MIN + 1); pub const MAX: Self = Self(i64::MAX); } diff --git a/crates/viewer/re_chunk_store_ui/src/chunk_list_mode.rs b/crates/viewer/re_chunk_store_ui/src/chunk_list_mode.rs index 1658d54c3041..f8a27a465546 100644 --- a/crates/viewer/re_chunk_store_ui/src/chunk_list_mode.rs +++ b/crates/viewer/re_chunk_store_ui/src/chunk_list_mode.rs @@ -2,8 +2,7 @@ use std::ops::RangeInclusive; use re_chunk_store::external::re_chunk::ComponentName; use re_chunk_store::ChunkStore; -use re_log_types::external::re_types_core::datatypes::TimeInt; -use re_log_types::{EntityPath, ResolvedTimeRange, TimeType, TimeZone, Timeline}; +use re_log_types::{EntityPath, ResolvedTimeRange, TimeInt, TimeType, TimeZone, Timeline}; use re_ui::UiExt; use re_viewer_context::TimeDragValue; @@ -178,7 +177,7 @@ impl ChunkListMode { }; } ChunkListQueryMode::Range(range) => { - let (mut min, mut max) = (range.min().into(), range.max().into()); + let (mut min, mut max) = (range.min(), range.max()); ui.label("from:"); match time_typ { TimeType::Time => { diff --git a/crates/viewer/re_selection_panel/src/visible_time_range_ui.rs b/crates/viewer/re_selection_panel/src/visible_time_range_ui.rs index 0fbb6afc57da..f73a8fa9cc5c 100644 --- a/crates/viewer/re_selection_panel/src/visible_time_range_ui.rs +++ b/crates/viewer/re_selection_panel/src/visible_time_range_ui.rs @@ -540,15 +540,16 @@ fn visible_history_boundary_ui( let low_bound_override = if low_bound { None } else { - Some(other_boundary_absolute - current_time) + Some((other_boundary_absolute - current_time).into()) }; - match time_type { + let mut edit_value = (*value).into(); + let response = match time_type { TimeType::Time => Some( time_drag_value .temporal_drag_value_ui( ui, - value, + &mut edit_value, false, low_bound_override, ctx.app_options.time_zone, @@ -561,27 +562,30 @@ fn visible_history_boundary_ui( ), TimeType::Sequence => Some( time_drag_value - .sequence_drag_value_ui(ui, value, false, low_bound_override) + .sequence_drag_value_ui(ui, &mut edit_value, false, low_bound_override) .on_hover_text( "Number of frames before/after the current time to use a time \ range boundary", ), ), - } + }; + *value = edit_value.into(); + response } TimeRangeBoundary::Absolute(value) => { // see note above let low_bound_override = if low_bound { None } else { - Some(other_boundary_absolute) + Some(other_boundary_absolute.into()) }; - match time_type { + let mut edit_value = (*value).into(); + let response = match time_type { TimeType::Time => { let (drag_resp, base_time_resp) = time_drag_value.temporal_drag_value_ui( ui, - value, + &mut edit_value, true, low_bound_override, ctx.app_options.time_zone, @@ -595,10 +599,12 @@ fn visible_history_boundary_ui( } TimeType::Sequence => Some( time_drag_value - .sequence_drag_value_ui(ui, value, true, low_bound_override) + .sequence_drag_value_ui(ui, &mut edit_value, true, low_bound_override) .on_hover_text("Absolute frame number to use as time range boundary"), ), - } + }; + *value = edit_value.into(); + response } TimeRangeBoundary::Infinite => None, }; diff --git a/crates/viewer/re_space_view_dataframe/src/query_kind_ui.rs b/crates/viewer/re_space_view_dataframe/src/query_kind_ui.rs index b0be3a92908a..7b6766345224 100644 --- a/crates/viewer/re_space_view_dataframe/src/query_kind_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/query_kind_ui.rs @@ -43,8 +43,7 @@ impl UiQueryKind { *time } else { TimeInt::MAX - } - .into(); + }; changed |= match time_type { TimeType::Time => time_drag_value @@ -63,7 +62,7 @@ impl UiQueryKind { }; if changed { - *self = Self::LatestAt { time: time.into() }; + *self = Self::LatestAt { time }; } } }); @@ -101,15 +100,15 @@ impl UiQueryKind { ui.add_space(-4.0); let mut from = if let Self::TimeRange { from, .. } = self { - (*from).into() + *from } else { - (*time_drag_value.range.start()).into() + time_drag_value.min_time() }; let mut to = if let Self::TimeRange { to, .. } = self { - (*to).into() + *to } else { - (*time_drag_value.range.end()).into() + time_drag_value.max_time() }; list_item::list_item_scope(ui, "time_range_custom_scope", |ui| { @@ -165,10 +164,7 @@ impl UiQueryKind { }); if changed { - *self = Self::TimeRange { - from: from.into(), - to: to.into(), - }; + *self = Self::TimeRange { from, to }; } if should_display_time_range { diff --git a/crates/viewer/re_viewer_context/src/time_drag_value.rs b/crates/viewer/re_viewer_context/src/time_drag_value.rs index 1f7788795f63..288629852ecf 100644 --- a/crates/viewer/re_viewer_context/src/time_drag_value.rs +++ b/crates/viewer/re_viewer_context/src/time_drag_value.rs @@ -3,8 +3,7 @@ use std::ops::RangeInclusive; use egui::{NumExt as _, Response}; use re_entity_db::TimeHistogram; -use re_log_types::{TimeType, TimeZone}; -use re_types_core::datatypes::TimeInt; +use re_log_types::{TimeInt, TimeType, TimeZone}; /// Drag value widget for editing time values for both sequence and temporal timelines. /// @@ -64,6 +63,16 @@ impl TimeDragValue { } } + /// Return the minimum time set for this drag value. + pub fn min_time(&self) -> TimeInt { + TimeInt::new_temporal(*self.range.start()) + } + + /// Return the maximum time set for this drag value. + pub fn max_time(&self) -> TimeInt { + TimeInt::new_temporal(*self.range.end()) + } + /// Show a sequence drag value widget. pub fn sequence_drag_value_ui( &self, @@ -83,14 +92,19 @@ impl TimeDragValue { let speed = (span as f32 * 0.005).at_least(1.0); if let Some(low_bound_override) = low_bound_override { - time_range = low_bound_override.0.at_least(*time_range.start())..=*time_range.end(); + time_range = + low_bound_override.as_i64().at_least(*time_range.start())..=*time_range.end(); } - ui.add( - egui::DragValue::new(&mut value.0) + let mut value_i64 = value.as_i64(); + let response = ui.add( + egui::DragValue::new(&mut value_i64) .range(time_range) .speed(speed), - ) + ); + *value = TimeInt::new_temporal(value_i64); + + response } /// Show a temporal drag value widget. @@ -127,10 +141,11 @@ impl TimeDragValue { let speed = (time_range.end() - time_range.start()) as f32 / factor * 0.005; if let Some(low_bound_override) = low_bound_override { - time_range = low_bound_override.0.at_least(*time_range.start())..=*time_range.end(); + time_range = + low_bound_override.as_i64().at_least(*time_range.start())..=*time_range.end(); } - let mut time_unit = (value.0.saturating_sub(offset)) as f32 / factor; + let mut time_unit = (value.as_i64().saturating_sub(offset)) as f32 / factor; let time_range = (*time_range.start() - offset) as f32 / factor ..=(*time_range.end() - offset) as f32 / factor; @@ -139,7 +154,8 @@ impl TimeDragValue { self.base_time.map(|base_time| { ui.label(format!( "{} + ", - TimeType::Time.format(TimeInt(base_time), time_zone_for_timestamps) + TimeType::Time + .format(TimeInt::new_temporal(base_time), time_zone_for_timestamps) )) }) } else { @@ -153,7 +169,7 @@ impl TimeDragValue { .suffix(self.unit_symbol), ); - *value = TimeInt((time_unit * factor).round() as i64 + offset); + *value = TimeInt::new_temporal((time_unit * factor).round() as i64 + offset); (drag_value_response, base_time_response) }