From 0fc3aedd4b33949129ed8e5ab1199e61d820a927 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 11:23:06 +0100 Subject: [PATCH 01/13] Minor clean-up --- crates/viewer/re_time_panel/src/data_density_graph.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/viewer/re_time_panel/src/data_density_graph.rs b/crates/viewer/re_time_panel/src/data_density_graph.rs index ccdcfc9ebd35..6c8f539665bd 100644 --- a/crates/viewer/re_time_panel/src/data_density_graph.rs +++ b/crates/viewer/re_time_panel/src/data_density_graph.rs @@ -3,7 +3,6 @@ //! The data density is the number of data points per unit of time. //! We collect this into a histogram, blur it, and then paint it. -use std::ops::RangeInclusive; use std::sync::Arc; use egui::emath::Rangef; @@ -211,7 +210,6 @@ impl DensityGraph { y_range: Rangef, painter: &egui::Painter, full_color: Color32, - hovered_x_range: RangeInclusive, ) { re_tracing::profile_function!(); @@ -270,11 +268,8 @@ impl DensityGraph { let inner_radius = (max_radius * normalized_density).at_least(MIN_RADIUS) - feather_radius; - let inner_color = if hovered_x_range.contains(&x) { - Color32::WHITE - } else { - full_color.gamma_multiply(lerp(0.5..=1.0, normalized_density)) - }; + let inner_color = full_color.gamma_multiply(lerp(0.5..=1.0, normalized_density)); + (inner_radius, inner_color) }; let outer_radius = inner_radius + feather_radius; @@ -417,8 +412,6 @@ pub fn data_density_graph_ui( row_rect.y_range(), time_area_painter, graph_color(ctx, &item.to_item(), ui), - // TODO(jprochazk): completely remove `hovered_x_range` and associated code from painter - 0f32..=0f32, ); if tooltips_enabled { From 92f577ccbe2522665112c522c7d3119b4ffbcc70 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 15:48:06 +0100 Subject: [PATCH 02/13] Update existing tests with new `TestContext` api --- .../re_time_panel/tests/time_panel_tests.rs | 175 ++++++++---------- 1 file changed, 81 insertions(+), 94 deletions(-) diff --git a/crates/viewer/re_time_panel/tests/time_panel_tests.rs b/crates/viewer/re_time_panel/tests/time_panel_tests.rs index 9bf24f3863b8..055b823dc45d 100644 --- a/crates/viewer/re_time_panel/tests/time_panel_tests.rs +++ b/crates/viewer/re_time_panel/tests/time_panel_tests.rs @@ -1,11 +1,9 @@ -use std::sync::Arc; - use egui::Vec2; -use re_chunk_store::{Chunk, LatestAtQuery, RowId}; +use re_chunk_store::{LatestAtQuery, RowId}; +use re_log_types::build_frame_nr; use re_log_types::example_components::MyPoint; use re_log_types::external::re_types_core::Component; -use re_log_types::{build_frame_nr, EntityPath}; use re_time_panel::TimePanel; use re_viewer_context::blueprint_timeline; use re_viewer_context::test_context::TestContext; @@ -18,19 +16,17 @@ pub fn time_panel_two_sections_should_match_snapshot() { let points1 = MyPoint::from_iter(0..1); for i in 0..2 { - let entity_path = EntityPath::from(format!("/entity/{i}")); - let mut builder = Chunk::builder(entity_path.clone()); - for frame in [10, 11, 12, 15, 18, 100, 102, 104].map(|frame| frame + i) { - builder = builder.with_sparse_component_batches( - RowId::new(), - [build_frame_nr(frame)], - [(MyPoint::descriptor(), Some(&points1 as _))], - ); - } - test_context - .recording_store - .add_chunk(&Arc::new(builder.build().unwrap())) - .unwrap(); + test_context.log_entity(format!("/entity/{i}").into(), |mut builder| { + for frame in [10, 11, 12, 15, 18, 100, 102, 104].map(|frame| frame + i) { + builder = builder.with_sparse_component_batches( + RowId::new(), + [build_frame_nr(frame)], + [(MyPoint::descriptor(), Some(&points1 as _))], + ); + } + + builder + }); } run_time_panel_and_save_snapshot( @@ -55,94 +51,30 @@ pub fn time_panel_dense_data_should_match_snapshot() { rng_seed.wrapping_mul(0x2545_f491_4f6c_dd1d) }; - let entity_path = EntityPath::from("/entity"); - let mut builder = Chunk::builder(entity_path.clone()); - for frame in 0..1_000 { - if rng() & 0b1 == 0 { - continue; + test_context.log_entity("/entity".into(), |mut builder| { + for frame in 0..1_000 { + if rng() & 0b1 == 0 { + continue; + } + + builder = builder.with_sparse_component_batches( + RowId::new(), + [build_frame_nr(frame)], + [(MyPoint::descriptor(), Some(&points1 as _))], + ); } - builder = builder.with_sparse_component_batches( - RowId::new(), - [build_frame_nr(frame)], - [(MyPoint::descriptor(), Some(&points1 as _))], - ); - } - test_context - .recording_store - .add_chunk(&Arc::new(builder.build().unwrap())) - .unwrap(); + builder + }); run_time_panel_and_save_snapshot(test_context, TimePanel::default(), "time_panel_dense_data"); } -#[test] -pub fn time_panel_filter_test_inactive_should_match_snapshot() { - run_time_panel_filter_tests(false, "", "time_panel_filter_test_inactive"); -} - -#[test] -pub fn time_panel_filter_test_active_no_query_should_match_snapshot() { - run_time_panel_filter_tests(true, "", "time_panel_filter_test_active_no_query"); -} - -#[test] -pub fn time_panel_filter_test_active_query_should_match_snapshot() { - run_time_panel_filter_tests(true, "ath", "time_panel_filter_test_active_query"); -} - -#[allow(clippy::unwrap_used)] -pub fn run_time_panel_filter_tests(filter_active: bool, query: &str, snapshot_name: &str) { - TimePanel::ensure_registered_subscribers(); - let mut test_context = TestContext::default(); - - let points1 = MyPoint::from_iter(0..1); - for i in 0..2 { - let entity_path = EntityPath::from(format!("/entity/{i}")); - let mut builder = Chunk::builder(entity_path.clone()); - - builder = builder.with_sparse_component_batches( - RowId::new(), - [build_frame_nr(1)], - [(MyPoint::descriptor(), Some(&points1 as _))], - ); - - test_context - .recording_store - .add_chunk(&Arc::new(builder.build().unwrap())) - .unwrap(); - } - - for i in 0..2 { - let entity_path = EntityPath::from(format!("/path/{i}")); - let mut builder = Chunk::builder(entity_path.clone()); - - builder = builder.with_sparse_component_batches( - RowId::new(), - [build_frame_nr(1)], - [(MyPoint::descriptor(), Some(&points1 as _))], - ); - - test_context - .recording_store - .add_chunk(&Arc::new(builder.build().unwrap())) - .unwrap(); - } - - let mut time_panel = TimePanel::default(); - if filter_active { - time_panel.activate_filter(query); - } - - run_time_panel_and_save_snapshot(test_context, time_panel, snapshot_name); -} - fn run_time_panel_and_save_snapshot( mut test_context: TestContext, mut time_panel: TimePanel, snapshot_name: &str, ) { - //TODO(ab): this contains a lot of boilerplate which should be provided by helpers let mut harness = test_context .setup_kittest_for_rendering() .with_size(Vec2::new(700.0, 300.0)) @@ -172,3 +104,58 @@ fn run_time_panel_and_save_snapshot( harness.run(); harness.snapshot(snapshot_name); } + +// --- + +#[test] +pub fn time_panel_filter_test_inactive_should_match_snapshot() { + run_time_panel_filter_tests(false, "", "time_panel_filter_test_inactive"); +} + +#[test] +pub fn time_panel_filter_test_active_no_query_should_match_snapshot() { + run_time_panel_filter_tests(true, "", "time_panel_filter_test_active_no_query"); +} + +#[test] +pub fn time_panel_filter_test_active_query_should_match_snapshot() { + run_time_panel_filter_tests(true, "ath", "time_panel_filter_test_active_query"); +} + +#[allow(clippy::unwrap_used)] +pub fn run_time_panel_filter_tests(filter_active: bool, query: &str, snapshot_name: &str) { + TimePanel::ensure_registered_subscribers(); + let mut test_context = TestContext::default(); + + let points1 = MyPoint::from_iter(0..1); + for i in 0..2 { + test_context.log_entity(format!("/entity/{i}").into(), |mut builder| { + builder = builder.with_sparse_component_batches( + RowId::new(), + [build_frame_nr(1)], + [(MyPoint::descriptor(), Some(&points1 as _))], + ); + + builder + }); + } + + for i in 0..2 { + test_context.log_entity(format!("/path/{i}").into(), |mut builder| { + builder = builder.with_sparse_component_batches( + RowId::new(), + [build_frame_nr(1)], + [(MyPoint::descriptor(), Some(&points1 as _))], + ); + + builder + }); + } + + let mut time_panel = TimePanel::default(); + if filter_active { + time_panel.activate_filter(query); + } + + run_time_panel_and_save_snapshot(test_context, time_panel, snapshot_name); +} From d27f110a65af94e6a5f7991f148a3b399e649a5d Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 17:10:52 +0100 Subject: [PATCH 03/13] Add a new test suite to cover various configuration of entity/timeline/time point --- .../various_entity_kinds_timeline_a_0.png | 3 + .../various_entity_kinds_timeline_a_5.png | 3 + ...y_kinds_timeline_a_9223372036854775807.png | 3 + .../various_entity_kinds_timeline_b_0.png | 3 + .../various_entity_kinds_timeline_b_5.png | 3 + ...y_kinds_timeline_b_9223372036854775807.png | 3 + .../re_time_panel/tests/time_panel_tests.rs | 160 +++++++++++++++++- 7 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_0.png create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_5.png create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_9223372036854775807.png create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_0.png create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_5.png create mode 100644 crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_9223372036854775807.png diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_0.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_0.png new file mode 100644 index 000000000000..44ea90e980fb --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_0.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:6f25d6bba58b9aeab6bc25d55ff2d7d5d7a72892c038257624650eabba7d774a +size 144221 diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_5.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_5.png new file mode 100644 index 000000000000..42859a51b67e --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_5.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:e4398719c591105cdbcd9eb904acdc487e2630a6b19a7ecf522a681c1f555469 +size 144429 diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_9223372036854775807.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_9223372036854775807.png new file mode 100644 index 000000000000..560fd161b6f3 --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_a_9223372036854775807.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:20a045edfc9294e5ac7a483f7d47177c3721865906eb37a6aa1e2cdd4e668e1a +size 144551 diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_0.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_0.png new file mode 100644 index 000000000000..395baf84496b --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_0.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:eee17ecdc118609f31ff0285acdfa49061ccdf7dd403e14c364532750470f699 +size 142735 diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_5.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_5.png new file mode 100644 index 000000000000..f251e9372667 --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_5.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:ba32eb0898dfd66fbd3a287fd21405772258a705427ee7f4678b6adb1bbf6a7a +size 142918 diff --git a/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_9223372036854775807.png b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_9223372036854775807.png new file mode 100644 index 000000000000..16a34356368b --- /dev/null +++ b/crates/viewer/re_time_panel/tests/snapshots/various_entity_kinds_timeline_b_9223372036854775807.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:0f56e3ef65134aa7785ef9d88101bdf0e66f7258d86bfc80b764c47cb17e10a8 +size 143213 diff --git a/crates/viewer/re_time_panel/tests/time_panel_tests.rs b/crates/viewer/re_time_panel/tests/time_panel_tests.rs index 055b823dc45d..5a39d85fa6f5 100644 --- a/crates/viewer/re_time_panel/tests/time_panel_tests.rs +++ b/crates/viewer/re_time_panel/tests/time_panel_tests.rs @@ -1,12 +1,14 @@ use egui::Vec2; use re_chunk_store::{LatestAtQuery, RowId}; -use re_log_types::build_frame_nr; -use re_log_types::example_components::MyPoint; -use re_log_types::external::re_types_core::Component; +use re_entity_db::InstancePath; +use re_log_types::{ + build_frame_nr, example_components::MyPoint, external::re_types_core::Component, EntityPath, + TimeInt, TimePoint, TimeType, Timeline, +}; use re_time_panel::TimePanel; -use re_viewer_context::blueprint_timeline; -use re_viewer_context::test_context::TestContext; +use re_types::archetypes::Points2D; +use re_viewer_context::{blueprint_timeline, test_context::TestContext, CollapseScope, TimeView}; use re_viewport_blueprint::ViewportBlueprint; #[test] @@ -159,3 +161,151 @@ pub fn run_time_panel_filter_tests(filter_active: bool, query: &str, snapshot_na run_time_panel_and_save_snapshot(test_context, time_panel, snapshot_name); } + +// -- + +/// This test focuses on various kinds of entities and ensures their representation in the tree is +/// correct regardless of the selected timeline and current time. +//TODO(ab): we should also test what happens when GC kicks in. +#[test] +pub fn test_various_entity_kinds_in_time_panel() { + TimePanel::ensure_registered_subscribers(); + + for timeline in ["timeline_a", "timeline_b"] { + for time in [0, 5, i64::MAX] { + let mut test_context = TestContext::default(); + + log_all_data(&mut test_context); + + test_context + .recording_config + .time_ctrl + .write() + .set_timeline_and_time(Timeline::new(timeline, TimeType::Sequence), time); + + test_context + .recording_config + .time_ctrl + .write() + .set_time_view(TimeView { + min: 0.into(), + time_spanned: 10.0, + }); + + let time_panel = TimePanel::default(); + + run_expanded_time_panel_and_save_snapshot( + test_context, + time_panel, + &format!("various_entity_kinds_{timeline}_{time}"), + ); + } + } +} + +pub fn log_all_data(test_context: &mut TestContext) { + let timeline_a = "timeline_a"; + let timeline_b = "timeline_b"; + + // just your average static entity + log_static_data(test_context, "static_entity"); + + // static data is over-logged multiple times + for _ in 0..3 { + log_static_data(test_context, "static_entity_multiple"); + } + + // static data overrides data logged on timeline a + log_data(test_context, "static_overrides_temporal", timeline_a, 3); + log_static_data(test_context, "static_overrides_temporal"); + + // data in single timeline + log_data(test_context, "timeline_a_only", timeline_a, 3); + log_data(test_context, "timeline_b_only", timeline_b, 3); + + // data in both timelines + log_data(test_context, "timeline_a_and_b", timeline_a, 2); + log_data(test_context, "timeline_a_and_b", timeline_b, 5); + + // nested entity with parent empty + log_data(test_context, "/empty/parent/of/entity", timeline_a, 3); + + // nested entity with data in a parent + log_data(test_context, "/parent_with_data/of/entity", timeline_a, 3); + log_data(test_context, "/parent_with_data", timeline_a, 1); + + // some entity with data logged "late" on the timeline + log_data(test_context, "/late_data", timeline_a, 9); + log_data(test_context, "/late_data", timeline_a, 10); +} + +pub fn log_data( + test_context: &mut TestContext, + entity_path: impl Into, + timeline: &str, + time: i64, +) { + test_context.log_entity(entity_path.into(), |builder| { + builder.with_archetype( + RowId::new(), + [( + Timeline::new(timeline, TimeType::Sequence), + TimeInt::try_from(time).expect("time must be valid"), + )], + &Points2D::new([[0.0, 0.0]]), + ) + }); +} + +pub fn log_static_data(test_context: &mut TestContext, entity_path: impl Into) { + test_context.log_entity(entity_path.into(), |builder| { + builder.with_archetype( + RowId::new(), + TimePoint::from(std::collections::BTreeMap::default()), + &Points2D::new([[0.0, 0.0]]), + ) + }); +} + +fn run_expanded_time_panel_and_save_snapshot( + mut test_context: TestContext, + mut time_panel: TimePanel, + snapshot_name: &str, +) { + let mut harness = test_context + .setup_kittest_for_rendering() + .with_size(Vec2::new(700.0, 1200.0)) + .build_ui(|ui| { + test_context.run(&ui.ctx().clone(), |viewer_ctx| { + re_context_menu::collapse_expand::collapse_expand_instance_path( + viewer_ctx, + viewer_ctx.recording(), + &InstancePath::entity_all("/".into()), + CollapseScope::StreamsTree, + true, + ); + + let blueprint = ViewportBlueprint::try_from_db( + viewer_ctx.store_context.blueprint, + &LatestAtQuery::latest(blueprint_timeline()), + ); + + let mut time_ctrl = viewer_ctx.rec_cfg.time_ctrl.read().clone(); + + time_panel.show_expanded_with_header( + viewer_ctx, + &blueprint, + viewer_ctx.recording(), + &mut time_ctrl, + ui, + ); + + *viewer_ctx.rec_cfg.time_ctrl.write() = time_ctrl; + }); + + test_context.handle_system_commands(); + }); + + harness.run(); + harness.snapshot(snapshot_name); +} From 9020b4b2ee9628f41de5f45177190240a0d6aaf4 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 11:49:44 +0100 Subject: [PATCH 04/13] Introduce streams tree data structures data update update data --- Cargo.lock | 1 + crates/viewer/re_time_panel/Cargo.toml | 1 + crates/viewer/re_time_panel/src/lib.rs | 11 +- .../re_time_panel/src/streams_tree_data.rs | 152 ++++++++++++++++++ 4 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 crates/viewer/re_time_panel/src/streams_tree_data.rs diff --git a/Cargo.lock b/Cargo.lock index 68944fa4819d..581a3f74b78e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6570,6 +6570,7 @@ dependencies = [ "re_viewer_context", "re_viewport_blueprint", "serde", + "smallvec", "vec1", ] diff --git a/crates/viewer/re_time_panel/Cargo.toml b/crates/viewer/re_time_panel/Cargo.toml index 626c8fa8b575..efa8d2341ac0 100644 --- a/crates/viewer/re_time_panel/Cargo.toml +++ b/crates/viewer/re_time_panel/Cargo.toml @@ -38,6 +38,7 @@ itertools.workspace = true nohash-hasher.workspace = true once_cell.workspace = true serde.workspace = true +smallvec.workspace = true vec1.workspace = true [dev-dependencies] diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index 69a110f676ad..c3c0a997a5a3 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -9,6 +9,7 @@ mod data_density_graph; mod paint_ticks; mod recursive_chunks_per_timeline_subscriber; +mod streams_tree_data; mod time_axis; mod time_control_ui; mod time_ranges_ui; @@ -88,7 +89,7 @@ impl TimePanelItem { } } -#[derive(Clone, Copy, Default, PartialEq, Eq, serde::Deserialize, serde::Serialize)] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Deserialize, serde::Serialize)] enum TimePanelSource { #[default] Recording, @@ -607,6 +608,14 @@ impl TimePanel { ) { re_tracing::profile_function!(); + let entity_tree_data = crate::streams_tree_data::StreamsTreeData::from_source_and_filter( + ctx, + self.source, + &self.filter_state.filter(), + ); + + dbg!(entity_tree_data); + egui::ScrollArea::vertical() .auto_shrink([false; 2]) // We turn off `drag_to_scroll` so that the `ScrollArea` don't steal input from diff --git a/crates/viewer/re_time_panel/src/streams_tree_data.rs b/crates/viewer/re_time_panel/src/streams_tree_data.rs new file mode 100644 index 000000000000..596750fcf7f7 --- /dev/null +++ b/crates/viewer/re_time_panel/src/streams_tree_data.rs @@ -0,0 +1,152 @@ +use std::ops::Range; + +use itertools::Itertools as _; +use smallvec::SmallVec; + +use re_entity_db::EntityTree; +use re_log_types::EntityPath; +use re_ui::filter_widget::FilterMatcher; +use re_viewer_context::ViewerContext; + +use crate::TimePanelSource; + +#[derive(Debug)] +pub struct StreamsTreeData { + pub children: Vec, +} + +impl StreamsTreeData { + pub fn from_source_and_filter( + ctx: &ViewerContext<'_>, + source: TimePanelSource, + filter_matcher: &FilterMatcher, + ) -> Self { + re_tracing::profile_function!(); + + let db = match source { + TimePanelSource::Recording => ctx.recording(), + TimePanelSource::Blueprint => ctx.blueprint_db(), + }; + + let root_data = EntityData::from_entity_tree_and_filter(db.tree(), filter_matcher, false); + + // We show "/" on top only for recording streams, because the `/` entity in blueprint + // is always empty, so it's just lost space. This works around an issue where the + // selection/hover state of the `/` entity is wrongly synchronized between both + // stores, due to `Item::*` not tracking stores for entity paths. + + Self { + children: match source { + TimePanelSource::Recording => root_data + .map(|entity_part_data| vec![entity_part_data]) + .unwrap_or_default(), + TimePanelSource::Blueprint => root_data + .map(|entity_part_data| entity_part_data.children) + .unwrap_or_default(), + }, + } + } +} + +// --- + +#[derive(Debug)] +pub struct EntityData { + pub entity_path: EntityPath, + + pub label: String, + pub highlight_sections: SmallVec<[Range; 1]>, + + pub default_open: bool, + + pub children: Vec, +} + +impl EntityData { + pub fn from_entity_tree_and_filter( + entity_tree: &EntityTree, + filter_matcher: &FilterMatcher, + mut is_already_a_match: bool, + ) -> Option { + // Early out + if filter_matcher.matches_nothing() { + return None; + } + + let mut label = entity_tree + .path + .last() + .map(|entity_part| entity_part.ui_string()) + .unwrap_or("/".to_owned()); + + // + // Filtering + // + + let (entity_part_matches, highlight_sections) = if filter_matcher.matches_everything() { + // fast path (filter is inactive) + (true, SmallVec::new()) + } else if let Some(entity_part) = entity_tree.path.last() { + // Nominal case of matching the hierarchy. + if let Some(match_sections) = filter_matcher.find_matches(&entity_part.ui_string()) { + (true, match_sections.collect()) + } else { + (false, SmallVec::new()) + } + } else { + // we are the root, it can never match anything + (false, SmallVec::new()) + }; + + // We want to keep entire branches if a single of its node matches. So we must propagate the + // "matched" state so we can make the right call when we reach leaf nodes. + is_already_a_match |= entity_part_matches; + + // + // Recurse + // + + if entity_tree.children.is_empty() { + // Discard a leaf item unless it is already a match. + is_already_a_match.then(|| { + // Leaf items are always collapsed by default, even when the filter is active. + let default_open = false; + + Self { + entity_path: entity_tree.path.clone(), + label, + highlight_sections, + default_open, + children: vec![], + } + }) + } else { + let children = entity_tree + .children + .values() + .filter_map(|sub_tree| { + Self::from_entity_tree_and_filter(sub_tree, filter_matcher, is_already_a_match) + }) + .collect_vec(); + + (is_already_a_match || !children.is_empty()).then(|| { + // Only top-level non-leaf entities are expanded by default, unless the filter is + // active. + let default_open = filter_matcher.is_active() || entity_tree.path.len() <= 1; + Self { + entity_path: entity_tree.path.clone(), + label: if children.is_empty() || entity_tree.path.is_root() { + label + } else { + // Indicate that we have children + label.push('/'); + label + }, + highlight_sections, + default_open, + children, + } + }) + } + } +} From cfd6e537cc254181ebba6351d4a75737351157fa Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 10:22:15 +0100 Subject: [PATCH 05/13] Update UI code to use `StreamsTreeData` --- crates/viewer/re_time_panel/src/lib.rs | 178 ++++++------------- crates/viewer/re_time_panel/src/time_axis.rs | 3 + 2 files changed, 55 insertions(+), 126 deletions(-) diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index c3c0a997a5a3..9d0a4e31a079 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -3,9 +3,6 @@ //! This crate provides a panel that shows all entities in the store and allows control of time and //! timelines, as well as all necessary ui elements that make it up. -// TODO(#6330): remove unwrap() -#![expect(clippy::unwrap_used)] - mod data_density_graph; mod paint_ticks; mod recursive_chunks_per_timeline_subscriber; @@ -25,12 +22,13 @@ use re_context_menu::{ }; use re_data_ui::DataUi as _; use re_data_ui::{item_ui::guess_instance_path_icon, sorted_component_list_for_ui}; -use re_entity_db::{EntityDb, EntityTree, InstancePath}; +use re_entity_db::{EntityDb, InstancePath}; use re_log_types::{ external::re_types_core::ComponentName, ApplicationId, ComponentPath, EntityPath, ResolvedTimeRange, TimeInt, TimeReal, TimeType, }; use re_types::blueprint::components::PanelState; +use re_ui::filter_widget::format_matching_text; use re_ui::{filter_widget, list_item, ContextExt as _, DesignTokens, UiExt as _}; use re_viewer_context::{ CollapseScope, HoverHighlight, Item, ItemContext, RecordingConfig, TimeControl, TimeView, @@ -38,6 +36,7 @@ use re_viewer_context::{ }; use re_viewport_blueprint::ViewportBlueprint; +use crate::streams_tree_data::EntityData; use recursive_chunks_per_timeline_subscriber::PathRecursiveChunksPerTimelineStoreSubscriber; use time_axis::TimelineAxis; use time_control_ui::TimeControlUi; @@ -608,14 +607,6 @@ impl TimePanel { ) { re_tracing::profile_function!(); - let entity_tree_data = crate::streams_tree_data::StreamsTreeData::from_source_and_filter( - ctx, - self.source, - &self.filter_state.filter(), - ); - - dbg!(entity_tree_data); - egui::ScrollArea::vertical() .auto_shrink([false; 2]) // We turn off `drag_to_scroll` so that the `ScrollArea` don't steal input from @@ -629,44 +620,33 @@ impl TimePanel { ui.scroll_with_delta(Vec2::Y * time_area_response.drag_delta().y); } - // Show "/" on top only for recording streams, because the `/` entity in blueprint - // is always empty, so it's just lost space. This works around an issue where the - // selection/hover state of the `/` entity is wrongly synchronized between both - // stores, due to `Item::*` not tracking stores for entity paths. - let show_root = self.source == TimePanelSource::Recording; - let filter_matcher = self.filter_state.filter(); - if show_root { - self.show_tree( + let entity_tree_data = + crate::streams_tree_data::StreamsTreeData::from_source_and_filter( ctx, - viewport_blueprint, - entity_db, - time_ctrl, - time_area_response, - time_area_painter, - entity_db.tree(), + self.source, &filter_matcher, - ui, ); - } else { - self.show_children( + + for child in &entity_tree_data.children { + self.show_entity( ctx, viewport_blueprint, entity_db, time_ctrl, time_area_response, time_area_painter, - entity_db.tree(), - &filter_matcher, + child, ui, ); } }); } + /// Display the list item for an entity. #[expect(clippy::too_many_arguments)] - fn show_tree( + fn show_entity( &mut self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint, @@ -674,68 +654,11 @@ impl TimePanel { time_ctrl: &mut TimeControl, time_area_response: &egui::Response, time_area_painter: &egui::Painter, - tree: &EntityTree, - filter_matcher: &filter_widget::FilterMatcher, + entity_data: &EntityData, ui: &mut egui::Ui, ) { - // We traverse and display this tree only if it contains a matching entity part. - 'early_exit: { - if filter_matcher.matches_nothing() { - return; - } - - // Filter is inactive or otherwise whitelisting everything. - if filter_matcher.matches_everything() { - break 'early_exit; - } - - // The current path is a match. - if tree - .path - .iter() - .any(|entity_part| filter_matcher.matches(&entity_part.ui_string())) - { - break 'early_exit; - } - - // Our subtree contains a match. - if tree - .find_first_child_recursive(|entity_path| { - entity_path - .last() - .is_some_and(|entity_part| filter_matcher.matches(&entity_part.ui_string())) - }) - .is_some() - { - break 'early_exit; - } - - // No match to be found, so nothing to display. - return; - } - - let db = match self.source { - TimePanelSource::Recording => ctx.recording(), - TimePanelSource::Blueprint => ctx.store_context.blueprint, - }; - - // The last part of the path component - let last_path_part = tree.path.last(); - let text = if let Some(last_path_part) = last_path_part { - let part_text = if tree.is_leaf() { - last_path_part.ui_string() - } else { - format!("{}/", last_path_part.ui_string()) // show we have children with a / - }; - - filter_matcher - .matches_formatted(ui.ctx(), &part_text) - .unwrap_or_else(|| part_text.into()) - } else { - "/".into() - }; - - let item = TimePanelItem::entity_path(tree.path.clone()); + let entity_path = &entity_data.entity_path; + let item = TimePanelItem::entity_path(entity_path.clone()); let is_selected = ctx.selection().contains_item(&item.to_item()); let is_item_hovered = ctx .selection_state() @@ -744,24 +667,22 @@ impl TimePanel { let collapse_scope = self.collapse_scope(); - // expand if children is focused + // Expand if one of the children is focused let focused_entity_path = ctx .focused_item .as_ref() .and_then(|item| item.entity_path()); - if focused_entity_path.is_some_and(|entity_path| entity_path.is_descendant_of(&tree.path)) { + if focused_entity_path.is_some_and(|entity_path| entity_path.is_descendant_of(entity_path)) + { collapse_scope - .entity(tree.path.clone()) + .entity(entity_path.clone()) .set_open(ui.ctx(), true); } // Globally unique id that is dependent on the "nature" of the tree (recording or blueprint, // in a filter session or not) - let id = collapse_scope.entity(tree.path.clone()).into(); - - let is_short_path = tree.path.len() <= 1 && !tree.is_leaf(); - let default_open = self.filter_state.is_active() || is_short_path; + let id = collapse_scope.entity(entity_path.clone()).into(); let list_item::ShowCollapsingResponse { item_response: response, @@ -776,23 +697,28 @@ impl TimePanel { .show_hierarchical_with_children( ui, id, - default_open, - list_item::LabelContent::new(text) - .with_icon(guess_instance_path_icon( - ctx, - &InstancePath::from(tree.path.clone()), - )) - .truncate(false), + entity_data.default_open, + list_item::LabelContent::new(format_matching_text( + ctx.egui_ctx, + &entity_data.label, + entity_data.highlight_sections.iter().cloned(), + None, + )) + //TODO: this is slow + .with_icon(guess_instance_path_icon( + ctx, + &InstancePath::from(entity_path.clone()), + )) + .truncate(false), |ui| { - self.show_children( + self.show_entity_contents( ctx, viewport_blueprint, entity_db, time_ctrl, time_area_response, time_area_painter, - tree, - filter_matcher, + entity_data, ui, ); }, @@ -804,13 +730,13 @@ impl TimePanel { ui, ctx, &time_ctrl.current_query(), - db, - &tree.path, + entity_db, + entity_path, include_subtree, ); }); - if Some(&tree.path) == focused_entity_path { + if Some(entity_path) == focused_entity_path { // Scroll only if the entity isn't already visible. This is important because that's what // happens when double-clicking an entity _in the blueprint tree_. In such case, it would be // annoying to induce a scroll motion. @@ -851,7 +777,7 @@ impl TimePanel { let tree_has_data_in_current_timeline = entity_db.subtree_has_data_on_timeline( &entity_db.storage_engine(), time_ctrl.timeline(), - &tree.path, + entity_path, ); if is_visible && tree_has_data_in_current_timeline { let row_rect = @@ -865,7 +791,7 @@ impl TimePanel { &mut self.data_density_graph_painter, ctx, time_ctrl, - db, + entity_db, time_area_painter, ui, &self.time_ranges_ui, @@ -877,8 +803,9 @@ impl TimePanel { } } + /// Display the contents of an entity, i.e. its sub-entities and its components. #[expect(clippy::too_many_arguments)] - fn show_children( + fn show_entity_contents( &mut self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint, @@ -886,12 +813,11 @@ impl TimePanel { time_ctrl: &mut TimeControl, time_area_response: &egui::Response, time_area_painter: &egui::Painter, - tree: &EntityTree, - filter_matcher: &filter_widget::FilterMatcher, + entity_data: &EntityData, ui: &mut egui::Ui, ) { - for child in tree.children.values() { - self.show_tree( + for child in &entity_data.children { + self.show_entity( ctx, viewport_blueprint, entity_db, @@ -899,20 +825,20 @@ impl TimePanel { time_area_response, time_area_painter, child, - filter_matcher, ui, ); } + let entity_path = &entity_data.entity_path; let engine = entity_db.storage_engine(); let store = engine.store(); // If this is an entity: - if let Some(components) = store.all_components_for_entity(&tree.path) { + if let Some(components) = store.all_components_for_entity(entity_path) { for component_name in sorted_component_list_for_ui(components.iter()) { - let is_static = store.entity_has_static_component(&tree.path, &component_name); + let is_static = store.entity_has_static_component(entity_path, &component_name); - let component_path = ComponentPath::new(tree.path.clone(), component_name); + let component_path = ComponentPath::new(entity_path.clone(), component_name); let short_component_name = component_path.component_name.short_name(); let item = TimePanelItem::component_path(component_path.clone()); let timeline = time_ctrl.timeline(); @@ -920,15 +846,15 @@ impl TimePanel { let component_has_data_in_current_timeline = store .entity_has_component_on_timeline( time_ctrl.timeline(), - &tree.path, + entity_path, &component_name, ); let num_static_messages = - store.num_static_events_for_component(&tree.path, component_name); + store.num_static_events_for_component(entity_path, component_name); let num_temporal_messages = store.num_temporal_events_for_component_on_timeline( time_ctrl.timeline(), - &tree.path, + entity_path, component_name, ); let total_num_messages = num_static_messages + num_temporal_messages; diff --git a/crates/viewer/re_time_panel/src/time_axis.rs b/crates/viewer/re_time_panel/src/time_axis.rs index fe7b0252203a..41aeedbfc666 100644 --- a/crates/viewer/re_time_panel/src/time_axis.rs +++ b/crates/viewer/re_time_panel/src/time_axis.rs @@ -1,3 +1,6 @@ +// TODO(#6330): remove unwrap() +#![expect(clippy::unwrap_used)] + use re_entity_db::TimeHistogram; use re_log_types::{ResolvedTimeRange, TimeInt, TimeType}; From ed17619eb9bf8f30153ea9045444dd6bff396a55 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 15:57:09 +0100 Subject: [PATCH 06/13] Update test snapshot based on (desired) UI change --- .../tests/snapshots/time_panel_filter_test_active_query.png | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_time_panel/tests/snapshots/time_panel_filter_test_active_query.png b/crates/viewer/re_time_panel/tests/snapshots/time_panel_filter_test_active_query.png index 8c19583c7280..7a1ce0daf8ca 100644 --- a/crates/viewer/re_time_panel/tests/snapshots/time_panel_filter_test_active_query.png +++ b/crates/viewer/re_time_panel/tests/snapshots/time_panel_filter_test_active_query.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:17c40bafbc465fe68335737b608569126c437c29319b42d49e7b38e87f027ee1 -size 27073 +oid sha256:7de7ddd6c3b874dd17ae124dd89b2ba396360fb8e9ad96102ec3571c56da158d +size 21810 From 2183bf53ac9a04d941f022cff485af068edf5373 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 16:21:13 +0100 Subject: [PATCH 07/13] Lint --- crates/viewer/re_time_panel/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index 9d0a4e31a079..3eb85b47d934 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -704,7 +704,6 @@ impl TimePanel { entity_data.highlight_sections.iter().cloned(), None, )) - //TODO: this is slow .with_icon(guess_instance_path_icon( ctx, &InstancePath::from(entity_path.clone()), From 1cfc12d10cec98b8c5c1935c166218a2bf11fdcf Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 11:51:50 +0100 Subject: [PATCH 08/13] Use faster method to determine if an entity has components --- crates/viewer/re_data_ui/src/item_ui.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/viewer/re_data_ui/src/item_ui.rs b/crates/viewer/re_data_ui/src/item_ui.rs index 1005e60fb5ae..3654b95578c1 100644 --- a/crates/viewer/re_data_ui/src/item_ui.rs +++ b/crates/viewer/re_data_ui/src/item_ui.rs @@ -185,8 +185,7 @@ pub fn instance_path_icon( if db .storage_engine() .store() - .all_components_on_timeline(timeline, &instance_path.entity_path) - .is_some() + .entity_has_data_on_timeline(timeline, &instance_path.entity_path) { &icons::ENTITY } else { From f65f3f5ea01eff87a5a18749f838ec11fa581a03 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Mon, 27 Jan 2025 11:54:19 +0100 Subject: [PATCH 09/13] Do not unnecessarily query for event data if those are not drawn cleanup --- crates/viewer/re_time_panel/src/lib.rs | 58 ++++++++++++++------------ 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index 3eb85b47d934..1df721cbb0c3 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -762,6 +762,10 @@ impl TimePanel { let response_rect = response.rect; self.next_col_right = self.next_col_right.max(response_rect.right()); + // + // Display the data density graph only if it is visible. + // + // From the left of the label, all the way to the right-most of the time panel let full_width_rect = Rect::from_x_y_ranges( response_rect.left()..=ui.max_rect().right(), @@ -769,35 +773,35 @@ impl TimePanel { ); let is_visible = ui.is_rect_visible(full_width_rect); + if is_visible { + let tree_has_data_in_current_timeline = entity_db.subtree_has_data_on_timeline( + &entity_db.storage_engine(), + time_ctrl.timeline(), + entity_path, + ); + if tree_has_data_in_current_timeline { + let row_rect = Rect::from_x_y_ranges( + time_area_response.rect.x_range(), + response_rect.y_range(), + ); - // ---------------------------------------------- - - // show the data in the time area: - let tree_has_data_in_current_timeline = entity_db.subtree_has_data_on_timeline( - &entity_db.storage_engine(), - time_ctrl.timeline(), - entity_path, - ); - if is_visible && tree_has_data_in_current_timeline { - let row_rect = - Rect::from_x_y_ranges(time_area_response.rect.x_range(), response_rect.y_range()); - - highlight_timeline_row(ui, ctx, time_area_painter, &item.to_item(), &row_rect); + highlight_timeline_row(ui, ctx, time_area_painter, &item.to_item(), &row_rect); - // show the density graph only if that item is closed - if is_closed { - data_density_graph::data_density_graph_ui( - &mut self.data_density_graph_painter, - ctx, - time_ctrl, - entity_db, - time_area_painter, - ui, - &self.time_ranges_ui, - row_rect, - &item, - true, - ); + // show the density graph only if that item is closed + if is_closed { + data_density_graph::data_density_graph_ui( + &mut self.data_density_graph_painter, + ctx, + time_ctrl, + entity_db, + time_area_painter, + ui, + &self.time_ranges_ui, + row_rect, + &item, + true, + ); + } } } } From 29bc04180f8f0c0627154519265fa3f9ad142b13 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 16:58:18 +0100 Subject: [PATCH 10/13] Remove profile scope from `handle_select_hover_drag_interactions` as it can be called many times in some benchmarks. Note: there is one code path that may result in this function to be different from essentially a no-op. It goes through `resolve_mono_instance_path`, which does have a scope to it. It's less than 1ms in my worst case benchmark- --- crates/viewer/re_viewer_context/src/viewer_context.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/viewer_context.rs b/crates/viewer/re_viewer_context/src/viewer_context.rs index 8d92d94f09cf..4a65f1c152a7 100644 --- a/crates/viewer/re_viewer_context/src/viewer_context.rs +++ b/crates/viewer/re_viewer_context/src/viewer_context.rs @@ -162,8 +162,6 @@ impl ViewerContext<'_> { interacted_items: impl Into, draggable: bool, ) { - re_tracing::profile_function!(); - let interacted_items = interacted_items.into().into_mono_instance_path_items(self); let selection_state = self.selection_state(); From 230a7dcfbef73814486f512db55d673b51eb066c Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 16:58:48 +0100 Subject: [PATCH 11/13] Move event count logic to inside the tooltip closure where it's needed --- crates/viewer/re_time_panel/src/lib.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index 1df721cbb0c3..e2a3277eb76e 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -853,15 +853,6 @@ impl TimePanel { &component_name, ); - let num_static_messages = - store.num_static_events_for_component(entity_path, component_name); - let num_temporal_messages = store.num_temporal_events_for_component_on_timeline( - time_ctrl.timeline(), - entity_path, - component_name, - ); - let total_num_messages = num_static_messages + num_temporal_messages; - let response = ui .list_item() .render_offscreen(false) @@ -894,6 +885,16 @@ impl TimePanel { let response_rect = response.rect; response.on_hover_ui(|ui| { + let num_static_messages = + store.num_static_events_for_component(entity_path, component_name); + let num_temporal_messages = store + .num_temporal_events_for_component_on_timeline( + time_ctrl.timeline(), + entity_path, + component_name, + ); + let total_num_messages = num_static_messages + num_temporal_messages; + if total_num_messages == 0 { ui.label(ui.ctx().warning_text(format!( "No event logged on timeline {:?}", From 4d7b3d12a7383d03a3fb51ed9d9f42efbb127064 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 28 Jan 2025 17:29:59 +0100 Subject: [PATCH 12/13] Do not query for event unnecessarily (again) --- crates/viewer/re_time_panel/src/lib.rs | 73 +++++++++++++++----------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/crates/viewer/re_time_panel/src/lib.rs b/crates/viewer/re_time_panel/src/lib.rs index e2a3277eb76e..9227bce26426 100644 --- a/crates/viewer/re_time_panel/src/lib.rs +++ b/crates/viewer/re_time_panel/src/lib.rs @@ -657,6 +657,8 @@ impl TimePanel { entity_data: &EntityData, ui: &mut egui::Ui, ) { + re_tracing::profile_function!(); + let entity_path = &entity_data.entity_path; let item = TimePanelItem::entity_path(entity_path.clone()); let is_selected = ctx.selection().contains_item(&item.to_item()); @@ -819,6 +821,8 @@ impl TimePanel { entity_data: &EntityData, ui: &mut egui::Ui, ) { + re_tracing::profile_function!(); + for child in &entity_data.children { self.show_entity( ctx, @@ -846,13 +850,6 @@ impl TimePanel { let item = TimePanelItem::component_path(component_path.clone()); let timeline = time_ctrl.timeline(); - let component_has_data_in_current_timeline = store - .entity_has_component_on_timeline( - time_ctrl.timeline(), - entity_path, - &component_name, - ); - let response = ui .list_item() .render_offscreen(false) @@ -957,32 +954,48 @@ impl TimePanel { ); let is_visible = ui.is_rect_visible(full_width_rect); - if is_visible && component_has_data_in_current_timeline { - // show the data in the time area: - let row_rect = Rect::from_x_y_ranges( - time_area_response.rect.x_range(), - response_rect.y_range(), - ); - highlight_timeline_row(ui, ctx, time_area_painter, &item.to_item(), &row_rect); + if is_visible { + let component_has_data_in_current_timeline = store + .entity_has_component_on_timeline( + time_ctrl.timeline(), + entity_path, + &component_name, + ); - let db = match self.source { - TimePanelSource::Recording => ctx.recording(), - TimePanelSource::Blueprint => ctx.store_context.blueprint, - }; + if component_has_data_in_current_timeline { + // show the data in the time area: + let row_rect = Rect::from_x_y_ranges( + time_area_response.rect.x_range(), + response_rect.y_range(), + ); - data_density_graph::data_density_graph_ui( - &mut self.data_density_graph_painter, - ctx, - time_ctrl, - db, - time_area_painter, - ui, - &self.time_ranges_ui, - row_rect, - &item, - true, - ); + highlight_timeline_row( + ui, + ctx, + time_area_painter, + &item.to_item(), + &row_rect, + ); + + let db = match self.source { + TimePanelSource::Recording => ctx.recording(), + TimePanelSource::Blueprint => ctx.store_context.blueprint, + }; + + data_density_graph::data_density_graph_ui( + &mut self.data_density_graph_painter, + ctx, + time_ctrl, + db, + time_area_painter, + ui, + &self.time_ranges_ui, + row_rect, + &item, + true, + ); + } } } } From 0c1eb2c1fe2ebcf7ff3501227f6b8af3895fba6e Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Thu, 30 Jan 2025 11:30:32 +0100 Subject: [PATCH 13/13] Addressed review comments --- .../re_time_panel/tests/time_panel_tests.rs | 77 +++++++------------ 1 file changed, 28 insertions(+), 49 deletions(-) diff --git a/crates/viewer/re_time_panel/tests/time_panel_tests.rs b/crates/viewer/re_time_panel/tests/time_panel_tests.rs index 5a39d85fa6f5..25a5235965fa 100644 --- a/crates/viewer/re_time_panel/tests/time_panel_tests.rs +++ b/crates/viewer/re_time_panel/tests/time_panel_tests.rs @@ -34,6 +34,8 @@ pub fn time_panel_two_sections_should_match_snapshot() { run_time_panel_and_save_snapshot( test_context, TimePanel::default(), + 300.0, + false, "time_panel_two_sections", ); } @@ -69,42 +71,13 @@ pub fn time_panel_dense_data_should_match_snapshot() { builder }); - run_time_panel_and_save_snapshot(test_context, TimePanel::default(), "time_panel_dense_data"); -} - -fn run_time_panel_and_save_snapshot( - mut test_context: TestContext, - mut time_panel: TimePanel, - snapshot_name: &str, -) { - let mut harness = test_context - .setup_kittest_for_rendering() - .with_size(Vec2::new(700.0, 300.0)) - .build_ui(|ui| { - test_context.run(&ui.ctx().clone(), |viewer_ctx| { - let blueprint = ViewportBlueprint::try_from_db( - viewer_ctx.store_context.blueprint, - &LatestAtQuery::latest(blueprint_timeline()), - ); - - let mut time_ctrl = viewer_ctx.rec_cfg.time_ctrl.read().clone(); - - time_panel.show_expanded_with_header( - viewer_ctx, - &blueprint, - viewer_ctx.recording(), - &mut time_ctrl, - ui, - ); - - *viewer_ctx.rec_cfg.time_ctrl.write() = time_ctrl; - }); - - test_context.handle_system_commands(); - }); - - harness.run(); - harness.snapshot(snapshot_name); + run_time_panel_and_save_snapshot( + test_context, + TimePanel::default(), + 300.0, + false, + "time_panel_dense_data", + ); } // --- @@ -159,7 +132,7 @@ pub fn run_time_panel_filter_tests(filter_active: bool, query: &str, snapshot_na time_panel.activate_filter(query); } - run_time_panel_and_save_snapshot(test_context, time_panel, snapshot_name); + run_time_panel_and_save_snapshot(test_context, time_panel, 300.0, false, snapshot_name); } // -- @@ -175,7 +148,7 @@ pub fn test_various_entity_kinds_in_time_panel() { for time in [0, 5, i64::MAX] { let mut test_context = TestContext::default(); - log_all_data(&mut test_context); + log_data_for_various_entity_kinds_tests(&mut test_context); test_context .recording_config @@ -194,16 +167,18 @@ pub fn test_various_entity_kinds_in_time_panel() { let time_panel = TimePanel::default(); - run_expanded_time_panel_and_save_snapshot( + run_time_panel_and_save_snapshot( test_context, time_panel, + 1200.0, + true, &format!("various_entity_kinds_{timeline}_{time}"), ); } } } -pub fn log_all_data(test_context: &mut TestContext) { +pub fn log_data_for_various_entity_kinds_tests(test_context: &mut TestContext) { let timeline_a = "timeline_a"; let timeline_b = "timeline_b"; @@ -267,23 +242,27 @@ pub fn log_static_data(test_context: &mut TestContext, entity_path: impl Into