Skip to content

Commit

Permalink
Fix re_types_core::datatypes::TimeInt::MIN and avoid using it for `…
Browse files Browse the repository at this point in the history
…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`.
  • Loading branch information
abey79 authored Sep 2, 2024
1 parent 554ef56 commit f532950
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 35 deletions.
3 changes: 2 additions & 1 deletion crates/store/re_types_core/src/datatypes/time_int_ext.rs
Original file line number Diff line number Diff line change
@@ -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);
}

Expand Down
5 changes: 2 additions & 3 deletions crates/viewer/re_chunk_store_ui/src/chunk_list_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 => {
Expand Down
26 changes: 16 additions & 10 deletions crates/viewer/re_selection_panel/src/visible_time_range_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
};
Expand Down
18 changes: 7 additions & 11 deletions crates/viewer/re_space_view_dataframe/src/query_kind_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ impl UiQueryKind {
*time
} else {
TimeInt::MAX
}
.into();
};

changed |= match time_type {
TimeType::Time => time_drag_value
Expand All @@ -63,7 +62,7 @@ impl UiQueryKind {
};

if changed {
*self = Self::LatestAt { time: time.into() };
*self = Self::LatestAt { time };
}
}
});
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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 {
Expand Down
36 changes: 26 additions & 10 deletions crates/viewer/re_viewer_context/src/time_drag_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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)
}
Expand Down

0 comments on commit f532950

Please sign in to comment.