From 421b6a708b9fa2b5ca4685fd168ee8ed61894fbc Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 28 Sep 2023 19:45:54 +0200 Subject: [PATCH] Fix auto-layout of space views (#3524) ### What * Closes https://github.com/rerun-io/rerun/issues/3521 The problem was `auto_layout` being set to `false` too early. This fix required a change to `egui_tiles`, so a patch-release of that needs to be done before we publish rerun 0.9: * https://github.com/rerun-io/egui_tiles/pull/29 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [ ] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3524) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3524) - [Docs preview](https://rerun.io/preview/8abfdc1bbcd6ed36786600e59705f90b25017f7e/docs) - [Examples preview](https://rerun.io/preview/8abfdc1bbcd6ed36786600e59705f90b25017f7e/examples) - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --- Cargo.lock | 3 +- Cargo.toml | 3 ++ .../src/blueprint_components/viewport.rs | 12 ++++++- crates/re_viewport/src/viewport.rs | 35 ++++++++++++++----- crates/re_viewport/src/viewport_blueprint.rs | 11 ++++-- .../re_viewport/src/viewport_blueprint_ui.rs | 10 +++++- 6 files changed, 59 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7ebcbd208d0a..0c0aa16f09f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1514,8 +1514,7 @@ dependencies = [ [[package]] name = "egui_tiles" version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e518e35195aa95b52de6bac44f53b649efd2caa4a65d066a6bd3a704ad62799a" +source = "git+https://github.com/rerun-io/egui_tiles.git?rev=b34432b2ff68eb2e087a41b241d70ec367a09334#b34432b2ff68eb2e087a41b241d70ec367a09334" dependencies = [ "ahash 0.8.3", "egui", diff --git a/Cargo.toml b/Cargo.toml index 072c43ba26e0..a17c9644aa7a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -164,3 +164,6 @@ debug = true # If that is not possible, patch to a branch that has a PR open on the upstream repo. # As a last resport, patch with a commit to our own repository. # ALWAYS document what PR the commit hash is part of, or when it was merged into the upstream trunk. + + +egui_tiles = { git = "https://github.com/rerun-io/egui_tiles.git", rev = "b34432b2ff68eb2e087a41b241d70ec367a09334" } diff --git a/crates/re_viewport/src/blueprint_components/viewport.rs b/crates/re_viewport/src/blueprint_components/viewport.rs index 70aa33e74f8d..e543cec6594f 100644 --- a/crates/re_viewport/src/blueprint_components/viewport.rs +++ b/crates/re_viewport/src/blueprint_components/viewport.rs @@ -60,7 +60,7 @@ re_log_types::arrow2convert_component_shim!(SpaceViewMaximized as "rerun.bluepri /// ]) /// ); /// ``` -#[derive(Clone, Default, ArrowField, ArrowSerialize, ArrowDeserialize)] +#[derive(Clone, ArrowField, ArrowSerialize, ArrowDeserialize)] pub struct ViewportLayout { #[arrow_field(type = "SerdeField>")] pub space_view_keys: std::collections::BTreeSet, @@ -71,6 +71,16 @@ pub struct ViewportLayout { pub auto_layout: bool, } +impl Default for ViewportLayout { + fn default() -> Self { + Self { + space_view_keys: Default::default(), + tree: Default::default(), + auto_layout: true, + } + } +} + re_log_types::arrow2convert_component_shim!(ViewportLayout as "rerun.blueprint.ViewportLayout"); #[test] diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 9f423a8a7514..60278ad889e4 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -141,23 +141,31 @@ impl<'a, 'b> Viewport<'a, 'b> { &mut blueprint.tree }; - let mut tab_viewer = TabViewer { - viewport_state: state, - ctx, - space_views: &mut blueprint.space_views, - maximized: &mut blueprint.maximized, - }; - ui.scope(|ui| { ui.spacing_mut().item_spacing.x = re_ui::ReUi::view_padding(); re_tracing::profile_scope!("tree.ui"); - let tree_before = tree.clone(); + + let mut tab_viewer = TabViewer { + viewport_state: state, + ctx, + space_views: &mut blueprint.space_views, + maximized: &mut blueprint.maximized, + edited: false, + }; + tree.ui(&mut tab_viewer, ui); // Detect if the user has moved a tab or similar. // If so we can no longer automatically change the layout without discarding user edits. - if blueprint.auto_layout && *tree != tree_before { + let is_dragging_a_tile = tree.dragged_id(ui.ctx()).is_some(); + if tab_viewer.edited || is_dragging_a_tile { + if blueprint.auto_layout { + re_log::trace!( + "The user is manipulating the egui_tiles tree - will no longer auto-layout" + ); + } + blueprint.auto_layout = false; } }); @@ -239,6 +247,9 @@ struct TabViewer<'a, 'b> { ctx: &'a mut ViewerContext<'b>, space_views: &'a mut BTreeMap, maximized: &'a mut Option, + + /// The user edited the tree. + edited: bool, } impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { @@ -444,6 +455,12 @@ impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { ..Default::default() } } + + // Callbacks: + + fn on_edit(&mut self) { + self.edited = true; + } } fn space_view_ui( diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index f9140498bc08..c7a8b7bb0393 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -177,6 +177,10 @@ impl<'a> ViewportBlueprint<'a> { } pub fn mark_user_interaction(&mut self) { + if self.auto_layout { + re_log::trace!("User edits - will no longer auto-layout"); + } + self.auto_layout = false; self.auto_space_views = false; } @@ -205,7 +209,7 @@ impl<'a> ViewportBlueprint<'a> { if self.auto_layout { // Re-run the auto-layout next frame: - re_log::trace!("No user edits yet - will re-run auto-layout"); + re_log::trace!("Added a space view with no user edits yet - will re-run auto-layout"); self.tree = Default::default(); } else { // Try to insert it in the tree, in the top level: @@ -214,11 +218,14 @@ impl<'a> ViewportBlueprint<'a> { if let Some(egui_tiles::Tile::Container(container)) = self.tree.tiles.get_mut(root_id) { + re_log::trace!("Inserting new space view into root container"); container.add_child(tile_id); } else { re_log::trace!("Root was not a container - will re-run auto-layout"); - self.tree = Default::default(); // we'll just re-initialize later instead + self.tree = Default::default(); } + } else { + re_log::trace!("No root found - will re-run auto-layout"); } } diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 91264b4947da..b1603fb799ef 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -37,7 +37,7 @@ impl ViewportBlueprint<'_> { egui_tiles::Tile::Pane(space_view_id) => space_view_id == focus_tab, egui_tiles::Tile::Container(_) => false, }); - re_log::trace!("Found {focus_tab}: {found}"); + re_log::trace!("Found tab {focus_tab}: {found}"); } for tile_id in remove { @@ -128,6 +128,10 @@ impl ViewportBlueprint<'_> { } if visibility_changed { + if self.auto_layout { + re_log::trace!("Container visibility changed - will no longer auto-layout"); + } + self.auto_layout = false; // Keep `auto_space_views` enabled. self.tree.set_visible(tile_id, visible); } @@ -193,6 +197,10 @@ impl ViewportBlueprint<'_> { item_ui::select_hovered_on_click(ctx, &response, &[item]); if visibility_changed { + if self.auto_layout { + re_log::trace!("Space view visibility changed - will no longer auto-layout"); + } + self.auto_layout = false; // Keep `auto_space_views` enabled. self.tree.set_visible(tile_id, visible); }