From 454c830886412a06d6fe41edae4f98c2c8ef6c41 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 12 Mar 2024 09:29:32 +0100 Subject: [PATCH 1/6] Pass the hierarchy to the visitor closure when walking the viewport container hierarchy --- Cargo.lock | 1 + crates/re_viewport/Cargo.toml | 1 + .../actions/collapse_expand_all.rs | 2 +- crates/re_viewport/src/viewport_blueprint.rs | 37 ++++++++++++++++--- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 70d3ed4c4acb..6fc0a3245f96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5155,6 +5155,7 @@ dependencies = [ "re_ui", "re_viewer_context", "rmp-serde", + "smallvec", ] [[package]] diff --git a/crates/re_viewport/Cargo.toml b/crates/re_viewport/Cargo.toml index a43d9e6a5787..b10509c45c57 100644 --- a/crates/re_viewport/Cargo.toml +++ b/crates/re_viewport/Cargo.toml @@ -49,3 +49,4 @@ nohash-hasher.workspace = true once_cell.workspace = true rayon.workspace = true rmp-serde.workspace = true +smallvec.workspace = true diff --git a/crates/re_viewport/src/context_menu/actions/collapse_expand_all.rs b/crates/re_viewport/src/context_menu/actions/collapse_expand_all.rs index bcc1624043e4..74d9878fd02f 100644 --- a/crates/re_viewport/src/context_menu/actions/collapse_expand_all.rs +++ b/crates/re_viewport/src/context_menu/actions/collapse_expand_all.rs @@ -46,7 +46,7 @@ impl ContextMenuAction for CollapseExpandAllAction { fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { ctx.viewport_blueprint - .visit_contents_in_container(container_id, &mut |contents| match contents { + .visit_contents_in_container(container_id, &mut |contents, _| match contents { Contents::Container(container_id) => CollapseScope::BlueprintTree .container(*container_id) .set_open(&ctx.egui_context, self.open()), diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 428a7979a90b..e1f1a2887d3f 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -3,8 +3,9 @@ use std::sync::atomic::{AtomicBool, Ordering}; use ahash::HashMap; use egui_tiles::{SimplificationOptions, TileId}; - use nohash_hasher::IntSet; +use smallvec::SmallVec; + use re_data_store::LatestAtQuery; use re_entity_db::EntityPath; use re_log_types::hash::Hash64; @@ -439,26 +440,50 @@ impl ViewportBlueprint { ) } + /// Walk the entire [`Contents`] tree, starting from the root container. + /// + /// See [`Self::visit_contents_in_container`] for details. + pub fn visit_contents(&self, visitor: &mut impl FnMut(&Contents, &SmallVec<[ContainerId; 4]>)) { + if let Some(root_container) = self.root_container { + self.visit_contents_in_container(&root_container, visitor); + } + } + /// Walk the subtree defined by the provided container id and call `visitor` for each /// [`Contents`]. /// - /// Note: `visitor` is first called for the container passed in argument. + /// Note: + /// - `visitor` is first called for the container passed in argument + /// - `visitor`'s second argument contains the hierarchy leading to the visited contents, up to + /// (and including) the container passed in argument pub fn visit_contents_in_container( &self, container_id: &ContainerId, - visitor: &mut impl FnMut(&Contents), + visitor: &mut impl FnMut(&Contents, &SmallVec<[ContainerId; 4]>), + ) { + let mut hierarchy = SmallVec::new(); + self.visit_contents_in_container_impl(container_id, &mut hierarchy, visitor); + } + + fn visit_contents_in_container_impl( + &self, + container_id: &ContainerId, + hierarchy: &mut SmallVec<[ContainerId; 4]>, + visitor: &mut impl FnMut(&Contents, &SmallVec<[ContainerId; 4]>), ) { - visitor(&Contents::Container(*container_id)); + visitor(&Contents::Container(*container_id), hierarchy); if let Some(container) = self.container(container_id) { + hierarchy.push(*container_id); for contents in &container.contents { - visitor(contents); + visitor(contents, hierarchy); match contents { Contents::Container(container_id) => { - self.visit_contents_in_container(container_id, visitor); + self.visit_contents_in_container_impl(container_id, hierarchy, visitor); } Contents::SpaceView(_) => {} } } + hierarchy.pop(); } } From ca13a2240d6c520c56cd6bb818ba035a016dadbb Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 12 Mar 2024 16:57:40 +0100 Subject: [PATCH 2/6] Handle container/space view/data result // TODO: instance path --- .../re_viewport/src/viewport_blueprint_ui.rs | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index b606f0882d65..d2c2cdb72e20 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -62,6 +62,8 @@ impl Viewport<'_, '_> { .auto_shrink([true, false]) .show(ui, |ui| { ctx.re_ui.panel_content(ui, |_, ui| { + self.handle_focused_item(ctx, ui); + self.root_container_tree_ui(ctx, ui); let empty_space_response = @@ -81,6 +83,77 @@ impl Viewport<'_, '_> { }); } + fn handle_focused_item(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { + if let Some(focused_item) = ctx.focused_item { + match focused_item { + Item::Container(container_id) => { + self.expand_all_contents_until(ui.ctx(), &Contents::Container(*container_id)) + } + Item::SpaceView(space_view_id) => { + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)) + } + Item::DataResult(space_view_id, instance_path) => { + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); + self.expand_all_data_results_until( + ctx, + ui.ctx(), + space_view_id, + &instance_path.entity_path, + ); + } + Item::InstancePath(instance_path) => { + //TODO: all occurrences: expand, first occurance: scroll + } + + Item::StoreId(_) | Item::ComponentPath(_) => {} + } + } + } + + fn expand_all_contents_until(&self, egui_ctx: &egui::Context, focused_contents: &Contents) { + //TODO(ab): this could look nicer if `Contents` was declared in re_view_context :) + let expend_contents = |contents: &Contents| match contents { + Contents::Container(container_id) => CollapseScope::BlueprintTree + .container(*container_id) + .set_open(egui_ctx, true), + Contents::SpaceView(space_view_id) => CollapseScope::BlueprintTree + .space_view(*space_view_id) + .set_open(egui_ctx, true), + }; + + self.blueprint.visit_contents(&mut |contents, hierarchy| { + if contents == focused_contents { + expend_contents(contents); + for parent in hierarchy { + expend_contents(&Contents::Container(*parent)); + } + } + }); + } + + fn expand_all_data_results_until( + &self, + ctx: &ViewerContext<'_>, + egui_ctx: &egui::Context, + space_view_id: &SpaceViewId, + entity_path: &EntityPath, + ) { + let result_tree = &ctx.lookup_query_result(*space_view_id).tree; + if result_tree.lookup_node_by_path(entity_path).is_some() { + EntityPath::incremental_walk( + result_tree + .root_node() + .map(|node| &node.data_result.entity_path), + entity_path, + ) + .for_each(|entity_path| { + CollapseScope::BlueprintTree + .data_result(*space_view_id, entity_path) + .set_open(egui_ctx, true); + }); + } + } + /// If a group or spaceview has a total of this number of elements, show its subtree by default? fn default_open_for_data_result(group: &DataResultNode) -> bool { let num_children = group.children.len(); @@ -145,6 +218,7 @@ impl Viewport<'_, '_> { &item_response, SelectionUpdateBehavior::UseSelection, ); + scroll_to_me_if_focused(ctx, ui, &item, &item_response); ctx.select_hovered_on_click(&item_response, item); self.handle_root_container_drag_and_drop_interaction( @@ -218,6 +292,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); + scroll_to_me_if_focused(ctx, ui, &item, &response); ctx.select_hovered_on_click(&response, item); self.blueprint @@ -351,6 +426,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); + scroll_to_me_if_focused(ctx, ui, &item, &response); ctx.select_hovered_on_click(&response, item); let content = Contents::SpaceView(*space_view_id); @@ -526,6 +602,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); + scroll_to_me_if_focused(ctx, ui, &item, &response); ctx.select_hovered_on_click(&response, item); } @@ -756,6 +833,23 @@ impl Viewport<'_, '_> { } } } +// ---------------------------------------------------------------------------- + +fn scroll_to_me_if_focused( + ctx: &ViewerContext<'_>, + ui: &egui::Ui, + item: &Item, + response: &egui::Response, +) { + if Some(item) == ctx.focused_item.as_ref() { + // 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. + if !ui.clip_rect().contains_rect(response.rect) { + response.scroll_to_me(Some(egui::Align::Center)); + } + } +} // ---------------------------------------------------------------------------- From c38ef0b75ad6761f53805e96b9e161e3c6b5e0a7 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 12 Mar 2024 18:55:56 +0100 Subject: [PATCH 3/6] Blueprint tree now handles Item::EntityPath --- crates/re_viewport/src/viewport.rs | 7 ++ .../re_viewport/src/viewport_blueprint_ui.rs | 102 ++++++++++++------ 2 files changed, 77 insertions(+), 32 deletions(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 473395323298..14dfec54b82a 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -48,6 +48,13 @@ pub struct ViewportState { /// /// See [`ViewportState::is_candidate_drop_parent_container`] for details. candidate_drop_parent_container_id: Option, + + /// The item that should be focused on in the blueprint tree. + /// + /// Set at each frame by [`Viewport::tree_ui`]. This is similar to + /// [`ViewerContext::focused_item`] but account for how specifically the blueprint tree should + /// handle the focused item. + pub(crate) blueprint_tree_focused_item: Option, } static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index d2c2cdb72e20..771b53df41ae 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -1,6 +1,7 @@ use egui::{Response, Ui}; use itertools::Itertools; use re_data_ui::item_ui::guess_instance_path_icon; +use smallvec::SmallVec; use re_entity_db::InstancePath; use re_log_types::EntityPath; @@ -54,7 +55,7 @@ impl<'a> DataResultNodeOrPath<'a> { impl Viewport<'_, '_> { /// Show the blueprint panel tree view. - pub fn tree_ui(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { + pub fn tree_ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { re_tracing::profile_function!(); egui::ScrollArea::both() @@ -62,7 +63,7 @@ impl Viewport<'_, '_> { .auto_shrink([true, false]) .show(ui, |ui| { ctx.re_ui.panel_content(ui, |_, ui| { - self.handle_focused_item(ctx, ui); + self.state.blueprint_tree_focused_item = self.check_focused_item(ctx, ui); self.root_container_tree_ui(ctx, ui); @@ -83,14 +84,16 @@ impl Viewport<'_, '_> { }); } - fn handle_focused_item(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { - if let Some(focused_item) = ctx.focused_item { + fn check_focused_item(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) -> Option { + ctx.focused_item.as_ref().and_then(|focused_item| { match focused_item { Item::Container(container_id) => { - self.expand_all_contents_until(ui.ctx(), &Contents::Container(*container_id)) + self.expand_all_contents_until(ui.ctx(), &Contents::Container(*container_id)); + Some(focused_item.clone()) } Item::SpaceView(space_view_id) => { - self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)) + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); + ctx.focused_item.clone() } Item::DataResult(space_view_id, instance_path) => { self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); @@ -100,14 +103,37 @@ impl Viewport<'_, '_> { space_view_id, &instance_path.entity_path, ); + + ctx.focused_item.clone() } Item::InstancePath(instance_path) => { - //TODO: all occurrences: expand, first occurance: scroll + let space_view_ids = + self.list_space_views_with_entity(ctx, &instance_path.entity_path); + + // focus on the first matching data result + let res = space_view_ids + .first() + .map(|id| Item::DataResult(*id, instance_path.clone())); + + for space_view_id in space_view_ids { + self.expand_all_contents_until( + ui.ctx(), + &Contents::SpaceView(space_view_id), + ); + self.expand_all_data_results_until( + ctx, + ui.ctx(), + &space_view_id, + &instance_path.entity_path, + ); + } + + res } - Item::StoreId(_) | Item::ComponentPath(_) => {} + Item::StoreId(_) | Item::ComponentPath(_) => None, } - } + }) } fn expand_all_contents_until(&self, egui_ctx: &egui::Context, focused_contents: &Contents) { @@ -131,6 +157,24 @@ impl Viewport<'_, '_> { }); } + #[inline] + fn list_space_views_with_entity( + &self, + ctx: &ViewerContext<'_>, + entity_path: &EntityPath, + ) -> SmallVec<[SpaceViewId; 4]> { + let mut space_view_ids = SmallVec::new(); + self.blueprint.visit_contents(&mut |contents, _| { + if let Contents::SpaceView(space_view_id) = contents { + let result_tree = &ctx.lookup_query_result(*space_view_id).tree; + if result_tree.lookup_node_by_path(entity_path).is_some() { + space_view_ids.push(*space_view_id) + } + } + }); + space_view_ids + } + fn expand_all_data_results_until( &self, ctx: &ViewerContext<'_>, @@ -154,6 +198,17 @@ impl Viewport<'_, '_> { } } + fn scroll_to_me_if_focused(&self, ui: &egui::Ui, item: &Item, response: &egui::Response) { + if Some(item) == self.state.blueprint_tree_focused_item.as_ref() { + // 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. + if !ui.clip_rect().contains_rect(response.rect) { + response.scroll_to_me(Some(egui::Align::Center)); + } + } + } + /// If a group or spaceview has a total of this number of elements, show its subtree by default? fn default_open_for_data_result(group: &DataResultNode) -> bool { let num_children = group.children.len(); @@ -218,7 +273,7 @@ impl Viewport<'_, '_> { &item_response, SelectionUpdateBehavior::UseSelection, ); - scroll_to_me_if_focused(ctx, ui, &item, &item_response); + self.scroll_to_me_if_focused(ui, &item, &item_response); ctx.select_hovered_on_click(&item_response, item); self.handle_root_container_drag_and_drop_interaction( @@ -292,7 +347,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - scroll_to_me_if_focused(ctx, ui, &item, &response); + self.scroll_to_me_if_focused(ui, &item, &response); ctx.select_hovered_on_click(&response, item); self.blueprint @@ -379,7 +434,7 @@ impl Viewport<'_, '_> { ); // Show 'projections' if there's any items that weren't part of the tree under origin but are directly included. - // The later is important since `+ image/camera/**` necessarily has `image` and `image/camera` in the data result tree. + // The latter is important since `+ image/camera/**` necessarily has `image` and `image/camera` in the data result tree. let mut projections = Vec::new(); result_tree.visit(&mut |node| { if node @@ -387,7 +442,7 @@ impl Viewport<'_, '_> { .entity_path .starts_with(&space_view.space_origin) { - false // If its under the origin, we're not interested, stop recursing. + false // If it's under the origin, we're not interested, stop recursing. } else if node.data_result.tree_prefix_only { true // Keep recursing until we find a projection. } else { @@ -426,7 +481,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - scroll_to_me_if_focused(ctx, ui, &item, &response); + self.scroll_to_me_if_focused(ui, &item, &response); ctx.select_hovered_on_click(&response, item); let content = Contents::SpaceView(*space_view_id); @@ -602,7 +657,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - scroll_to_me_if_focused(ctx, ui, &item, &response); + self.scroll_to_me_if_focused(ui, &item, &response); ctx.select_hovered_on_click(&response, item); } @@ -833,23 +888,6 @@ impl Viewport<'_, '_> { } } } -// ---------------------------------------------------------------------------- - -fn scroll_to_me_if_focused( - ctx: &ViewerContext<'_>, - ui: &egui::Ui, - item: &Item, - response: &egui::Response, -) { - if Some(item) == ctx.focused_item.as_ref() { - // 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. - if !ui.clip_rect().contains_rect(response.rect) { - response.scroll_to_me(Some(egui::Align::Center)); - } - } -} // ---------------------------------------------------------------------------- From 4db3f92f43f3102201062fe9d02f7749b74b42b7 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Tue, 12 Mar 2024 19:00:33 +0100 Subject: [PATCH 4/6] Renaming and docs --- crates/re_viewport/src/viewport.rs | 2 +- .../re_viewport/src/viewport_blueprint_ui.rs | 24 ++++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 14dfec54b82a..8285bccc8897 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -54,7 +54,7 @@ pub struct ViewportState { /// Set at each frame by [`Viewport::tree_ui`]. This is similar to /// [`ViewerContext::focused_item`] but account for how specifically the blueprint tree should /// handle the focused item. - pub(crate) blueprint_tree_focused_item: Option, + pub(crate) blueprint_tree_scroll_to_item: Option, } static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 771b53df41ae..928dfa297bba 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -63,7 +63,7 @@ impl Viewport<'_, '_> { .auto_shrink([true, false]) .show(ui, |ui| { ctx.re_ui.panel_content(ui, |_, ui| { - self.state.blueprint_tree_focused_item = self.check_focused_item(ctx, ui); + self.state.blueprint_tree_scroll_to_item = self.handle_focused_item(ctx, ui); self.root_container_tree_ui(ctx, ui); @@ -84,7 +84,8 @@ impl Viewport<'_, '_> { }); } - fn check_focused_item(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) -> Option { + /// Expend all required items and compute which item we should scroll to. + fn handle_focused_item(&self, ctx: &ViewerContext<'_>, ui: &egui::Ui) -> Option { ctx.focused_item.as_ref().and_then(|focused_item| { match focused_item { Item::Container(container_id) => { @@ -136,6 +137,7 @@ impl Viewport<'_, '_> { }) } + /// Expand all containers until reaching the provided content. fn expand_all_contents_until(&self, egui_ctx: &egui::Context, focused_contents: &Contents) { //TODO(ab): this could look nicer if `Contents` was declared in re_view_context :) let expend_contents = |contents: &Contents| match contents { @@ -157,6 +159,7 @@ impl Viewport<'_, '_> { }); } + /// List all space views that have the provided entity as data result. #[inline] fn list_space_views_with_entity( &self, @@ -168,13 +171,15 @@ impl Viewport<'_, '_> { if let Contents::SpaceView(space_view_id) = contents { let result_tree = &ctx.lookup_query_result(*space_view_id).tree; if result_tree.lookup_node_by_path(entity_path).is_some() { - space_view_ids.push(*space_view_id) + space_view_ids.push(*space_view_id); } } }); space_view_ids } + /// Expand data results of the provided space view all the way to the provided entity. + #[allow(clippy::unused_self)] fn expand_all_data_results_until( &self, ctx: &ViewerContext<'_>, @@ -198,8 +203,9 @@ impl Viewport<'_, '_> { } } - fn scroll_to_me_if_focused(&self, ui: &egui::Ui, item: &Item, response: &egui::Response) { - if Some(item) == self.state.blueprint_tree_focused_item.as_ref() { + /// Check if the provided item should be scrolled to. + fn scroll_to_me_if_needed(&self, ui: &egui::Ui, item: &Item, response: &egui::Response) { + if Some(item) == self.state.blueprint_tree_scroll_to_item.as_ref() { // 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. @@ -273,7 +279,7 @@ impl Viewport<'_, '_> { &item_response, SelectionUpdateBehavior::UseSelection, ); - self.scroll_to_me_if_focused(ui, &item, &item_response); + self.scroll_to_me_if_needed(ui, &item, &item_response); ctx.select_hovered_on_click(&item_response, item); self.handle_root_container_drag_and_drop_interaction( @@ -347,7 +353,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - self.scroll_to_me_if_focused(ui, &item, &response); + self.scroll_to_me_if_needed(ui, &item, &response); ctx.select_hovered_on_click(&response, item); self.blueprint @@ -481,7 +487,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - self.scroll_to_me_if_focused(ui, &item, &response); + self.scroll_to_me_if_needed(ui, &item, &response); ctx.select_hovered_on_click(&response, item); let content = Contents::SpaceView(*space_view_id); @@ -657,7 +663,7 @@ impl Viewport<'_, '_> { &response, SelectionUpdateBehavior::UseSelection, ); - self.scroll_to_me_if_focused(ui, &item, &response); + self.scroll_to_me_if_needed(ui, &item, &response); ctx.select_hovered_on_click(&response, item); } From 3998c667d33564db491dbf065f0ddcda17a39862 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 13 Mar 2024 10:51:04 +0100 Subject: [PATCH 5/6] Fix bug and add release checklist --- .../re_viewport/src/viewport_blueprint_ui.rs | 23 ++++---- tests/python/release_checklist/check_focus.py | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 tests/python/release_checklist/check_focus.py diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 928dfa297bba..50c0554d6e75 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -189,17 +189,15 @@ impl Viewport<'_, '_> { ) { let result_tree = &ctx.lookup_query_result(*space_view_id).tree; if result_tree.lookup_node_by_path(entity_path).is_some() { - EntityPath::incremental_walk( - result_tree - .root_node() - .map(|node| &node.data_result.entity_path), - entity_path, - ) - .for_each(|entity_path| { - CollapseScope::BlueprintTree - .data_result(*space_view_id, entity_path) - .set_open(egui_ctx, true); - }); + if let Some(root_node) = result_tree.root_node() { + EntityPath::incremental_walk(Some(&root_node.data_result.entity_path), entity_path) + .chain(std::iter::once(root_node.data_result.entity_path.clone())) + .for_each(|entity_path| { + CollapseScope::BlueprintTree + .data_result(*space_view_id, entity_path) + .set_open(egui_ctx, true); + }); + } } } @@ -611,8 +609,7 @@ impl Viewport<'_, '_> { let response = list_item .show_collapsing( ui, - CollapseScope::BlueprintTree - .data_result(space_view.id, node.data_result.entity_path.clone()), + CollapseScope::BlueprintTree.data_result(space_view.id, entity_path.clone()), default_open, |_, ui| { for child in node.children.iter().sorted_by_key(|c| { diff --git a/tests/python/release_checklist/check_focus.py b/tests/python/release_checklist/check_focus.py new file mode 100644 index 000000000000..f2d1bf5a34cd --- /dev/null +++ b/tests/python/release_checklist/check_focus.py @@ -0,0 +1,57 @@ +from __future__ import annotations + +import os +from argparse import Namespace +from uuid import uuid4 + +import rerun as rr + +README = """ +# Focus checks + +## Preparation + +TODO(ab): automate this with blueprints +TODO(ab): add lots of stuff via blueprint to make the tree more crowded and check scrolling + +- Reset the blueprint +- Clone the 3D space view such as to have 2 of them. + +## Checks + +- Collapse all in the blueprint tree. +- Double-click on the box in the first space view, check corresponding space view expands. +- Collapse all in the blueprint tree. +- Double-click on the leaf "boxes3d" entity in the streams view, check both space views expand. +""" + + +def log_readme() -> None: + rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True) + + +def log_some_space_views() -> None: + rr.set_time_sequence("frame_nr", 0) + + rr.log( + "/objects/boxes/boxes3d", + rr.Boxes3D(centers=[[0, 0, 0], [1, 1.5, 1.15], [3, 2, 1]], half_sizes=[0.5, 1, 0.5] * 3), + ) + + +def run(args: Namespace) -> None: + # TODO(cmc): I have no idea why this works without specifying a `recording_id`, but + # I'm not gonna rely on it anyway. + rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4()) + + log_readme() + log_some_space_views() + + +if __name__ == "__main__": + import argparse + + parser = argparse.ArgumentParser(description="Interactive release checklist") + rr.script_add_args(parser) + args = parser.parse_args() + run(args) From ce5c582ffbe0844840846373a467a6907fb7b895 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler Date: Wed, 13 Mar 2024 10:59:34 +0100 Subject: [PATCH 6/6] Review comments --- crates/re_viewport/src/viewport_blueprint.rs | 2 +- .../re_viewport/src/viewport_blueprint_ui.rs | 78 +++++++++---------- 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index e1f1a2887d3f..ae0f09eab2f0 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -454,7 +454,7 @@ impl ViewportBlueprint { /// /// Note: /// - `visitor` is first called for the container passed in argument - /// - `visitor`'s second argument contains the hierarchy leading to the visited contents, up to + /// - `visitor`'s second argument contains the hierarchy leading to the visited contents, from /// (and including) the container passed in argument pub fn visit_contents_in_container( &self, diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 50c0554d6e75..293eb6863ee4 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -86,55 +86,51 @@ impl Viewport<'_, '_> { /// Expend all required items and compute which item we should scroll to. fn handle_focused_item(&self, ctx: &ViewerContext<'_>, ui: &egui::Ui) -> Option { - ctx.focused_item.as_ref().and_then(|focused_item| { - match focused_item { - Item::Container(container_id) => { - self.expand_all_contents_until(ui.ctx(), &Contents::Container(*container_id)); - Some(focused_item.clone()) - } - Item::SpaceView(space_view_id) => { - self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); - ctx.focused_item.clone() - } - Item::DataResult(space_view_id, instance_path) => { - self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); + let focused_item = ctx.focused_item.as_ref()?; + match focused_item { + Item::Container(container_id) => { + self.expand_all_contents_until(ui.ctx(), &Contents::Container(*container_id)); + Some(focused_item.clone()) + } + Item::SpaceView(space_view_id) => { + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); + ctx.focused_item.clone() + } + Item::DataResult(space_view_id, instance_path) => { + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(*space_view_id)); + self.expand_all_data_results_until( + ctx, + ui.ctx(), + space_view_id, + &instance_path.entity_path, + ); + + ctx.focused_item.clone() + } + Item::InstancePath(instance_path) => { + let space_view_ids = + self.list_space_views_with_entity(ctx, &instance_path.entity_path); + + // focus on the first matching data result + let res = space_view_ids + .first() + .map(|id| Item::DataResult(*id, instance_path.clone())); + + for space_view_id in space_view_ids { + self.expand_all_contents_until(ui.ctx(), &Contents::SpaceView(space_view_id)); self.expand_all_data_results_until( ctx, ui.ctx(), - space_view_id, + &space_view_id, &instance_path.entity_path, ); - - ctx.focused_item.clone() - } - Item::InstancePath(instance_path) => { - let space_view_ids = - self.list_space_views_with_entity(ctx, &instance_path.entity_path); - - // focus on the first matching data result - let res = space_view_ids - .first() - .map(|id| Item::DataResult(*id, instance_path.clone())); - - for space_view_id in space_view_ids { - self.expand_all_contents_until( - ui.ctx(), - &Contents::SpaceView(space_view_id), - ); - self.expand_all_data_results_until( - ctx, - ui.ctx(), - &space_view_id, - &instance_path.entity_path, - ); - } - - res } - Item::StoreId(_) | Item::ComponentPath(_) => None, + res } - }) + + Item::StoreId(_) | Item::ComponentPath(_) => None, + } } /// Expand all containers until reaching the provided content.