Skip to content

Commit

Permalink
Fix multi-entity drag-and-drop only adding one entity (#8519)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
abey79 authored Dec 18, 2024
1 parent e9b2bff commit ed05c75
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 15 deletions.
6 changes: 3 additions & 3 deletions crates/top/rerun/src/commands/entrypoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
22 changes: 13 additions & 9 deletions crates/viewer/re_viewport/src/viewport_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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));
Expand Down
34 changes: 31 additions & 3 deletions crates/viewer/re_viewport_blueprint/src/view_contents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>,
Expand Down Expand Up @@ -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);
Expand Down
60 changes: 60 additions & 0 deletions tests/python/release_checklist/check_multi_entity_drag_and_drop.py
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit ed05c75

Please sign in to comment.