From bcd20921e4fc54e98da3daad82c7db61a9802b29 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 29 Mar 2024 08:42:05 -0400 Subject: [PATCH] Also remove nested inclusions when removing a subtree (#5720) ### What - Resolves: https://github.com/rerun-io/rerun/issues/5690 Introduce new helper method that not only adds an exclusion but also removes all the current matches within the subtree. ### 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: [app.rerun.io](https://app.rerun.io/pr/5720/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5720/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/5720/index.html?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/5720) - [Docs preview](https://rerun.io/preview/fe3ca4a5457776f4ade8171ce94b332b18fdf40e/docs) - [Examples preview](https://rerun.io/preview/fe3ca4a5457776f4ade8171ce94b332b18fdf40e/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- .../src/path/entity_path_filter.rs | 16 +++++++++++ .../re_space_view/src/space_view_contents.rs | 28 ++++++++++++++++--- .../src/context_menu/actions/remove.rs | 5 ++-- .../src/space_view_entity_picker.rs | 4 +-- .../re_viewport/src/viewport_blueprint_ui.rs | 8 ++---- 5 files changed, 47 insertions(+), 14 deletions(-) diff --git a/crates/re_log_types/src/path/entity_path_filter.rs b/crates/re_log_types/src/path/entity_path_filter.rs index 779e0a9fadc5..da33f0ed84c7 100644 --- a/crates/re_log_types/src/path/entity_path_filter.rs +++ b/crates/re_log_types/src/path/entity_path_filter.rs @@ -243,6 +243,22 @@ impl EntityPathFilter { ); } + /// Remove a subtree and any existing rules that it would match. + /// + /// Because most-specific matches win, if we only add a subtree exclusion + /// it can still be overridden by existing inclusions. This method ensures + /// that not only do we add a subtree exclusion, but clear out any existing + /// inclusions or (now redundant) exclusions that would match the subtree. + pub fn remove_subtree_and_matching_rules(&mut self, clone: EntityPath) { + let new_exclusion = EntityPathRule::including_subtree(clone); + + // Remove any rule that is a subtree of the new exclusion. + self.rules + .retain(|rule, _| !new_exclusion.matches(&rule.path)); + + self.rules.insert(new_exclusion, RuleEffect::Exclude); + } + /// Remove any rule for the given entity path (ignoring whether or not that rule includes the subtree). pub fn remove_rule_for(&mut self, entity_path: &EntityPath) { self.rules.retain(|rule, _| rule.path != *entity_path); diff --git a/crates/re_space_view/src/space_view_contents.rs b/crates/re_space_view/src/space_view_contents.rs index 9d63fda4cd5f..cb8181921648 100644 --- a/crates/re_space_view/src/space_view_contents.rs +++ b/crates/re_space_view/src/space_view_contents.rs @@ -184,15 +184,35 @@ impl SpaceViewContents { } } - pub fn add_entity_exclusion(&self, ctx: &ViewerContext<'_>, rule: EntityPathRule) { - // TODO(emilk): ignore new rule if it is already covered by existing rules (noop) + /// Remove a subtree and any existing rules that it would match. + /// + /// Because most-specific matches win, if we only add a subtree exclusion + /// it can still be overridden by existing inclusions. This method ensures + /// that not only do we add a subtree exclusion, but clear out any existing + /// inclusions or (now redundant) exclusions that would match the subtree. + pub fn remove_subtree_and_matching_rules(&self, ctx: &ViewerContext<'_>, path: EntityPath) { + let mut new_entity_path_filter = self.entity_path_filter.clone(); + new_entity_path_filter.remove_subtree_and_matching_rules(path); + self.set_entity_path_filter(ctx, &new_entity_path_filter); + } + + /// Directly add an exclusion rule to the [`EntityPathFilter`]. + /// + /// This is a direct modification of the filter and will not do any simplification + /// related to overlapping or conflicting rules. + /// + /// If you are trying to remove an entire subtree, prefer using [`Self::remove_subtree_and_matching_rules`]. + pub fn raw_add_entity_exclusion(&self, ctx: &ViewerContext<'_>, rule: EntityPathRule) { let mut new_entity_path_filter = self.entity_path_filter.clone(); new_entity_path_filter.add_rule(RuleEffect::Exclude, rule); self.set_entity_path_filter(ctx, &new_entity_path_filter); } - pub fn add_entity_inclusion(&self, ctx: &ViewerContext<'_>, rule: EntityPathRule) { - // TODO(emilk): ignore new rule if it is already covered by existing rules (noop) + /// Directly add an inclusion rule to the [`EntityPathFilter`]. + /// + /// This is a direct modification of the filter and will not do any simplification + /// related to overlapping or conflicting rules. + pub fn raw_add_entity_inclusion(&self, ctx: &ViewerContext<'_>, rule: EntityPathRule) { let mut new_entity_path_filter = self.entity_path_filter.clone(); new_entity_path_filter.add_rule(RuleEffect::Include, rule); self.set_entity_path_filter(ctx, &new_entity_path_filter); diff --git a/crates/re_viewport/src/context_menu/actions/remove.rs b/crates/re_viewport/src/context_menu/actions/remove.rs index 423bc5157f38..838abe636990 100644 --- a/crates/re_viewport/src/context_menu/actions/remove.rs +++ b/crates/re_viewport/src/context_menu/actions/remove.rs @@ -1,5 +1,4 @@ use re_entity_db::InstancePath; -use re_log_types::EntityPathRule; use re_viewer_context::{ContainerId, Contents, Item, SpaceViewId}; use crate::context_menu::{ContextMenuAction, ContextMenuContext}; @@ -48,9 +47,9 @@ impl ContextMenuAction for RemoveAction { instance_path: &InstancePath, ) { if let Some(space_view) = ctx.viewport_blueprint.space_view(space_view_id) { - space_view.contents.add_entity_exclusion( + space_view.contents.remove_subtree_and_matching_rules( ctx.viewer_context, - EntityPathRule::including_subtree(instance_path.entity_path.clone()), + instance_path.entity_path.clone(), ); } } diff --git a/crates/re_viewport/src/space_view_entity_picker.rs b/crates/re_viewport/src/space_view_entity_picker.rs index 898911edacb8..4b59093a9537 100644 --- a/crates/re_viewport/src/space_view_entity_picker.rs +++ b/crates/re_viewport/src/space_view_entity_picker.rs @@ -211,7 +211,7 @@ fn add_entities_line_ui( let response = ctx.re_ui.small_icon_button(ui, &re_ui::icons::REMOVE); if response.clicked() { - space_view.contents.add_entity_exclusion( + space_view.contents.raw_add_entity_exclusion( ctx, EntityPathRule::including_subtree(entity_tree.path.clone()), ); @@ -230,7 +230,7 @@ fn add_entities_line_ui( let response = ctx.re_ui.small_icon_button(ui, &re_ui::icons::ADD); if response.clicked() { - space_view.contents.add_entity_inclusion( + space_view.contents.raw_add_entity_inclusion( ctx, EntityPathRule::including_subtree(entity_tree.path.clone()), ); diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 32896c41d78a..09738b7e9312 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -5,7 +5,6 @@ use smallvec::SmallVec; use re_entity_db::InstancePath; use re_log_types::EntityPath; -use re_log_types::EntityPathRule; use re_space_view::SpaceViewBlueprint; use re_types::blueprint::components::Visible; use re_ui::{drag_and_drop::DropTarget, list_item::ListItem, ReUi}; @@ -600,10 +599,9 @@ impl Viewport<'_, '_> { "Remove group and all its children from the space view", ); if response.clicked() { - space_view.contents.add_entity_exclusion( - ctx, - EntityPathRule::including_subtree(entity_path.clone()), - ); + space_view + .contents + .remove_subtree_and_matching_rules(ctx, entity_path.clone()); } response | vis_response