Skip to content

Commit

Permalink
Fix auto-layout of space views (#3524)
Browse files Browse the repository at this point in the history
### What
* Closes #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:

* rerun-io/egui_tiles#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)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/8abfdc1bbcd6ed36786600e59705f90b25017f7e/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
  • Loading branch information
emilk authored Sep 28, 2023
1 parent 1bbedce commit 421b6a7
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 15 deletions.
3 changes: 1 addition & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
12 changes: 11 additions & 1 deletion crates/re_viewport/src/blueprint_components/viewport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::collections::BTreeSet<SpaceViewId>>")]
pub space_view_keys: std::collections::BTreeSet<SpaceViewId>,
Expand All @@ -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]
Expand Down
35 changes: 26 additions & 9 deletions crates/re_viewport/src/viewport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
});
Expand Down Expand Up @@ -239,6 +247,9 @@ struct TabViewer<'a, 'b> {
ctx: &'a mut ViewerContext<'b>,
space_views: &'a mut BTreeMap<SpaceViewId, SpaceViewBlueprint>,
maximized: &'a mut Option<SpaceViewId>,

/// The user edited the tree.
edited: bool,
}

impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
Expand Down Expand Up @@ -444,6 +455,12 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
..Default::default()
}
}

// Callbacks:

fn on_edit(&mut self) {
self.edited = true;
}
}

fn space_view_ui(
Expand Down
11 changes: 9 additions & 2 deletions crates/re_viewport/src/viewport_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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:
Expand All @@ -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");
}
}

Expand Down
10 changes: 9 additions & 1 deletion crates/re_viewport/src/viewport_blueprint_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down

1 comment on commit 421b6a7

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.25.

Benchmark suite Current: 421b6a7 Previous: 29ab0e3 Ratio
mono_points_arrow/generate_message_bundles 23481575 ns/iter (± 2949716) 18783554 ns/iter (± 674035) 1.25

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.