Skip to content

Commit

Permalink
Fix undo not working in some entity drag-and-drop instances (#8526)
Browse files Browse the repository at this point in the history
This is a band-aid patch to address an issue with entity path filter
which would result in the following behaviours:

- the entity path filter would be perma-writen to the blueprint store
when the view is selected (in some cases)
- in such cases, this would break undo on user change (aka with entity
drag-and-drop)

The core issue is deeper and might cause panics with rust 1.81+. The
TL;DR is that the current `EntityPathFilter` type is ambiguous as to
whether its content has substitutions (aka $origin) applied or not. In
particular `Eq` and `Ord` apply to unsubstituted, resp. substituted
content (which can lead to the above panic). This should be further
cleaned by having two structures, one unsubstituted and another
substituted.

---------

Co-authored-by: Clement Rey <[email protected]>
  • Loading branch information
abey79 and teh-cmc authored Dec 18, 2024
1 parent c5ee5d3 commit 9d38189
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
5 changes: 4 additions & 1 deletion crates/store/re_log_types/src/path/entity_path_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@ pub enum EntityPathFilterParseError {
}

/// A set of substitutions for entity paths.
///
/// Important: the same substitutions must be used in every place we parse entity path filters.
// TODO(ab): the above requirement is a foot-gun we already fell in and should be addressed.
#[derive(Default)]
pub struct EntityPathSubs(pub HashMap<String, String>);
pub struct EntityPathSubs(HashMap<String, String>);

impl EntityPathSubs {
/// Create a new set of substitutions from a single origin.
Expand Down
9 changes: 7 additions & 2 deletions crates/viewer/re_selection_panel/src/selection_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use re_data_ui::{
DataUi,
};
use re_entity_db::{EntityPath, InstancePath};
use re_log_types::{ComponentPath, EntityPathFilter};
use re_log_types::{ComponentPath, EntityPathFilter, EntityPathSubs};
use re_types::blueprint::components::Interactive;
use re_ui::{
icons,
Expand Down Expand Up @@ -559,7 +559,12 @@ fn entity_path_filter_ui(
}

// Apply the edit.
let new_filter = EntityPathFilter::parse_forgiving(&filter_string, &Default::default());
//
// NOTE: The comparison of `EntityPathFilter` is done on the _expanded_ data (i.e. with variables substituted),
// so we must make sure to expand the new filter too before we compare it to the existing one.
// See <https://github.com/rerun-io/rerun/pull/8526>
let new_filter =
EntityPathFilter::parse_forgiving(&filter_string, &EntityPathSubs::new_with_origin(origin));
if &new_filter == filter {
None // no change
} else {
Expand Down

0 comments on commit 9d38189

Please sign in to comment.