From 79bcb7004c17b30d051618467c754e4d1695d50b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 8 Apr 2024 17:45:38 +0200 Subject: [PATCH 1/2] Enforce root layout container to be always present --- .../re_blueprint_tree/src/blueprint_tree.rs | 17 +---- .../actions/move_contents_to_new_container.rs | 43 +++++------ .../re_context_menu/src/actions/remove.rs | 4 +- .../re_context_menu/src/actions/show_hide.rs | 4 +- crates/viewer/re_viewport/src/viewport.rs | 76 ++++++++----------- .../re_viewport_blueprint/src/container.rs | 16 ++++ .../src/viewport_blueprint.rs | 49 ++++++------ 7 files changed, 101 insertions(+), 108 deletions(-) diff --git a/crates/viewer/re_blueprint_tree/src/blueprint_tree.rs b/crates/viewer/re_blueprint_tree/src/blueprint_tree.rs index 24457cf04771..ab339c712be2 100644 --- a/crates/viewer/re_blueprint_tree/src/blueprint_tree.rs +++ b/crates/viewer/re_blueprint_tree/src/blueprint_tree.rs @@ -184,10 +184,7 @@ impl BlueprintTree { blueprint: &ViewportBlueprint, ui: &mut egui::Ui, ) { - let Some(container_id) = blueprint.root_container else { - // nothing to draw if there is no root container - return; - }; + let container_id = blueprint.root_container; let Some(container_blueprint) = blueprint.containers.get(&container_id) else { re_log::warn_once!("Cannot find root container {container_id}"); @@ -655,14 +652,12 @@ impl BlueprintTree { // root container. let target_container_id = if let Some(Item::Container(container_id)) = ctx.selection().single_item() { - Some(*container_id) + *container_id } else { blueprint.root_container }; - if let Some(target_container_id) = target_container_id { - show_add_space_view_or_container_modal(target_container_id); - } + show_add_space_view_or_container_modal(target_container_id); } } @@ -821,15 +816,11 @@ impl BlueprintTree { // TODO(ab): this is a rather primitive behavior. Ideally we should allow dropping in the last container based // on the horizontal position of the cursor. - let Some(root_container_id) = blueprint.root_container else { - return; - }; - if ui.rect_contains_pointer(empty_space) { let drop_target = re_ui::drag_and_drop::DropTarget::new( empty_space.x_range(), empty_space.top(), - Contents::Container(root_container_id), + Contents::Container(blueprint.root_container), usize::MAX, ); diff --git a/crates/viewer/re_context_menu/src/actions/move_contents_to_new_container.rs b/crates/viewer/re_context_menu/src/actions/move_contents_to_new_container.rs index 68f6fc11b6f0..f7104b0a54cf 100644 --- a/crates/viewer/re_context_menu/src/actions/move_contents_to_new_container.rs +++ b/crates/viewer/re_context_menu/src/actions/move_contents_to_new_container.rs @@ -22,9 +22,7 @@ impl ContextMenuAction for MoveContentsToNewContainerAction { ctx.selection.iter().all(|(item, _)| match item { Item::SpaceView(_) => true, - Item::Container(container_id) => { - ctx.viewport_blueprint.root_container != Some(*container_id) - } + Item::Container(container_id) => ctx.viewport_blueprint.root_container != *container_id, _ => false, }) } @@ -36,9 +34,7 @@ impl ContextMenuAction for MoveContentsToNewContainerAction { fn supports_item(&self, ctx: &ContextMenuContext<'_>, item: &Item) -> bool { match item { Item::SpaceView(_) => true, - Item::Container(container_id) => { - ctx.viewport_blueprint.root_container != Some(*container_id) - } + Item::Container(container_id) => ctx.viewport_blueprint.root_container != *container_id, _ => false, } } @@ -57,26 +53,25 @@ impl ContextMenuAction for MoveContentsToNewContainerAction { } fn process_selection(&self, ctx: &ContextMenuContext<'_>) { - if let Some(root_container_id) = ctx.viewport_blueprint.root_container { - let (target_container_id, target_position) = ctx - .clicked_item_enclosing_container_id_and_position() - .unwrap_or((root_container_id, 0)); + let root_container_id = ctx.viewport_blueprint.root_container; + let (target_container_id, target_position) = ctx + .clicked_item_enclosing_container_id_and_position() + .unwrap_or((root_container_id, 0)); - let contents = ctx - .selection - .iter() - .filter_map(|(item, _)| item.try_into().ok()) - .collect(); + let contents = ctx + .selection + .iter() + .filter_map(|(item, _)| item.try_into().ok()) + .collect(); - ctx.viewport_blueprint.move_contents_to_new_container( - contents, - self.0, - target_container_id, - target_position, - ); + ctx.viewport_blueprint.move_contents_to_new_container( + contents, + self.0, + target_container_id, + target_position, + ); - ctx.viewport_blueprint - .mark_user_interaction(ctx.viewer_context); - } + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); } } diff --git a/crates/viewer/re_context_menu/src/actions/remove.rs b/crates/viewer/re_context_menu/src/actions/remove.rs index a3d1393c6b48..a7390d0118b2 100644 --- a/crates/viewer/re_context_menu/src/actions/remove.rs +++ b/crates/viewer/re_context_menu/src/actions/remove.rs @@ -14,9 +14,7 @@ impl ContextMenuAction for RemoveAction { fn supports_item(&self, ctx: &ContextMenuContext<'_>, item: &Item) -> bool { match item { Item::SpaceView(_) => true, - Item::Container(container_id) => { - ctx.viewport_blueprint.root_container != Some(*container_id) - } + Item::Container(container_id) => ctx.viewport_blueprint.root_container != *container_id, Item::DataResult(_, instance_path) => instance_path.is_all(), _ => false, } diff --git a/crates/viewer/re_context_menu/src/actions/show_hide.rs b/crates/viewer/re_context_menu/src/actions/show_hide.rs index 89049fc00c4c..7c447e2d495d 100644 --- a/crates/viewer/re_context_menu/src/actions/show_hide.rs +++ b/crates/viewer/re_context_menu/src/actions/show_hide.rs @@ -15,7 +15,7 @@ impl ContextMenuAction for ShowAction { Item::Container(container_id) => { !ctx.viewport_blueprint .is_contents_visible(&Contents::Container(*container_id)) - && ctx.viewport_blueprint.root_container != Some(*container_id) + && ctx.viewport_blueprint.root_container != *container_id } Item::DataResult(space_view_id, instance_path) => { data_result_visible(ctx, space_view_id, instance_path).is_some_and(|vis| !vis) @@ -69,7 +69,7 @@ impl ContextMenuAction for HideAction { Item::Container(container_id) => { ctx.viewport_blueprint .is_contents_visible(&Contents::Container(*container_id)) - && ctx.viewport_blueprint.root_container != Some(*container_id) + && ctx.viewport_blueprint.root_container != *container_id } Item::DataResult(space_view_id, instance_path) => { data_result_visible(ctx, space_view_id, instance_path).unwrap_or(false) diff --git a/crates/viewer/re_viewport/src/viewport.rs b/crates/viewer/re_viewport/src/viewport.rs index 2cdc2e8607b9..213027cc873b 100644 --- a/crates/viewer/re_viewport/src/viewport.rs +++ b/crates/viewer/re_viewport/src/viewport.rs @@ -244,9 +244,8 @@ impl<'a> Viewport<'a> { ); reset = true; - } else if let Some(parent_id) = - parent_container.or(self.blueprint.root_container) - { + } else { + let parent_id = parent_container.unwrap_or(self.blueprint.root_container); let tile_id = self.tree.tiles.insert_pane(space_view_id); let container_tile_id = blueprint_id_to_tile_id(&parent_id); if let Some(egui_tiles::Tile::Container(container)) = @@ -266,31 +265,26 @@ impl<'a> Viewport<'a> { re_log::trace!("Root was not a container - will re-run auto-layout"); reset = true; } - } else { - re_log::trace!("No root found - will re-run auto-layout"); } self.tree_edited = true; } TreeAction::AddContainer(container_kind, parent_container) => { - if let Some(parent_id) = parent_container.or(self.blueprint.root_container) { - let tile_id = self - .tree - .tiles - .insert_container(egui_tiles::Container::new(container_kind, vec![])); - if let Some(egui_tiles::Tile::Container(container)) = - self.tree.tiles.get_mut(blueprint_id_to_tile_id(&parent_id)) - { - re_log::trace!("Inserting new space view into container {parent_id:?}"); - container.add_child(tile_id); - } else { - re_log::trace!( - "Parent or root was not a container - will re-run auto-layout" - ); - reset = true; - } + let parent_id = parent_container.unwrap_or(self.blueprint.root_container); + let tile_id = self + .tree + .tiles + .insert_container(egui_tiles::Container::new(container_kind, vec![])); + if let Some(egui_tiles::Tile::Container(container)) = + self.tree.tiles.get_mut(blueprint_id_to_tile_id(&parent_id)) + { + re_log::trace!("Inserting new space view into container {parent_id:?}"); + container.add_child(tile_id); } else { - re_log::trace!("No root found - will re-run auto-layout"); + re_log::trace!( + "Parent or root was not a container - will re-run auto-layout" + ); + reset = true; } self.tree_edited = true; @@ -461,7 +455,7 @@ struct TabViewer<'a, 'b> { ctx: &'a ViewerContext<'b>, viewport_blueprint: &'a ViewportBlueprint, maximized: &'a mut Option, - root_container_id: Option, + root_container_id: ContainerId, tree_action_sender: std::sync::mpsc::Sender, /// List of query & system execution results for each space view. @@ -773,26 +767,22 @@ impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { // drag and drop operation often lead to many spurious empty containers. To work // around this, we run a simplification pass when a drop occurs. - if let Some(root_container_id) = self.root_container_id { - if self - .tree_action_sender - .send(TreeAction::SimplifyContainer( - root_container_id, - egui_tiles::SimplificationOptions { - prune_empty_tabs: true, - prune_empty_containers: false, - prune_single_child_tabs: true, - prune_single_child_containers: false, - all_panes_must_have_tabs: true, - join_nested_linear_containers: false, - }, - )) - .is_err() - { - re_log::warn_once!( - "Channel between ViewportBlueprint and Viewport is broken" - ); - } + if self + .tree_action_sender + .send(TreeAction::SimplifyContainer( + self.root_container_id, + egui_tiles::SimplificationOptions { + prune_empty_tabs: true, + prune_empty_containers: false, + prune_single_child_tabs: true, + prune_single_child_containers: false, + all_panes_must_have_tabs: true, + join_nested_linear_containers: false, + }, + )) + .is_err() + { + re_log::warn_once!("Channel between ViewportBlueprint and Viewport is broken"); } self.edited = true; diff --git a/crates/viewer/re_viewport_blueprint/src/container.rs b/crates/viewer/re_viewport_blueprint/src/container.rs index 433846df1c69..982a5be1a5bd 100644 --- a/crates/viewer/re_viewport_blueprint/src/container.rs +++ b/crates/viewer/re_viewport_blueprint/src/container.rs @@ -35,6 +35,22 @@ pub struct ContainerBlueprint { pub grid_columns: Option, } +impl Default for ContainerBlueprint { + fn default() -> Self { + Self { + id: ContainerId::random(), + container_kind: egui_tiles::ContainerKind::Grid, + display_name: None, + contents: vec![], + col_shares: vec![], + row_shares: vec![], + active_tab: None, + visible: true, + grid_columns: None, + } + } +} + impl ContainerBlueprint { /// Attempt to load a [`ContainerBlueprint`] from the blueprint store. pub fn try_from_db( diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index 0ca7529f8165..5953a6a30aa4 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -33,7 +33,7 @@ pub struct ViewportBlueprint { pub containers: BTreeMap, /// The root container. - pub root_container: Option, + pub root_container: ContainerId, /// The layouts of all the space views. pub tree: egui_tiles::Tree, @@ -123,13 +123,28 @@ impl ViewportBlueprint { }) .unwrap_or_default(); - let containers: BTreeMap = all_container_ids + let mut containers: BTreeMap = all_container_ids .into_iter() .filter_map(|id| ContainerBlueprint::try_from_db(blueprint_db, query, id)) .map(|c| (c.id, c)) .collect(); - let root_container = root_container.map(|id| id.0.into()); + let root_container = root_container.map_or_else( + || { + // Create ephemeral root container if none was given. + // Note that this may happen again on every frame since it is *not* stored to the blueprint store, + // until there has been any other reason to store a (modified) blueprint. + // It's important that this id is stable across frames, otherwise the root container can't be selected. + let id = ContainerId::hashed_from_str("fallback_root_container"); + let root_container = ContainerBlueprint { + id, + ..Default::default() + }; + containers.insert(id, root_container); + id + }, + |id| id.0.into(), + ); // Auto layouting and auto space view are only enabled if no blueprint has been provided by the user. // Only enable auto-space-views if this is the app-default blueprint @@ -455,9 +470,7 @@ impl ViewportBlueprint { /// /// 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); - } + self.visit_contents_in_container(&self.root_container, visitor); } /// Walk the subtree defined by the provided container id and call `visitor` for each @@ -501,11 +514,7 @@ impl ViewportBlueprint { /// Given a predicate, finds the (first) matching contents by recursively walking from the root /// container. pub fn find_contents_by(&self, predicate: &impl Fn(&Contents) -> bool) -> Option { - if let Some(root_container) = self.root_container { - self.find_contents_in_container_by(predicate, &root_container) - } else { - None - } + self.find_contents_in_container_by(predicate, &self.root_container) } /// Given a predicate, finds the (first) matching contents by recursively walking from the given @@ -555,15 +564,11 @@ impl ViewportBlueprint { &self, contents: &Contents, ) -> Option<(ContainerId, usize)> { - if let Some(container_id) = self.root_container { - if *contents == Contents::Container(container_id) { - // root doesn't have a parent - return None; - } - self.find_parent_and_position_index_impl(contents, &container_id) - } else { - None + if *contents == Contents::Container(self.root_container) { + // root doesn't have a parent + return None; } + self.find_parent_and_position_index_impl(contents, &self.root_container) } fn find_parent_and_position_index_impl( @@ -915,7 +920,7 @@ impl ViewportBlueprint { fn build_tree_from_space_views_and_containers<'a>( space_views: impl Iterator, containers: impl Iterator, - root_container: Option, + root_container: ContainerId, ) -> egui_tiles::Tree { re_tracing::profile_function!(); let mut tree = egui_tiles::Tree::empty("viewport_tree"); @@ -937,9 +942,7 @@ fn build_tree_from_space_views_and_containers<'a>( } // And finally, set the root - if let Some(root_container) = root_container.map(|id| blueprint_id_to_tile_id(&id)) { - tree.root = Some(root_container); - } + tree.root = Some(blueprint_id_to_tile_id(&root_container)); tree } From 468aad199e154aeb9b630e28524475855494288c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 23 Oct 2024 16:49:51 +0200 Subject: [PATCH 2/2] fix creating a non-empty tile tree when there's no root --- .../src/viewport_blueprint.rs | 36 ++++++++----------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index 5953a6a30aa4..d33aa70e8d61 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -123,29 +123,12 @@ impl ViewportBlueprint { }) .unwrap_or_default(); - let mut containers: BTreeMap = all_container_ids + let containers: BTreeMap = all_container_ids .into_iter() .filter_map(|id| ContainerBlueprint::try_from_db(blueprint_db, query, id)) .map(|c| (c.id, c)) .collect(); - let root_container = root_container.map_or_else( - || { - // Create ephemeral root container if none was given. - // Note that this may happen again on every frame since it is *not* stored to the blueprint store, - // until there has been any other reason to store a (modified) blueprint. - // It's important that this id is stable across frames, otherwise the root container can't be selected. - let id = ContainerId::hashed_from_str("fallback_root_container"); - let root_container = ContainerBlueprint { - id, - ..Default::default() - }; - containers.insert(id, root_container); - id - }, - |id| id.0.into(), - ); - // Auto layouting and auto space view are only enabled if no blueprint has been provided by the user. // Only enable auto-space-views if this is the app-default blueprint let is_app_default_blueprint = blueprint_db @@ -159,7 +142,7 @@ impl ViewportBlueprint { let tree = build_tree_from_space_views_and_containers( space_views.values(), containers.values(), - root_container, + root_container.clone(), ); let past_viewer_recommendations = past_viewer_recommendations @@ -168,10 +151,15 @@ impl ViewportBlueprint { .cloned() .collect(); + let root_container_or_placeholder = root_container.clone().map_or_else( + || ContainerId::hashed_from_str("placeholder_root_container"), + |id| id.0.into(), + ); + Self { space_views, containers, - root_container, + root_container: root_container_or_placeholder, tree, maximized: maximized.map(|id| id.0.into()), auto_layout, @@ -920,7 +908,7 @@ impl ViewportBlueprint { fn build_tree_from_space_views_and_containers<'a>( space_views: impl Iterator, containers: impl Iterator, - root_container: ContainerId, + root_container: Option, ) -> egui_tiles::Tree { re_tracing::profile_function!(); let mut tree = egui_tiles::Tree::empty("viewport_tree"); @@ -942,7 +930,11 @@ fn build_tree_from_space_views_and_containers<'a>( } // And finally, set the root - tree.root = Some(blueprint_id_to_tile_id(&root_container)); + + if let Some(root_container) = root_container { + let root_container = ContainerId::from(root_container.0); + tree.root = Some(blueprint_id_to_tile_id(&root_container)); + } tree }