Skip to content

Commit

Permalink
Enforce root layout container to be always present (rerun-io#5856)
Browse files Browse the repository at this point in the history
### What

Previously, it was possible to end up without a root blueprint container
which can cause weird ui behavior like not being able to add space
views.
This is now solved by making up a root container upon reading the
blueprint layout from the data store.

(Review with whitespace diff disabled!)

### 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)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[rerun.io/viewer](https://rerun.io/viewer/pr/5856)
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/5856?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/5856?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5856)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
Wumpf authored Oct 29, 2024
1 parent 6599f87 commit 7174abe
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 109 deletions.
17 changes: 4 additions & 13 deletions crates/viewer/re_blueprint_tree/src/blueprint_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}
Expand All @@ -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,
}
}
Expand All @@ -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);
}
}
4 changes: 1 addition & 3 deletions crates/viewer/re_context_menu/src/actions/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down
4 changes: 2 additions & 2 deletions crates/viewer/re_context_menu/src/actions/show_hide.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
76 changes: 33 additions & 43 deletions crates/viewer/re_viewport/src/viewport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)) =
Expand All @@ -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;
Expand Down Expand Up @@ -461,7 +455,7 @@ struct TabViewer<'a, 'b> {
ctx: &'a ViewerContext<'b>,
viewport_blueprint: &'a ViewportBlueprint,
maximized: &'a mut Option<SpaceViewId>,
root_container_id: Option<ContainerId>,
root_container_id: ContainerId,
tree_action_sender: std::sync::mpsc::Sender<TreeAction>,

/// List of query & system execution results for each space view.
Expand Down Expand Up @@ -773,26 +767,22 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> 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;
Expand Down
16 changes: 16 additions & 0 deletions crates/viewer/re_viewport_blueprint/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,22 @@ pub struct ContainerBlueprint {
pub grid_columns: Option<u32>,
}

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(
Expand Down
43 changes: 19 additions & 24 deletions crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub struct ViewportBlueprint {
pub containers: BTreeMap<ContainerId, ContainerBlueprint>,

/// The root container.
pub root_container: Option<ContainerId>,
pub root_container: ContainerId,

/// The layouts of all the space views.
pub tree: egui_tiles::Tree<SpaceViewId>,
Expand Down Expand Up @@ -129,8 +129,6 @@ impl ViewportBlueprint {
.map(|c| (c.id, c))
.collect();

let root_container = root_container.map(|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
Expand All @@ -144,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
Expand All @@ -153,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,
Expand Down Expand Up @@ -455,9 +458,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
Expand Down Expand Up @@ -501,11 +502,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<Contents> {
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
Expand Down Expand Up @@ -555,15 +552,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(
Expand Down Expand Up @@ -915,7 +908,7 @@ impl ViewportBlueprint {
fn build_tree_from_space_views_and_containers<'a>(
space_views: impl Iterator<Item = &'a SpaceViewBlueprint>,
containers: impl Iterator<Item = &'a ContainerBlueprint>,
root_container: Option<ContainerId>,
root_container: Option<RootContainer>,
) -> egui_tiles::Tree<SpaceViewId> {
re_tracing::profile_function!();
let mut tree = egui_tiles::Tree::empty("viewport_tree");
Expand All @@ -937,8 +930,10 @@ 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);

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
Expand Down

0 comments on commit 7174abe

Please sign in to comment.