From ed05c75a0ce9d06c1dd786aefc94c22a8630ac7a Mon Sep 17 00:00:00 2001 From: Antoine Beyeler <49431240+abey79@users.noreply.github.com> Date: Wed, 18 Dec 2024 09:02:02 +0100 Subject: [PATCH] Fix multi-entity drag-and-drop only adding one entity (#8519) ### Related * Related to #8518 * Related to #8431 ### What Fix a bug in #8431 where multi-entity drag-and-drop would only add a single entity due to a foot-gun in our entity path filter mutation API (see #8518 for more). Also add a new checklist. --- crates/top/rerun/src/commands/entrypoint.rs | 6 +- crates/viewer/re_viewport/src/viewport_ui.rs | 22 ++++--- .../src/view_contents.rs | 34 ++++++++++- .../check_multi_entity_drag_and_drop.py | 60 +++++++++++++++++++ 4 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 tests/python/release_checklist/check_multi_entity_drag_and_drop.py diff --git a/crates/top/rerun/src/commands/entrypoint.rs b/crates/top/rerun/src/commands/entrypoint.rs index 5ef16f301e40..6ea1089cb469 100644 --- a/crates/top/rerun/src/commands/entrypoint.rs +++ b/crates/top/rerun/src/commands/entrypoint.rs @@ -621,7 +621,7 @@ where } fn run_impl( - main_thread_token: crate::MainThreadToken, + _main_thread_token: crate::MainThreadToken, _build_info: re_build_info::BuildInfo, call_source: CallSource, args: Args, @@ -838,10 +838,10 @@ fn run_impl( } else { #[cfg(feature = "native_viewer")] return re_viewer::run_native_app( - main_thread_token, + _main_thread_token, Box::new(move |cc| { let mut app = re_viewer::App::new( - main_thread_token, + _main_thread_token, _build_info, &call_source.app_env(), startup_options, diff --git a/crates/viewer/re_viewport/src/viewport_ui.rs b/crates/viewer/re_viewport/src/viewport_ui.rs index 602c57a7f67b..ceba4abe229e 100644 --- a/crates/viewer/re_viewport/src/viewport_ui.rs +++ b/crates/viewer/re_viewport/src/viewport_ui.rs @@ -6,7 +6,7 @@ use ahash::HashMap; use egui_tiles::{Behavior as _, EditAction}; use re_context_menu::{context_menu_ui_for_item, SelectionUpdateBehavior}; -use re_log_types::{EntityPath, EntityPathRule}; +use re_log_types::{EntityPath, EntityPathRule, RuleEffect}; use re_ui::{design_tokens, ContextExt as _, DesignTokens, Icon, UiExt as _}; use re_viewer_context::{ blueprint_id_to_tile_id, icon_for_container_kind, Contents, DragAndDropFeedback, @@ -277,14 +277,18 @@ impl ViewportUi { if ctx.egui_ctx.input(|i| i.pointer.any_released()) { egui::DragAndDrop::clear_payload(ctx.egui_ctx); - for entity in entities { - if can_entity_be_added(entity) { - view_blueprint.contents.raw_add_entity_inclusion( - ctx, - EntityPathRule::including_subtree(entity.clone()), - ); - } - } + view_blueprint + .contents + .mutate_entity_path_filter(ctx, |filter| { + for entity in entities { + if can_entity_be_added(entity) { + filter.add_rule( + RuleEffect::Include, + EntityPathRule::including_subtree(entity.clone()), + ); + } + } + }); ctx.selection_state() .set_selection(Item::View(view_blueprint.id)); diff --git a/crates/viewer/re_viewport_blueprint/src/view_contents.rs b/crates/viewer/re_viewport_blueprint/src/view_contents.rs index ef4079f1f515..7988a7c9d65a 100644 --- a/crates/viewer/re_viewport_blueprint/src/view_contents.rs +++ b/crates/viewer/re_viewport_blueprint/src/view_contents.rs @@ -142,6 +142,9 @@ impl ViewContents { ); } + /// Set the entity path filter. WARNING: a single mutation is allowed per frame, or data will be + /// lost. + //TODO(#8518): address this massive foot-gun. pub fn set_entity_path_filter( &self, ctx: &ViewerContext<'_>, @@ -184,40 +187,65 @@ impl ViewContents { } } - /// Remove a subtree and any existing rules that it would match. + /// Perform arbitrary mutation on the entity path filter. WARNING: a single mutation is allowed + /// per frame, or data will be lost. + /// + /// This method exists because of the single mutation per frame limitation. It currently is the + /// only way to perform multiple mutations on the entity path filter in a single frame. + //TODO(#8518): address this massive foot-gun. + pub fn mutate_entity_path_filter( + &self, + ctx: &ViewerContext<'_>, + f: impl FnOnce(&mut EntityPathFilter), + ) { + let mut new_entity_path_filter = self.entity_path_filter.clone(); + f(&mut new_entity_path_filter); + self.set_entity_path_filter(ctx, &new_entity_path_filter); + } + + /// Remove a subtree and any existing rules that it would match. WARNING: a single mutation is + /// allowed per frame, or data will be lost. /// /// 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. + //TODO(#8518): address this massive foot-gun. 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`]. + /// Directly add an exclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is + /// allowed per frame, or data will be lost. /// /// 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`]. + //TODO(#8518): address this massive foot-gun. 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); } - /// Directly add an inclusion rule to the [`EntityPathFilter`]. + /// Directly add an inclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is + /// allowed per frame, or data will be lost. /// /// This is a direct modification of the filter and will not do any simplification /// related to overlapping or conflicting rules. + //TODO(#8518): address this massive foot-gun. 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); } + /// Remove rules for a given entity. WARNING: a single mutation is allowed per frame, or data + /// will be lost. + //TODO(#8518): address this massive foot-gun. pub fn remove_filter_rule_for(&self, ctx: &ViewerContext<'_>, ent_path: &EntityPath) { let mut new_entity_path_filter = self.entity_path_filter.clone(); new_entity_path_filter.remove_rule_for(ent_path); diff --git a/tests/python/release_checklist/check_multi_entity_drag_and_drop.py b/tests/python/release_checklist/check_multi_entity_drag_and_drop.py new file mode 100644 index 000000000000..e541458b3881 --- /dev/null +++ b/tests/python/release_checklist/check_multi_entity_drag_and_drop.py @@ -0,0 +1,60 @@ +from __future__ import annotations + +import os +from argparse import Namespace +from uuid import uuid4 + +import numpy as np +import rerun as rr +import rerun.blueprint as rrb + +README = """\ +# Multi-entity drag-and-drop + +This test checks that dragging multiple entities to a view correctly adds all entities. + +1. Multi-select `cos_curve` and `line_curve` entities in the streams tree. +2. Drag them to the PLOT view. +3. _Expect_: both entities are visible in the plot view and each are listed in the view's entity path filter. +""" + + +def log_readme() -> None: + rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), static=True) + + +def blueprint() -> rrb.BlueprintLike: + return rrb.Vertical( + rrb.TextDocumentView(origin="readme"), + rrb.TimeSeriesView(origin="/", contents=[], name="PLOT"), + ) + + +def log_some_scalar_entities() -> None: + times = np.arange(100) + curves = [ + ("cos_curve", np.cos(times / 100 * 2 * np.pi)), + ("line_curve", times / 100 + 0.2), + ] + + time_column = rr.TimeSequenceColumn("frame", times) + + for path, curve in curves: + rr.send_columns(path, times=[time_column], components=[rr.components.ScalarBatch(curve)]) + + +def run(args: Namespace) -> None: + rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4()) + rr.send_blueprint(blueprint(), make_default=True, make_active=True) + + log_readme() + log_some_scalar_entities() + + +if __name__ == "__main__": + import argparse + + parser = argparse.ArgumentParser(description="Interactive release checklist") + rr.script_add_args(parser) + args = parser.parse_args() + run(args)