Skip to content

Commit

Permalink
Also remove nested inclusions when removing a subtree (#5720)
Browse files Browse the repository at this point in the history
### What
- Resolves: #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)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/fe3ca4a5457776f4ade8171ce94b332b18fdf40e/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
jleibs authored Mar 29, 2024
1 parent 8782c5b commit bcd2092
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 14 deletions.
16 changes: 16 additions & 0 deletions crates/re_log_types/src/path/entity_path_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 24 additions & 4 deletions crates/re_space_view/src/space_view_contents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions crates/re_viewport/src/context_menu/actions/remove.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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(),
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/re_viewport/src/space_view_entity_picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
);
Expand All @@ -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()),
);
Expand Down
8 changes: 3 additions & 5 deletions crates/re_viewport/src/viewport_blueprint_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit bcd2092

Please sign in to comment.