Also remove nested inclusions when removing a subtree

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);

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);

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(),
             );
         }
     }

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()),
                         );

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