From f294dbee87071b50806488a2579a8797ae76e196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jochen=20G=C3=B6rtler?= Date: Tue, 7 Jan 2025 14:11:59 +0100 Subject: [PATCH] Use `egui_kittest` for the graph view (#8578) Co-authored-by: Andreas Reich Co-authored-by: Andreas Reich Co-authored-by: Andreas Reich --- Cargo.lock | 3 + crates/store/re_dataframe/src/query.rs | 11 +- .../re_log_types/src/time_point/timeline.rs | 15 -- .../re_view_dataframe/src/dataframe_ui.rs | 21 +- crates/viewer/re_view_graph/Cargo.toml | 5 + .../re_view_graph/src/layout/geometry.rs | 2 - .../viewer/re_view_graph/src/layout/result.rs | 1 - crates/viewer/re_view_graph/src/lib.rs | 1 + .../re_view_graph/src/visualizers/edges.rs | 4 +- crates/viewer/re_view_graph/tests/basic.rs | 254 ++++++++++++++++++ .../tests/snapshots/coincident_nodes.png | 3 + .../tests/snapshots/self_and_multi_edges.png | 3 + .../re_viewer/src/blueprint/validation.rs | 2 +- .../re_viewer_context/src/query_context.rs | 2 +- .../re_viewer_context/src/selection_state.rs | 2 +- .../re_viewer_context/src/test_context.rs | 16 +- .../re_viewer_context/src/time_control.rs | 79 +++++- .../src/typed_entity_collections.rs | 6 +- .../re_viewer_context/src/view/highlights.rs | 6 +- .../re_viewer_context/src/view/view_query.rs | 1 + crates/viewer/re_viewport/src/lib.rs | 3 + .../re_viewport/src/system_execution.rs | 2 +- .../release_checklist/check_graph_view.py | 11 - .../check_graph_view_multi_self_edges.py | 78 ------ 24 files changed, 390 insertions(+), 141 deletions(-) create mode 100644 crates/viewer/re_view_graph/tests/basic.rs create mode 100644 crates/viewer/re_view_graph/tests/snapshots/coincident_nodes.png create mode 100644 crates/viewer/re_view_graph/tests/snapshots/self_and_multi_edges.png delete mode 100644 tests/python/release_checklist/check_graph_view_multi_self_edges.py diff --git a/Cargo.lock b/Cargo.lock index dc064e820503..81994ebce82d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6668,10 +6668,12 @@ version = "0.22.0-alpha.1+dev" dependencies = [ "ahash", "egui", + "egui_kittest", "fjadra", "itertools 0.13.0", "nohash-hasher", "re_chunk", + "re_chunk_store", "re_data_ui", "re_entity_db", "re_format", @@ -6684,6 +6686,7 @@ dependencies = [ "re_ui", "re_view", "re_viewer_context", + "re_viewport", "re_viewport_blueprint", ] diff --git a/crates/store/re_dataframe/src/query.rs b/crates/store/re_dataframe/src/query.rs index b67c56b531b8..b5c8e9e2d2a2 100644 --- a/crates/store/re_dataframe/src/query.rs +++ b/crates/store/re_dataframe/src/query.rs @@ -167,7 +167,10 @@ impl QueryHandle { re_tracing::profile_scope!("init"); // The timeline doesn't matter if we're running in static-only mode. - let filtered_index = self.query.filtered_index.unwrap_or_default(); + let filtered_index = self + .query + .filtered_index + .unwrap_or_else(|| Timeline::new_sequence("")); // 1. Compute the schema for the query. let view_contents = store.schema_for_query(&self.query); @@ -332,8 +335,10 @@ impl QueryHandle { .map(|(_view_idx, descr)| match descr { ColumnDescriptor::Time(_) => None, ColumnDescriptor::Component(descr) => { - let query = - re_chunk::LatestAtQuery::new(Timeline::default(), TimeInt::STATIC); + let query = re_chunk::LatestAtQuery::new( + Timeline::new_sequence(""), + TimeInt::STATIC, + ); let results = cache.latest_at( &query, diff --git a/crates/store/re_log_types/src/time_point/timeline.rs b/crates/store/re_log_types/src/time_point/timeline.rs index d428b23d4176..d86463f57cb4 100644 --- a/crates/store/re_log_types/src/time_point/timeline.rs +++ b/crates/store/re_log_types/src/time_point/timeline.rs @@ -7,12 +7,6 @@ re_string_interner::declare_new_type!( pub struct TimelineName; ); -impl Default for TimelineName { - fn default() -> Self { - Self::from(String::default()) - } -} - // ---------------------------------------------------------------------------- /// A time frame/space, e.g. `log_time` or `frame_nr`, coupled with the type of time @@ -27,15 +21,6 @@ pub struct Timeline { typ: TimeType, } -impl Default for Timeline { - fn default() -> Self { - Self { - name: TimelineName::default(), - typ: TimeType::Sequence, - } - } -} - impl Timeline { /// For absolute or relative time. #[inline] diff --git a/crates/viewer/re_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_view_dataframe/src/dataframe_ui.rs index c8f5833347d8..711aaa5cd54b 100644 --- a/crates/viewer/re_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_view_dataframe/src/dataframe_ui.rs @@ -9,7 +9,7 @@ use itertools::Itertools; use re_chunk_store::{ColumnDescriptor, LatestAtQuery}; use re_dataframe::external::re_query::StorageEngineArcReadGuard; use re_dataframe::QueryHandle; -use re_log_types::{EntityPath, TimeInt, Timeline, TimelineName}; +use re_log_types::{EntityPath, TimeInt, TimeType, Timeline, TimelineName}; use re_types_core::ComponentName; use re_ui::UiExt as _; use re_viewer_context::{SystemCommandSender, ViewId, ViewerContext}; @@ -203,7 +203,11 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> { re_tracing::profile_function!(); // TODO(ab): actual static-only support - let filtered_index = self.query_handle.query().filtered_index.unwrap_or_default(); + let filtered_index = self + .query_handle + .query() + .filtered_index + .unwrap_or_else(|| Timeline::new("", TimeType::Sequence)); self.query_handle .seek_to_row(info.visible_rows.start as usize); @@ -270,8 +274,11 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> { let column = &self.selected_columns[cell.col_range.start]; // TODO(ab): actual static-only support - let filtered_index = - self.query_handle.query().filtered_index.unwrap_or_default(); + let filtered_index = self + .query_handle + .query() + .filtered_index + .unwrap_or_else(|| Timeline::new("", TimeType::Sequence)); // if this column can actually be hidden, then that's the corresponding action let hide_action = match column { @@ -396,7 +403,11 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> { .unwrap_or(TimeInt::MAX); // TODO(ab): actual static-only support - let filtered_index = self.query_handle.query().filtered_index.unwrap_or_default(); + let filtered_index = self + .query_handle + .query() + .filtered_index + .unwrap_or_else(|| Timeline::new("", TimeType::Sequence)); let latest_at_query = LatestAtQuery::new(filtered_index, timestamp); if ui.is_sizing_pass() { diff --git a/crates/viewer/re_view_graph/Cargo.toml b/crates/viewer/re_view_graph/Cargo.toml index b1f469349f5b..4da414d45aef 100644 --- a/crates/viewer/re_view_graph/Cargo.toml +++ b/crates/viewer/re_view_graph/Cargo.toml @@ -39,3 +39,8 @@ egui.workspace = true fjadra.workspace = true itertools.workspace = true nohash-hasher.workspace = true + +[dev-dependencies] +egui_kittest.workspace = true +re_chunk_store.workspace = true +re_viewport.workspace = true diff --git a/crates/viewer/re_view_graph/src/layout/geometry.rs b/crates/viewer/re_view_graph/src/layout/geometry.rs index 0a8e2d306d19..52498c900049 100644 --- a/crates/viewer/re_view_graph/src/layout/geometry.rs +++ b/crates/viewer/re_view_graph/src/layout/geometry.rs @@ -39,7 +39,6 @@ impl EdgeGeometry { } /// The starting position of an edge. - #[expect(unused)] pub fn source_pos(&self) -> Pos2 { match self.path { PathGeometry::Line { source, .. } | PathGeometry::CubicBezier { source, .. } => source, @@ -54,7 +53,6 @@ impl EdgeGeometry { } /// The direction of the edge at the source node (normalized). - #[expect(unused)] pub fn source_arrow_direction(&self) -> Vec2 { use PathGeometry::{CubicBezier, Line}; match self.path { diff --git a/crates/viewer/re_view_graph/src/layout/result.rs b/crates/viewer/re_view_graph/src/layout/result.rs index 7ed15553187b..85c03a5d33fb 100644 --- a/crates/viewer/re_view_graph/src/layout/result.rs +++ b/crates/viewer/re_view_graph/src/layout/result.rs @@ -41,7 +41,6 @@ impl Layout { } /// Gets the shape of an edge in the final layout. - #[expect(unused)] pub fn get_edge(&self, edge: &EdgeId) -> Option<&[EdgeGeometry]> { self.edges.get(edge).map(|es| es.as_slice()) } diff --git a/crates/viewer/re_view_graph/src/lib.rs b/crates/viewer/re_view_graph/src/lib.rs index ce9a4b6f2667..463e83317eed 100644 --- a/crates/viewer/re_view_graph/src/lib.rs +++ b/crates/viewer/re_view_graph/src/lib.rs @@ -9,4 +9,5 @@ mod ui; mod view; mod visualizers; +pub use ui::GraphViewState; pub use view::GraphView; diff --git a/crates/viewer/re_view_graph/src/visualizers/edges.rs b/crates/viewer/re_view_graph/src/visualizers/edges.rs index d2a8445e01b2..13695e781bfa 100644 --- a/crates/viewer/re_view_graph/src/visualizers/edges.rs +++ b/crates/viewer/re_view_graph/src/visualizers/edges.rs @@ -16,7 +16,7 @@ pub struct EdgesVisualizer { pub struct EdgeInstance { // We will need this in the future, when we want to select individual edges. - pub _instance: Instance, + pub instance: Instance, pub source: components::GraphNode, pub target: components::GraphNode, pub source_index: NodeId, @@ -86,7 +86,7 @@ impl VisualizerSystem for EdgesVisualizer { let target_index = NodeId::from_entity_node(entity_path, &target); EdgeInstance { - _instance: Instance::from(i as u64), + instance: Instance::from(i as u64), source, target, source_index, diff --git a/crates/viewer/re_view_graph/tests/basic.rs b/crates/viewer/re_view_graph/tests/basic.rs new file mode 100644 index 000000000000..596cd7fb945b --- /dev/null +++ b/crates/viewer/re_view_graph/tests/basic.rs @@ -0,0 +1,254 @@ +//! Basic tests for the graph view, mostly focused on edge cases (pun intended). + +use std::sync::Arc; + +use egui::Vec2; +use re_chunk_store::{Chunk, RowId}; +use re_entity_db::EntityPath; +use re_types::{components, Component as _}; +use re_view_graph::{GraphView, GraphViewState}; +use re_viewer_context::{test_context::TestContext, ViewClass, ViewClassExt as _, ViewId}; +use re_viewport_blueprint::ViewBlueprint; + +#[test] +pub fn coincident_nodes() { + let mut test_context = TestContext::default(); + let name = "coincident_nodes"; + + // It's important to first register the view class before adding any entities, + // otherwise the `VisualizerEntitySubscriber` for our visualizers doesn't exist yet, + // and thus will not find anything applicable to the visualizer. + test_context + .view_class_registry + .add_class::() + .unwrap(); + + let entity_path = EntityPath::from(name); + + let nodes = [ + components::GraphNode("A".into()), + components::GraphNode("B".into()), + ]; + + let edges = [components::GraphEdge(("A", "B").into())]; + + let directed = components::GraphType::Directed; + + let positions = [ + components::Position2D([42.0, 42.0].into()), + components::Position2D([42.0, 42.0].into()), + ]; + + let mut builder = Chunk::builder(entity_path.clone()); + builder = builder.with_sparse_component_batches( + RowId::new(), + [(test_context.active_timeline(), 1)], + [ + (components::GraphNode::descriptor(), Some(&nodes as _)), + (components::Position2D::descriptor(), Some(&positions as _)), + (components::GraphEdge::descriptor(), Some(&edges as _)), + (components::GraphType::descriptor(), Some(&[directed] as _)), + ], + ); + + test_context + .recording_store + .add_chunk(&Arc::new(builder.build().unwrap())) + .unwrap(); + + run_graph_view_and_save_snapshot(&mut test_context, name, Vec2::new(100.0, 100.0)).unwrap(); +} + +#[test] +pub fn self_and_multi_edges() { + let mut test_context = TestContext::default(); + let name = "self_and_multi_edges"; + + // It's important to first register the view class before adding any entities, + // otherwise the `VisualizerEntitySubscriber` for our visualizers doesn't exist yet, + // and thus will not find anything applicable to the visualizer. + test_context + .view_class_registry + .add_class::() + .unwrap(); + + let entity_path = EntityPath::from(name); + + let nodes = [ + components::GraphNode("A".into()), + components::GraphNode("B".into()), + ]; + + let edges = [ + // self-edges + components::GraphEdge(("A", "A").into()), + components::GraphEdge(("B", "B").into()), + // duplicated edges + components::GraphEdge(("A", "B").into()), + components::GraphEdge(("A", "B").into()), + components::GraphEdge(("B", "A").into()), + // duplicated self-edges + components::GraphEdge(("A", "A").into()), + // implicit edges + components::GraphEdge(("B", "C").into()), + components::GraphEdge(("C", "C").into()), + ]; + + let directed = components::GraphType::Directed; + + let positions = [ + components::Position2D([0.0, 0.0].into()), + components::Position2D([200.0, 200.0].into()), + ]; + + let mut builder = Chunk::builder(entity_path.clone()); + builder = builder.with_sparse_component_batches( + RowId::new(), + [(test_context.active_timeline(), 1)], + [ + (components::GraphNode::descriptor(), Some(&nodes as _)), + (components::Position2D::descriptor(), Some(&positions as _)), + (components::GraphEdge::descriptor(), Some(&edges as _)), + (components::GraphType::descriptor(), Some(&[directed] as _)), + ], + ); + + test_context + .recording_store + .add_chunk(&Arc::new(builder.build().unwrap())) + .unwrap(); + + run_graph_view_and_save_snapshot(&mut test_context, name, Vec2::new(400.0, 400.0)).unwrap(); +} + +pub fn setup_graph_view_blueprint( + test_context: &mut TestContext, +) -> Result> { + // Views are always logged at `/{view_id}` in the blueprint store. + let view_id = ViewId::hashed_from_str("/graph"); + + // Use the timeline that is queried for blueprints. + let timepoint = [(test_context.blueprint_query.timeline(), 0)]; + + let view_chunk = Chunk::builder(view_id.as_entity_path().clone()) + .with_archetype( + RowId::new(), + timepoint, + &re_types::blueprint::archetypes::ViewBlueprint::new(GraphView::identifier().as_str()), + ) + .build()?; + test_context + .blueprint_store + .add_chunk(&Arc::new(view_chunk))?; + + // TODO(andreas): can we use the `ViewProperty` utilities for this? + let view_contents_chunk = + Chunk::builder(format!("{}/ViewContents", view_id.as_entity_path()).into()) + .with_archetype( + RowId::new(), + timepoint, + &re_types::blueprint::archetypes::ViewContents::new(std::iter::once( + re_types::datatypes::Utf8::from("/**"), + )), + ) + .build()?; + test_context + .blueprint_store + .add_chunk(&Arc::new(view_contents_chunk))?; + + Ok(view_id) +} + +fn run_graph_view_and_save_snapshot( + test_context: &mut TestContext, + _name: &str, + size: Vec2, +) -> Result<(), Box> { + let view_id = setup_graph_view_blueprint(test_context)?; + let view_blueprint = ViewBlueprint::try_from_db( + view_id, + &test_context.blueprint_store, + &test_context.blueprint_query, + ) + .expect("failed to get view blueprint"); + + let mut view_state = GraphViewState::default(); + let class_identifier = GraphView::identifier(); + + let view_class_registry = &mut test_context.view_class_registry; + let applicable_entities_per_visualizer = view_class_registry + .applicable_entities_for_visualizer_systems(&test_context.recording_store.store_id()); + + // TODO(andreas): this is c&p from TestContext::run. Make it nicer plz ;) + let store_context = re_viewer_context::StoreContext { + app_id: "rerun_test".into(), + blueprint: &test_context.blueprint_store, + default_blueprint: None, + recording: &test_context.recording_store, + bundle: &Default::default(), + caches: &Default::default(), + hub: &Default::default(), + }; + + // Execute the queries for every `View` + test_context.query_results = std::iter::once((view_id, { + // TODO(andreas): This needs to be done in a store subscriber that exists per view (instance, not class!). + // Note that right now we determine *all* visualizable entities, not just the queried ones. + // In a store subscriber set this is fine, but on a per-frame basis it's wasteful. + let visualizable_entities = view_class_registry + .get_class_or_log_error(class_identifier) + .determine_visualizable_entities( + &applicable_entities_per_visualizer, + &test_context.recording_store, + &view_class_registry.new_visualizer_collection(class_identifier), + &view_blueprint.space_origin, + ); + + view_blueprint.contents.execute_query( + &store_context, + view_class_registry, + &test_context.blueprint_query, + view_id, + &visualizable_entities, + ) + })) + .collect(); + + //TODO(ab): this contains a lot of boilerplate which should be provided by helpers + let mut harness = egui_kittest::Harness::builder() + .with_size(size) + .build_ui(|ui| { + test_context.run(&ui.ctx().clone(), |viewer_ctx| { + let view_class = test_context + .view_class_registry + .get_class_or_log_error(GraphView::identifier()); + + let (view_query, system_execution_output) = re_viewport::execute_systems_for_view( + viewer_ctx, + &view_blueprint, + viewer_ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in? + &view_state, + ); + + view_class + .ui( + viewer_ctx, + ui, + &mut view_state, + &view_query, + system_execution_output, + ) + .expect("failed to run graph view ui"); + }); + + test_context.handle_system_commands(); + }); + + harness.run(); + + //TODO(#8245): enable this everywhere when we have a software renderer setup + #[cfg(target_os = "macos")] + harness.wgpu_snapshot(_name); + + Ok(()) +} diff --git a/crates/viewer/re_view_graph/tests/snapshots/coincident_nodes.png b/crates/viewer/re_view_graph/tests/snapshots/coincident_nodes.png new file mode 100644 index 000000000000..1f3b8b03f2e3 --- /dev/null +++ b/crates/viewer/re_view_graph/tests/snapshots/coincident_nodes.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d4fe9ef8314d411182e04989cd96d8923a9bc08629cadc01b1e496c32574ef2a +size 910 diff --git a/crates/viewer/re_view_graph/tests/snapshots/self_and_multi_edges.png b/crates/viewer/re_view_graph/tests/snapshots/self_and_multi_edges.png new file mode 100644 index 000000000000..192e27623ad1 --- /dev/null +++ b/crates/viewer/re_view_graph/tests/snapshots/self_and_multi_edges.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:87fb4df34ff38a9e10b81b0b3eb7a11cdfd9fa82684892254dadaa56c8b87320 +size 24417 diff --git a/crates/viewer/re_viewer/src/blueprint/validation.rs b/crates/viewer/re_viewer/src/blueprint/validation.rs index 3b4221ce5b04..a5e7ff8988ef 100644 --- a/crates/viewer/re_viewer/src/blueprint/validation.rs +++ b/crates/viewer/re_viewer/src/blueprint/validation.rs @@ -19,7 +19,7 @@ pub(crate) fn validate_component(blueprint: &EntityDb) -> bool { // Otherwise, our usage of serde-fields means we still might have a problem // this can go away once we stop using serde-fields. // Walk the blueprint and see if any cells fail to deserialize for this component type. - let query = LatestAtQuery::latest(Timeline::default()); + let query = LatestAtQuery::latest(Timeline::new_sequence("")); for path in blueprint.entity_paths() { if let Some(array) = engine .cache() diff --git a/crates/viewer/re_viewer_context/src/query_context.rs b/crates/viewer/re_viewer_context/src/query_context.rs index 67cc7e76fe7f..b2ce007dd84a 100644 --- a/crates/viewer/re_viewer_context/src/query_context.rs +++ b/crates/viewer/re_viewer_context/src/query_context.rs @@ -109,7 +109,7 @@ impl Clone for DataQueryResult { } /// A hierarchical tree of [`DataResult`]s -#[derive(Clone, Default)] +#[derive(Clone, Default, Debug)] pub struct DataResultTree { data_results: SlotMap, // TODO(jleibs): Decide if we really want to compute this per-query. diff --git a/crates/viewer/re_viewer_context/src/selection_state.rs b/crates/viewer/re_viewer_context/src/selection_state.rs index ec10c4833dcb..0e5276c25399 100644 --- a/crates/viewer/re_viewer_context/src/selection_state.rs +++ b/crates/viewer/re_viewer_context/src/selection_state.rs @@ -64,7 +64,7 @@ pub enum HoverHighlight { } /// Combination of selection & hover highlight which can occur independently. -#[derive(Copy, Clone, PartialEq, Eq, Default)] +#[derive(Copy, Clone, PartialEq, Eq, Default, Debug)] pub struct InteractionHighlight { pub selection: SelectionHighlight, pub hover: HoverHighlight, diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index b772f32dc21a..3e014354bc54 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use ahash::HashMap; use re_chunk_store::LatestAtQuery; use re_entity_db::EntityDb; use re_log_types::{StoreId, StoreKind}; @@ -7,8 +8,8 @@ use re_types_core::reflection::Reflection; use crate::{ blueprint_timeline, command_channel, ApplicationSelectionState, CommandReceiver, CommandSender, - ComponentUiRegistry, ItemCollection, RecordingConfig, StoreContext, SystemCommand, - ViewClassRegistry, ViewerContext, + ComponentUiRegistry, DataQueryResult, ItemCollection, RecordingConfig, StoreContext, + SystemCommand, ViewClassRegistry, ViewId, ViewerContext, }; /// Harness to execute code that rely on [`crate::ViewerContext`]. @@ -30,6 +31,9 @@ pub struct TestContext { pub selection_state: ApplicationSelectionState, pub recording_config: RecordingConfig, + // Populating this in `run` would pull in too many dependencies into the test harness for now. + pub query_results: HashMap, + pub blueprint_query: LatestAtQuery, pub component_ui_registry: ComponentUiRegistry, pub reflection: Reflection, @@ -63,6 +67,7 @@ impl Default for TestContext { selection_state: Default::default(), recording_config, blueprint_query, + query_results: Default::default(), component_ui_registry, reflection, command_sender, @@ -72,6 +77,11 @@ impl Default for TestContext { } impl TestContext { + /// Timeline the recording config is using by default. + pub fn active_timeline(&self) -> re_chunk::Timeline { + *self.recording_config.time_ctrl.read().timeline() + } + pub fn edit_selection(&mut self, edit_fn: impl FnOnce(&mut ApplicationSelectionState)) { edit_fn(&mut self.selection_state); @@ -107,7 +117,7 @@ impl TestContext { store_context: &store_context, applicable_entities_per_visualizer: &Default::default(), indicated_entities_per_visualizer: &Default::default(), - query_results: &Default::default(), + query_results: &self.query_results, rec_cfg: &self.recording_config, blueprint_cfg: &Default::default(), selection_state: &self.selection_state, diff --git a/crates/viewer/re_viewer_context/src/time_control.rs b/crates/viewer/re_viewer_context/src/time_control.rs index 45d5e1fbc929..be5596ff3b3a 100644 --- a/crates/viewer/re_viewer_context/src/time_control.rs +++ b/crates/viewer/re_viewer_context/src/time_control.rs @@ -137,7 +137,7 @@ pub struct TimeControl { impl Default for TimeControl { fn default() -> Self { Self { - timeline: ActiveTimeline::Auto(Timeline::default()), + timeline: ActiveTimeline::Auto(default_timeline([])), states: Default::default(), playing: true, following: true, @@ -440,9 +440,7 @@ impl TimeControl { if matches!(self.timeline, ActiveTimeline::Auto(_)) || !is_timeline_valid(self.timeline(), times_per_timeline) { - self.timeline = ActiveTimeline::Auto( - default_timeline(times_per_timeline.timelines()).map_or(Default::default(), |t| *t), - ); + self.timeline = ActiveTimeline::Auto(default_timeline(times_per_timeline.timelines())); } } @@ -586,18 +584,25 @@ fn range(values: &TimeCounts) -> ResolvedTimeRange { } /// Pick the timeline that should be the default, prioritizing user-defined ones. -fn default_timeline<'a>(timelines: impl Iterator) -> Option<&'a Timeline> { - let mut log_time_timeline = None; +fn default_timeline<'a>(timelines: impl IntoIterator) -> Timeline { + let mut found_log_tick = false; + let mut found_log_time = false; for timeline in timelines { - if timeline == &Timeline::log_time() { - log_time_timeline = Some(timeline); - } else if timeline != &Timeline::log_tick() { - return Some(timeline); // user timeline - always prefer! + if timeline == &Timeline::log_tick() { + found_log_tick = true; + } else if timeline == &Timeline::log_time() { + found_log_time = true; + } else { + return *timeline; } } - log_time_timeline + if found_log_tick && !found_log_time { + Timeline::log_tick() + } else { + Timeline::log_time() + } } fn step_fwd_time(time: TimeReal, values: &TimeCounts) -> TimeInt { @@ -656,3 +661,55 @@ fn step_back_time_looped( step_back_time(time, values).into() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_default_timeline() { + let log_time = Timeline::log_time(); + let log_tick = Timeline::log_tick(); + let custom_timeline0 = Timeline::new("my_timeline0", TimeType::Time); + let custom_timeline1 = Timeline::new("my_timeline1", TimeType::Time); + + assert_eq!(default_timeline([]), log_time); + assert_eq!(default_timeline([&log_tick]), log_tick); + assert_eq!(default_timeline([&log_time]), log_time); + assert_eq!(default_timeline([&log_time, &log_tick]), log_time); + assert_eq!( + default_timeline([&log_time, &log_tick, &custom_timeline0]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&custom_timeline0, &log_time, &log_tick]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&log_time, &custom_timeline0, &log_tick]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&custom_timeline0, &log_time]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&custom_timeline0, &log_tick]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&log_time, &custom_timeline0]), + custom_timeline0 + ); + assert_eq!( + default_timeline([&log_tick, &custom_timeline0]), + custom_timeline0 + ); + + assert_eq!( + default_timeline([&custom_timeline0, &custom_timeline1]), + custom_timeline0 + ); + assert_eq!(default_timeline([&custom_timeline0]), custom_timeline0); + } +} diff --git a/crates/viewer/re_viewer_context/src/typed_entity_collections.rs b/crates/viewer/re_viewer_context/src/typed_entity_collections.rs index bceb841c9912..1dfdd1729c3d 100644 --- a/crates/viewer/re_viewer_context/src/typed_entity_collections.rs +++ b/crates/viewer/re_viewer_context/src/typed_entity_collections.rs @@ -8,7 +8,7 @@ use crate::ViewSystemIdentifier; /// List of entities that are *applicable* to a given visualizer. /// /// An entity is applicable if it at any point in time on any timeline has all required components. -#[derive(Default, Clone)] +#[derive(Default, Clone, Debug)] pub struct ApplicableEntities(pub IntSet); impl std::ops::Deref for ApplicableEntities { @@ -45,7 +45,7 @@ impl std::ops::Deref for IndicatedEntities { /// /// This is a subset of [`ApplicableEntities`] and differs on a /// per view instance base. -#[derive(Default, Clone)] +#[derive(Default, Clone, Debug)] pub struct VisualizableEntities(pub IntSet); impl std::ops::Deref for VisualizableEntities { @@ -57,7 +57,7 @@ impl std::ops::Deref for VisualizableEntities { } } -#[derive(Default)] +#[derive(Default, Debug)] pub struct PerVisualizer(pub IntMap); impl std::ops::Deref for PerVisualizer { diff --git a/crates/viewer/re_viewer_context/src/view/highlights.rs b/crates/viewer/re_viewer_context/src/view/highlights.rs index b90a287c71fb..c158d8f5a78d 100644 --- a/crates/viewer/re_viewer_context/src/view/highlights.rs +++ b/crates/viewer/re_viewer_context/src/view/highlights.rs @@ -9,7 +9,7 @@ use crate::{HoverHighlight, InteractionHighlight, SelectionHighlight}; /// Highlights of a specific entity path in a specific view. /// /// Using this in bulk on many instances is faster than querying single objects. -#[derive(Default)] +#[derive(Default, Debug)] pub struct ViewEntityHighlight { overall: InteractionHighlight, instances: ahash::HashMap, @@ -70,7 +70,7 @@ impl OptionalViewEntityHighlight<'_> { } } -#[derive(Default)] +#[derive(Default, Debug)] pub struct ViewOutlineMasks { pub overall: OutlineMaskPreference, pub instances: ahash::HashMap, @@ -99,7 +99,7 @@ impl ViewOutlineMasks { /// Highlights in a specific view. /// /// Using this in bulk on many objects is faster than querying single objects. -#[derive(Default)] +#[derive(Default, Debug)] pub struct ViewHighlights { pub highlighted_entity_paths: IntMap, pub outlines_masks: IntMap, diff --git a/crates/viewer/re_viewer_context/src/view/view_query.rs b/crates/viewer/re_viewer_context/src/view/view_query.rs index 978ab1ba37f4..716fc5e4da15 100644 --- a/crates/viewer/re_viewer_context/src/view/view_query.rs +++ b/crates/viewer/re_viewer_context/src/view/view_query.rs @@ -247,6 +247,7 @@ impl DataResult { pub type PerSystemDataResults<'a> = BTreeMap>; +#[derive(Debug)] pub struct ViewQuery<'s> { /// The id of the space in which context the query happens. pub view_id: ViewId, diff --git a/crates/viewer/re_viewport/src/lib.rs b/crates/viewer/re_viewport/src/lib.rs index 5d1449a06318..35eaa13fc3db 100644 --- a/crates/viewer/re_viewport/src/lib.rs +++ b/crates/viewer/re_viewport/src/lib.rs @@ -16,3 +16,6 @@ pub mod external { pub use re_types; pub use re_view; } + +// TODO(andreas): cfg test this only? +pub use system_execution::execute_systems_for_view; diff --git a/crates/viewer/re_viewport/src/system_execution.rs b/crates/viewer/re_viewport/src/system_execution.rs index 396ccbba4fab..4e3052590cb3 100644 --- a/crates/viewer/re_viewport/src/system_execution.rs +++ b/crates/viewer/re_viewport/src/system_execution.rs @@ -56,7 +56,7 @@ fn run_view_systems( pub fn execute_systems_for_view<'a>( ctx: &'a ViewerContext<'_>, view: &'a ViewBlueprint, - latest_at: TimeInt, + latest_at: TimeInt, // <- TODO(andreas): why not ctx.current_query().at()? view_state: &dyn ViewState, ) -> (ViewQuery<'a>, SystemExecutionOutput) { re_tracing::profile_function!(view.class_identifier().as_str()); diff --git a/tests/python/release_checklist/check_graph_view.py b/tests/python/release_checklist/check_graph_view.py index 4998792806e9..d97fd53fdbe1 100644 --- a/tests/python/release_checklist/check_graph_view.py +++ b/tests/python/release_checklist/check_graph_view.py @@ -15,7 +15,6 @@ * `graph` has directed edges, while `graph2` has undirected edges. * `graph` and `graph2` are shown in two different viewers. * There is a third viewer, `Both`, that shows both `graph` and `graph2` in the same viewer. -* The `coincident` viewer shows two nodes, `A` and `B`, at the same position """ @@ -23,10 +22,6 @@ def log_readme() -> None: rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), static=True) -def log_coincident_nodes() -> None: - rr.log("coincident", rr.GraphNodes(["A", "B"], labels=["A", "B"], positions=[[-150, 0], [150, 0]])) - - def log_graphs() -> None: DATA = [ ("A", None), @@ -56,18 +51,12 @@ def run(args: Namespace) -> None: log_readme() log_graphs() - log_coincident_nodes() rr.send_blueprint( rrb.Grid( rrb.GraphView(origin="graph", name="Graph 1"), rrb.GraphView(origin="graph2", name="Graph 2"), rrb.GraphView(name="Both", contents=["/graph", "/graph2"]), - rrb.GraphView( - origin="coincident", - name="Coincident nodes", - overrides={"coincident": [rr.components.Position2D([0, 0])]}, - ), rrb.TextDocumentView(origin="readme", name="Instructions"), ), make_default=True, diff --git a/tests/python/release_checklist/check_graph_view_multi_self_edges.py b/tests/python/release_checklist/check_graph_view_multi_self_edges.py deleted file mode 100644 index bef301ff7c23..000000000000 --- a/tests/python/release_checklist/check_graph_view_multi_self_edges.py +++ /dev/null @@ -1,78 +0,0 @@ -from __future__ import annotations - -import os -from argparse import Namespace -from uuid import uuid4 - -import rerun as rr -import rerun.blueprint as rrb - -README = """\ -# Graph view (multi- and self-edges) - -Please check that both graph views show: -* two self-edges for `A`, a single one for `B`. -* Additionally, there should be: - * two edges from `A` to `B`. - * one edge from `B` to `A`. - * one edge connecting `B` to an implicit node `C`. - * a self-edge for `C`. -""" - - -def log_readme() -> None: - rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), static=True) - - -def log_multi_and_self_graph() -> None: - rr.log( - "graph", - rr.GraphNodes(["A", "B"], labels=["A", "B"]), - rr.GraphEdges( - [ - # self-edges - ("A", "A"), - ("B", "B"), - # duplicated edges - ("A", "B"), - ("A", "B"), - ("B", "A"), - # duplicated self-edges - ("A", "A"), - # implicit edges - ("B", "C"), - ("C", "C"), - ], - graph_type=rr.GraphType.Directed, - ), - ) - - -def run(args: Namespace) -> None: - rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4()) - - log_multi_and_self_graph() - log_readme() - - rr.send_blueprint( - rrb.Grid( - rrb.GraphView(origin="graph", name="Multiple edges and self-edges"), - rrb.GraphView( - origin="graph", - name="Multiple edges and self-edges (without labels)", - defaults=[rr.components.ShowLabels(False)], - ), - rrb.TextDocumentView(origin="readme", name="Instructions"), - ), - make_active=True, - make_default=True, - ) - - -if __name__ == "__main__": - import argparse - - parser = argparse.ArgumentParser(description="Interactive release checklist") - rr.script_add_args(parser) - args = parser.parse_args() - run(args)