diff --git a/crates/store/re_dataframe/examples/query.rs b/crates/store/re_dataframe/examples/query.rs index f4ffc1af746d..ffae993f81ee 100644 --- a/crates/store/re_dataframe/examples/query.rs +++ b/crates/store/re_dataframe/examples/query.rs @@ -3,8 +3,8 @@ use itertools::Itertools; use re_dataframe::{ - ChunkStoreConfig, EntityPathFilter, QueryEngine, QueryExpression, ResolvedTimeRange, - SparseFillStrategy, StoreKind, TimeInt, Timeline, + ChunkStoreConfig, EntityPathFilter, EntityPathSubs, QueryEngine, QueryExpression, + ResolvedTimeRange, SparseFillStrategy, StoreKind, TimeInt, Timeline, }; use re_log_encoding::VersionPolicy; @@ -30,7 +30,9 @@ fn main() -> anyhow::Result<()> { let time_to = args.get(4).map_or(TimeInt::MAX, |s| { TimeInt::new_temporal(s.parse::().unwrap()) }); - let entity_path_filter = EntityPathFilter::try_from(args.get(5).map_or("/**", |s| s.as_str()))?; + let entity_path_filter = + EntityPathFilter::parse_strict(args.get(5).map_or("/**", |s| s.as_str()))? + .resolve_strict(&EntityPathSubs::empty())?; // TODO(cmc): We need to take a selector, not a Timeline. let timeline = match timeline_name { diff --git a/crates/store/re_dataframe/src/engine.rs b/crates/store/re_dataframe/src/engine.rs index 3fc2bb875944..4d6bcdda75ad 100644 --- a/crates/store/re_dataframe/src/engine.rs +++ b/crates/store/re_dataframe/src/engine.rs @@ -4,7 +4,7 @@ use re_chunk::{EntityPath, TransportChunk}; use re_chunk_store::{ ChunkStore, ChunkStoreConfig, ChunkStoreHandle, ColumnDescriptor, QueryExpression, }; -use re_log_types::{EntityPathFilter, StoreId}; +use re_log_types::{ResolvedEntityPathFilter, StoreId}; use re_query::{QueryCache, QueryCacheHandle, StorageEngine, StorageEngineLike}; use crate::QueryHandle; @@ -104,7 +104,7 @@ impl QueryEngine { #[inline] pub fn iter_entity_paths_sorted<'a>( &self, - filter: &'a EntityPathFilter, + filter: &'a ResolvedEntityPathFilter, ) -> impl Iterator + 'a { self.engine.with(|store, _cache| { store diff --git a/crates/store/re_dataframe/src/lib.rs b/crates/store/re_dataframe/src/lib.rs index 064727f9b612..5713c3413ad2 100644 --- a/crates/store/re_dataframe/src/lib.rs +++ b/crates/store/re_dataframe/src/lib.rs @@ -17,7 +17,8 @@ pub use self::external::re_chunk_store::{ }; #[doc(no_inline)] pub use self::external::re_log_types::{ - EntityPath, EntityPathFilter, ResolvedTimeRange, StoreKind, TimeInt, Timeline, + EntityPath, EntityPathFilter, EntityPathSubs, ResolvedEntityPathFilter, ResolvedTimeRange, + StoreKind, TimeInt, Timeline, }; #[doc(no_inline)] pub use self::external::re_query::{QueryCache, QueryCacheHandle, StorageEngine}; diff --git a/crates/store/re_grpc_client/src/lib.rs b/crates/store/re_grpc_client/src/lib.rs index f324862dc3a8..f5bdc8a0ea4e 100644 --- a/crates/store/re_grpc_client/src/lib.rs +++ b/crates/store/re_grpc_client/src/lib.rs @@ -577,7 +577,7 @@ fn activate_catalog_blueprint( .with_archetype(RowId::new(), timepoint, &vb) .build()?; - let epf = EntityPathFilter::parse_forgiving("/**", &Default::default()); + let epf = EntityPathFilter::parse_forgiving("/**"); let vc = ViewContents::new(epf.iter_expressions()); let view_contents_chunk = ChunkBuilder::new( ChunkId::new(), diff --git a/crates/store/re_log_types/src/path/entity_path_filter.rs b/crates/store/re_log_types/src/path/entity_path_filter.rs index d88427a82412..bb51e2df2b5f 100644 --- a/crates/store/re_log_types/src/path/entity_path_filter.rs +++ b/crates/store/re_log_types/src/path/entity_path_filter.rs @@ -5,9 +5,9 @@ use itertools::Itertools as _; use crate::EntityPath; -/// Error returned by [`EntityPathFilter::parse_strict`]. +/// Error returned by [`EntityPathFilter::resolve_strict`] and [`EntityPathFilter::parse_strict`]. #[derive(thiserror::Error, Debug, PartialEq, Eq)] -pub enum EntityPathFilterParseError { +pub enum EntityPathFilterError { #[error("Path parse error: {0}")] PathParseError(#[from] crate::PathParseError), @@ -19,7 +19,6 @@ pub enum EntityPathFilterParseError { /// /// Important: the same substitutions must be used in every place we resolve [`EntityPathFilter`] to /// [`ResolvedEntityPathFilter`]. -#[derive(Default)] pub struct EntityPathSubs(HashMap); impl EntityPathSubs { @@ -27,6 +26,11 @@ impl EntityPathSubs { pub fn new_with_origin(origin: &EntityPath) -> Self { Self(std::iter::once(("origin".to_owned(), origin.to_string())).collect()) } + + /// No variable substitutions. + pub fn empty() -> Self { + Self(HashMap::default()) + } } /// A way to filter a set of `EntityPath`s. @@ -47,18 +51,22 @@ impl EntityPathSubs { /// (`/world/**` matches both `/world` and `/world/car/driver`). /// Other uses of `*` are not (yet) supported. /// -/// The rules are stored as-is, are not sorted and no variable substitutions are performed. +/// Since variable substitution (and thus path parsing) hasn't been performed yet, +/// the rules can not be sorted yet from general to specific, instead they are stored +/// in alphabetical order. /// To expand variables & evaluate the filter, use [`ResolvedEntityPathFilter`]. -#[derive(Clone, Default, PartialEq, Eq)] +#[derive(Clone, Default, PartialEq, Eq, Debug, Hash)] pub struct EntityPathFilter { - rules: HashMap, + rules: BTreeMap, } // Note: it's not possible to implement that for `S: AsRef` because this conflicts with some // blanket implementation in `core` :( -impl From<&str> for EntityPathFilter { - fn from(value: &str) -> Self { - Self::parse(value) +impl TryFrom<&str> for EntityPathFilter { + type Error = EntityPathFilterError; + + fn try_from(value: &str) -> Result { + Self::parse_strict(value) } } @@ -79,7 +87,7 @@ impl From<&str> for EntityPathFilter { /// The last rule matching `/world/car/hood` is `- /world/car/**`, so it is excluded. /// The last rule matching `/world` is `- /world`, so it is excluded. /// The last rule matching `/world/house` is `+ /world/**`, so it is included. -#[derive(Clone, Default)] +#[derive(Clone, Default, PartialEq, Eq, Hash)] pub struct ResolvedEntityPathFilter { rules: BTreeMap, } @@ -94,13 +102,16 @@ impl std::fmt::Debug for ResolvedEntityPathFilter { /// /// This is a raw, whitespace trimmed path expression without any variable substitutions. /// See [`EntityPathFilter`] for more information. -#[derive(Clone, Debug, PartialEq, Eq, Hash)] +/// +/// Note that ordering of unresolved entity path rules is simply alphabetical. +/// In contrast, [`ResolvedEntityPathRule`] are ordered by entity path from least specific to most specific. +#[derive(Clone, Debug, PartialEq, Eq, Hash, Ord, PartialOrd)] pub struct EntityPathRule(String); impl From for EntityPathRule { #[inline] fn from(entity_path: EntityPath) -> Self { - Self::exact(&entity_path) + Self::exact_entity(&entity_path) } } @@ -128,7 +139,7 @@ impl std::fmt::Display for EntityPathRule { } } -/// A path rule with all variables Resolved. +/// A path rule with all variables resolved to entity paths. #[derive(Clone, Debug)] pub struct ResolvedEntityPathRule { /// The original rule, with unresolved variables. @@ -138,7 +149,7 @@ pub struct ResolvedEntityPathRule { pub resolved_path: EntityPath, } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum RuleEffect { Include, Exclude, @@ -233,7 +244,23 @@ fn split_whitespace_smart(path: &'_ str) -> Vec<&'_ str> { } impl EntityPathFilter { - /// Parse an entity path filter from a string. + /// Iterates the expressions in alphabetical order. + /// + /// This is **not** the order the rules are evaluated in + /// (use [`ResolvedEntityPathFilter::iter_unresolved_expressions`] for that instead). + pub fn iter_expressions(&self) -> impl Iterator + '_ { + self.rules.iter().map(|(filter, effect)| { + let mut s = String::new(); + s.push_str(match effect { + RuleEffect::Include => "+ ", + RuleEffect::Exclude => "- ", + }); + s.push_str(&filter.0); + s + }) + } + + /// Parse an entity path filter from a string, ignoring any parsing errors. /// /// Example of rules: /// @@ -246,14 +273,36 @@ impl EntityPathFilter { /// Each line is a rule. /// /// The first character should be `+` or `-`. If missing, `+` is assumed. - /// The rest of the line is trimmed and treated as an entity path. + /// The rest of the line is trimmed and treated as an entity path after variable substitution through [`Self::resolve`]. /// /// Conflicting rules are resolved by the last rule. - pub fn parse(rules: &str) -> Self { + pub fn parse_forgiving(rules: &str) -> Self { let split_rules = split_whitespace_smart(rules); Self::from_query_expressions(split_rules) } + /// Parse an entity path filter from a string. + /// + /// Example of rules: + /// + /// ```diff + /// + /world/** + /// - /world/roads/** + /// + /world/roads/main + /// ``` + /// + /// Each line is a rule. + /// + /// The first character should be `+` or `-`. If missing, `+` is assumed. + /// The rest of the line is trimmed and treated as an entity path after variable substitution through [`Self::resolve`]. + /// + /// Conflicting rules are resolved by the last rule. + #[allow(clippy::unnecessary_wraps)] + pub fn parse_strict(rules: &str) -> Result { + // TODO(andreas): Do some error checking here. + Ok(Self::parse_forgiving(rules)) + } + /// Build a filter from a list of query expressions. /// /// Each item in the iterator should be a query expression. @@ -295,38 +344,68 @@ impl EntityPathFilter { pub fn all() -> Self { Self { rules: std::iter::once(( - EntityPathRule::exact(&EntityPath::root()), + EntityPathRule::exact_entity(&EntityPath::root()), RuleEffect::Include, )) .collect(), } } + /// Include this path or variable expression, but not the subtree. + pub fn add_exact(&mut self, path_or_variable: &str) { + self.rules + .insert(EntityPathRule::exact(path_or_variable), RuleEffect::Include); + } + /// Include this entity, but not the subtree. - pub fn add_exact(&mut self, path: &EntityPath) { + pub fn add_exact_entity(&mut self, path: &EntityPath) { self.rules - .insert(EntityPathRule::exact(path), RuleEffect::Include); + .insert(EntityPathRule::exact_entity(path), RuleEffect::Include); + } + + /// Include this entity or variable expression with subtree. + pub fn add_subtree(&mut self, path_or_variable: &str) { + self.rules.insert( + EntityPathRule::including_subtree(path_or_variable), + RuleEffect::Include, + ); } /// Include this entity with subtree. - pub fn add_subtree(&mut self, subtree: &EntityPath) { + pub fn add_entity_subtree(&mut self, entity_path: &EntityPath) { self.rules.insert( - EntityPathRule::including_subtree(subtree), + EntityPathRule::including_entity_subtree(entity_path), RuleEffect::Include, ); } + /// Creates a new entity path filter that includes only a single path or variable expression. + pub fn single_filter(path_or_variable: &str) -> Self { + let mut filter = Self::default(); + filter.add_exact(path_or_variable); + filter + } + /// Creates a new entity path filter that includes only a single entity. pub fn single_entity_filter(entity_path: &EntityPath) -> Self { let mut filter = Self::default(); - filter.add_exact(entity_path); + filter.add_exact_entity(entity_path); + filter + } + + /// Creates a new entity path filter that includes a single subtree. + pub fn subtree_filter(path_or_variable: &str) -> Self { + let mut filter = Self::default(); + filter.add_subtree(path_or_variable); filter } /// Creates a new entity path filter that includes a single subtree. + /// + /// To use this with unsubsituted variables, use [`Self::subtree_filter`] instead. pub fn subtree_entity_filter(entity_path: &EntityPath) -> Self { let mut filter = Self::default(); - filter.add_subtree(entity_path); + filter.add_entity_subtree(entity_path); filter } @@ -349,7 +428,7 @@ impl EntityPathFilter { pub fn resolve_strict( self, subst_env: &EntityPathSubs, - ) -> Result { + ) -> Result { let rules = self .rules .into_iter() @@ -364,8 +443,23 @@ impl EntityPathFilter { } impl ResolvedEntityPathFilter { + /// Turns the resolved filter back into an unresolved filter. + pub fn unresolved(&self) -> EntityPathFilter { + EntityPathFilter { + rules: self + .rules + .iter() + .map(|(rule, effect)| (rule.rule.clone(), *effect)) + .collect(), + } + } + /// Iterate over the raw expressions of the rules, displaying the raw unresolved expressions. - pub fn iter_unresolved_rules(&self) -> impl Iterator + '_ { + /// + /// Note that they are iterated in the order of the resolved rules in contrast to [`EntityPathFilter::iter_expressions`]. + pub fn iter_unresolved_expressions(&self) -> impl Iterator + '_ { + // Do **not** call `unresolved()` because this would yield a different order! + self.rules.iter().map(|(filter, effect)| { let mut s = String::new(); s.push_str(match effect { @@ -378,7 +472,7 @@ impl ResolvedEntityPathFilter { } pub fn formatted(&self) -> String { - self.iter_unresolved_rules().join("\n") + self.iter_unresolved_expressions().join("\n") } /// Find the most specific matching rule and return its effect. @@ -416,16 +510,23 @@ impl ResolvedEntityPathFilter { }) } + /// Adds a rule to this filter. + /// + /// If there's already an effect for the rule, it is overwritten with the new effect. + pub fn add_rule(&mut self, effect: RuleEffect, rule: ResolvedEntityPathRule) { + self.rules.insert(rule, effect); + } + /// 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, path: EntityPath) { + pub fn remove_subtree_and_matching_rules(&mut self, entity_path: EntityPath) { let new_exclusion = ResolvedEntityPathRule { - rule: EntityPathRule::including_subtree(&path), - resolved_path: path, + rule: EntityPathRule::including_entity_subtree(&entity_path), + resolved_path: entity_path, }; // Remove any rule that is a subtree of the new exclusion. @@ -578,16 +679,28 @@ impl EntityPathRule { self.0.ends_with("/**") } + /// Match this path or variable expression, but not children. + #[inline] + pub fn exact(path_or_variable: &str) -> Self { + Self(path_or_variable.to_owned()) + } + /// Match this path, but not children. #[inline] - pub fn exact(path: &EntityPath) -> Self { + pub fn exact_entity(path: &EntityPath) -> Self { Self(path.to_string()) } + /// Match this path or variable expression and any entity in its subtree. + #[inline] + pub fn including_subtree(path_or_variable: &str) -> Self { + Self(format!("{path_or_variable}/**")) + } + /// Match this path and any entity in its subtree. #[inline] - pub fn including_subtree(path: &EntityPath) -> Self { - Self(format!("{path}/**")) + pub fn including_entity_subtree(entity_path: &EntityPath) -> Self { + Self::including_subtree(&entity_path.to_string()) } pub fn parse(expression: &str) -> Self { @@ -606,7 +719,25 @@ impl ResolvedEntityPathRule { } } - fn subsitute_variables(rule: &EntityPathRule, subst_env: &EntityPathSubs) -> String { + /// Match this path, but not children. + #[inline] + pub fn exact_entity(path: &EntityPath) -> Self { + Self { + rule: EntityPathRule::exact_entity(path), + resolved_path: path.clone(), + } + } + + /// Match this path and any entity in its subtree. + #[inline] + pub fn including_subtree(entity_path: &EntityPath) -> Self { + Self { + rule: EntityPathRule::including_entity_subtree(entity_path), + resolved_path: entity_path.clone(), + } + } + + fn substitute_variables(rule: &EntityPathRule, subst_env: &EntityPathSubs) -> String { // TODO(#5528): This is a very naive implementation of variable substitution. // unclear if we want to do this here, push this down into `EntityPath::parse`, // or even supported deferred evaluation on the `EntityPath` itself. @@ -621,15 +752,15 @@ impl ResolvedEntityPathRule { pub fn parse_strict( expression: &str, subst_env: &EntityPathSubs, - ) -> Result { + ) -> Result { let rule = EntityPathRule::parse(expression); - let expression_sub = Self::subsitute_variables(&rule, subst_env); + let expression_sub = Self::substitute_variables(&rule, subst_env); // Check for unresolved substitutions. if let Some(start) = expression_sub.find('$') { let rest = &expression_sub[start + 1..]; let end = rest.find(char::is_whitespace).unwrap_or(rest.len()); - return Err(EntityPathFilterParseError::UnresolvedSubstitution( + return Err(EntityPathFilterError::UnresolvedSubstitution( rest[..end].to_owned(), )); } @@ -654,7 +785,7 @@ impl ResolvedEntityPathRule { pub fn parse_forgiving(expression: &str, subst_env: &EntityPathSubs) -> Self { let rule = EntityPathRule::parse(expression); - let expression_sub = Self::subsitute_variables(&rule, subst_env); + let expression_sub = Self::substitute_variables(&rule, subst_env); if expression_sub == "/**" { Self { @@ -693,7 +824,7 @@ impl std::fmt::Display for ResolvedEntityPathRule { impl PartialEq for ResolvedEntityPathRule { fn eq(&self, other: &Self) -> bool { - // Careful! This has to check the same fields as `Ord`! + // Careful! This has to check the same fields as `Ord`/`Hash`! self.rule.include_subtree() == other.rule.include_subtree() && self.resolved_path == other.resolved_path } @@ -705,7 +836,7 @@ impl std::cmp::Ord for ResolvedEntityPathRule { /// Most specific last, which means recursive first. #[inline] fn cmp(&self, other: &Self) -> std::cmp::Ordering { - // Careful! This has to check the same fields as `PartialEq`! + // Careful! This has to check the same fields as `PartialEq`/`Hash`! (&self.resolved_path, !self.rule.include_subtree()) .cmp(&(&other.resolved_path, !other.rule.include_subtree())) } @@ -718,6 +849,12 @@ impl std::cmp::PartialOrd for ResolvedEntityPathRule { } } +impl std::hash::Hash for ResolvedEntityPathRule { + fn hash(&self, state: &mut H) { + (self.rule.include_subtree(), self.resolved_path.hash()).hash(state); + } +} + #[cfg(test)] mod tests { use crate::{ @@ -726,9 +863,7 @@ mod tests { }; #[test] - fn test_rule_order() { - let subst_env = Default::default(); - + fn test_resolved_rule_order() { use std::cmp::Ordering; fn check_total_order(rules: &[ResolvedEntityPathRule]) { @@ -763,15 +898,16 @@ mod tests { "/world/car/driver", "/x/y/z", ]; - let rules = rules.map(|rule| ResolvedEntityPathRule::parse_forgiving(rule, &subst_env)); + let rules = rules + .map(|rule| ResolvedEntityPathRule::parse_forgiving(rule, &EntityPathSubs::empty())); check_total_order(&rules); } #[test] fn test_entity_path_filter() { - let subst_env = Default::default(); + let subst_env = EntityPathSubs::empty(); - let filter = EntityPathFilter::parse( + let filter = EntityPathFilter::parse_forgiving( r#" + /world/** - /world/ @@ -798,7 +934,7 @@ mod tests { } assert_eq!( - EntityPathFilter::parse("/**") + EntityPathFilter::parse_forgiving("/**") .resolve_forgiving(&subst_env) .formatted(), "+ /**" @@ -811,7 +947,7 @@ mod tests { // We can't do in-place substitution. let subst_env = EntityPathSubs::new_with_origin(&EntityPath::from("/annoyingly/long/path")); - let filter = EntityPathFilter::parse( + let filter = EntityPathFilter::parse_forgiving( r#" + $origin/** - $origin @@ -844,7 +980,7 @@ mod tests { } assert_eq!( - EntityPathFilter::parse("/**") + EntityPathFilter::parse_forgiving("/**") .resolve_forgiving(&subst_env) .formatted(), "+ /**" @@ -853,9 +989,7 @@ mod tests { #[test] fn test_entity_path_filter_subtree() { - let subst_env = Default::default(); - - let filter = EntityPathFilter::parse( + let filter = EntityPathFilter::parse_forgiving( r#" + /world/** - /world/car/** @@ -865,7 +999,7 @@ mod tests { - /world/houses/** "#, ) - .resolve_forgiving(&subst_env); + .resolve_forgiving(&EntityPathSubs::empty()); for (path, expected) in [ ("/2D", false), @@ -891,7 +1025,7 @@ mod tests { #[test] fn test_is_superset_of() { - let subst_env = Default::default(); + let subst_env = EntityPathSubs::empty(); struct TestCase { filter: &'static str, @@ -954,10 +1088,11 @@ mod tests { ]; for case in &cases { - let filter = EntityPathFilter::parse(case.filter).resolve_forgiving(&subst_env); + let filter = + EntityPathFilter::parse_forgiving(case.filter).resolve_forgiving(&subst_env); for contains in &case.contains { let contains_filter = - EntityPathFilter::parse(contains).resolve_forgiving(&subst_env); + EntityPathFilter::parse_forgiving(contains).resolve_forgiving(&subst_env); assert!( filter.is_superset_of(&contains_filter), "Expected {:?} to fully contain {:?}, but it didn't", @@ -967,7 +1102,7 @@ mod tests { } for not_contains in &case.not_contains { let not_contains_filter = - EntityPathFilter::parse(not_contains).resolve_forgiving(&subst_env); + EntityPathFilter::parse_forgiving(not_contains).resolve_forgiving(&subst_env); assert!( !filter.is_superset_of(¬_contains_filter), "Expected {:?} to NOT fully contain {:?}, but it did", diff --git a/crates/store/re_log_types/src/path/mod.rs b/crates/store/re_log_types/src/path/mod.rs index fa394a720003..f6ba35627d1b 100644 --- a/crates/store/re_log_types/src/path/mod.rs +++ b/crates/store/re_log_types/src/path/mod.rs @@ -14,7 +14,10 @@ mod parse_path; pub use component_path::ComponentPath; pub use data_path::DataPath; pub use entity_path::{EntityPath, EntityPathHash}; -pub use entity_path_filter::{EntityPathFilter, EntityPathRule, EntityPathSubs, RuleEffect}; +pub use entity_path_filter::{ + EntityPathFilter, EntityPathFilterError, EntityPathRule, EntityPathSubs, + ResolvedEntityPathFilter, ResolvedEntityPathRule, RuleEffect, +}; pub use entity_path_part::EntityPathPart; pub use parse_path::PathParseError; diff --git a/crates/viewer/re_context_menu/src/actions/add_entities_to_new_view.rs b/crates/viewer/re_context_menu/src/actions/add_entities_to_new_view.rs index 37ffe62d9403..be8578bf45f0 100644 --- a/crates/viewer/re_context_menu/src/actions/add_entities_to_new_view.rs +++ b/crates/viewer/re_context_menu/src/actions/add_entities_to_new_view.rs @@ -144,7 +144,7 @@ fn create_view_for_selected_entities( .recommended_root_for_entities(&entities_of_interest, ctx.viewer_context.recording()) .unwrap_or_else(EntityPath::root); - let mut filter = EntityPathFilter::default(); + let mut query_filter = EntityPathFilter::default(); let target_container_id = ctx .clicked_item_enclosing_container_id_and_position() @@ -154,11 +154,14 @@ fn create_view_for_selected_entities( // relative to the origin. This makes sense since if you create a view and // then change the origin you likely wanted those entities to still be there. for path in entities_of_interest { - filter.add_rule(RuleEffect::Include, EntityPathRule::including_subtree(path)); + query_filter.add_rule( + RuleEffect::Include, + EntityPathRule::including_entity_subtree(&path), + ); } let recommended = RecommendedView { origin, - query_filter: filter, + query_filter, }; let view = ViewBlueprint::new(identifier, recommended); diff --git a/crates/viewer/re_selection_panel/src/selection_panel.rs b/crates/viewer/re_selection_panel/src/selection_panel.rs index 4bec1216ff8d..7222ccda832b 100644 --- a/crates/viewer/re_selection_panel/src/selection_panel.rs +++ b/crates/viewer/re_selection_panel/src/selection_panel.rs @@ -7,7 +7,7 @@ use re_data_ui::{ DataUi, }; use re_entity_db::{EntityPath, InstancePath}; -use re_log_types::{ComponentPath, EntityPathFilter, EntityPathSubs}; +use re_log_types::{ComponentPath, EntityPathFilter, ResolvedEntityPathFilter}; use re_types::blueprint::components::Interactive; use re_ui::{ icons, @@ -478,7 +478,7 @@ fn entity_path_filter_ui( ctx: &ViewerContext<'_>, ui: &mut egui::Ui, view_id: ViewId, - filter: &EntityPathFilter, + filter: &ResolvedEntityPathFilter, origin: &EntityPath, ) -> Option { fn syntax_highlight_entity_path_filter( @@ -559,13 +559,8 @@ fn entity_path_filter_ui( } // Apply the edit. - // - // 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 - let new_filter = - EntityPathFilter::parse_forgiving(&filter_string, &EntityPathSubs::new_with_origin(origin)); - if &new_filter == filter { + let new_filter = EntityPathFilter::parse_forgiving(&filter_string); + if new_filter == filter.unresolved() { None // no change } else { Some(new_filter) diff --git a/crates/viewer/re_selection_panel/src/view_entity_picker.rs b/crates/viewer/re_selection_panel/src/view_entity_picker.rs index d1a6b7ecb4fc..74ed964631bc 100644 --- a/crates/viewer/re_selection_panel/src/view_entity_picker.rs +++ b/crates/viewer/re_selection_panel/src/view_entity_picker.rs @@ -3,7 +3,7 @@ use nohash_hasher::IntMap; use re_data_ui::item_ui; use re_entity_db::{EntityPath, EntityTree, InstancePath}; -use re_log_types::{EntityPathFilter, EntityPathRule}; +use re_log_types::{ResolvedEntityPathFilter, ResolvedEntityPathRule}; use re_ui::{list_item, UiExt as _}; use re_viewer_context::{DataQueryResult, ViewId, ViewerContext}; use re_viewport_blueprint::{ @@ -87,7 +87,7 @@ fn add_entities_tree_ui( tree: &EntityTree, view: &ViewBlueprint, query_result: &DataQueryResult, - entity_path_filter: &EntityPathFilter, + entity_path_filter: &ResolvedEntityPathFilter, entities_add_info: &IntMap, ) { let item_content = list_item::CustomContent::new(|ui, content_ctx| { @@ -153,7 +153,7 @@ fn add_entities_line_ui( entity_tree: &EntityTree, view: &ViewBlueprint, query_result: &DataQueryResult, - entity_path_filter: &EntityPathFilter, + entity_path_filter: &ResolvedEntityPathFilter, entities_add_info: &IntMap, ) { re_tracing::profile_function!(); @@ -214,7 +214,7 @@ fn add_entities_line_ui( if response.clicked() { view.contents.raw_add_entity_exclusion( ctx, - EntityPathRule::including_subtree(entity_tree.path.clone()), + ResolvedEntityPathRule::including_subtree(&entity_tree.path), ); } @@ -231,7 +231,7 @@ fn add_entities_line_ui( if response.clicked() { view.contents.raw_add_entity_inclusion( ctx, - EntityPathRule::including_subtree(entity_tree.path.clone()), + ResolvedEntityPathRule::including_subtree(&entity_tree.path), ); } diff --git a/crates/viewer/re_viewer_context/src/view/spawn_heuristics.rs b/crates/viewer/re_viewer_context/src/view/spawn_heuristics.rs index 526470376d13..d376e4053271 100644 --- a/crates/viewer/re_viewer_context/src/view/spawn_heuristics.rs +++ b/crates/viewer/re_viewer_context/src/view/spawn_heuristics.rs @@ -1,4 +1,4 @@ -use re_log_types::{hash::Hash64, EntityPath, EntityPathFilter, EntityPathSubs}; +use re_log_types::{hash::Hash64, EntityPath, EntityPathFilter}; use re_types::ViewClassIdentifier; /// Properties of a view that as recommended to be spawned by default via view spawn heuristics. @@ -48,25 +48,19 @@ impl ViewSpawnHeuristics { impl RecommendedView { #[inline] - pub fn new<'a>(origin: EntityPath, expressions: impl IntoIterator) -> Self { - let space_env = EntityPathSubs::new_with_origin(&origin); + pub fn new_subtree(origin: EntityPath) -> Self { Self { origin, - query_filter: EntityPathFilter::from_query_expressions_forgiving( - expressions, - &space_env, - ), + query_filter: EntityPathFilter::subtree_filter("$origin"), } } - #[inline] - pub fn new_subtree(origin: EntityPath) -> Self { - Self::new(origin, std::iter::once("$origin/**")) - } - #[inline] pub fn new_single_entity(origin: EntityPath) -> Self { - Self::new(origin, std::iter::once("$origin")) + Self { + origin, + query_filter: EntityPathFilter::single_filter("$origin"), + } } #[inline] diff --git a/crates/viewer/re_viewport/src/viewport_ui.rs b/crates/viewer/re_viewport/src/viewport_ui.rs index ceba4abe229e..ced974648597 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, RuleEffect}; +use re_log_types::{EntityPath, ResolvedEntityPathRule, 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, @@ -284,7 +284,7 @@ impl ViewportUi { if can_entity_be_added(entity) { filter.add_rule( RuleEffect::Include, - EntityPathRule::including_subtree(entity.clone()), + ResolvedEntityPathRule::including_subtree(&entity), ); } } diff --git a/crates/viewer/re_viewport_blueprint/src/view.rs b/crates/viewer/re_viewport_blueprint/src/view.rs index 3177beae78da..bcd06f5d5a46 100644 --- a/crates/viewer/re_viewport_blueprint/src/view.rs +++ b/crates/viewer/re_viewport_blueprint/src/view.rs @@ -71,12 +71,16 @@ impl ViewBlueprint { pub fn new(view_class: ViewClassIdentifier, recommended: RecommendedView) -> Self { let id = ViewId::random(); + // TODO(andreas): do we need more context here? + let path_subs = EntityPathSubs::new_with_origin(&recommended.origin); + let query_filter = recommended.query_filter.resolve_forgiving(&path_subs); + Self { display_name: None, class_identifier: view_class, id, space_origin: recommended.origin, - contents: ViewContents::new(id, view_class, recommended.query_filter), + contents: ViewContents::new(id, view_class, query_filter), visible: true, defaults_path: Self::defaults_path(id), pending_writes: Default::default(), diff --git a/crates/viewer/re_viewport_blueprint/src/view_contents.rs b/crates/viewer/re_viewport_blueprint/src/view_contents.rs index 7988a7c9d65a..ef96719299b7 100644 --- a/crates/viewer/re_viewport_blueprint/src/view_contents.rs +++ b/crates/viewer/re_viewport_blueprint/src/view_contents.rs @@ -1,11 +1,10 @@ use nohash_hasher::{IntMap, IntSet}; +use re_log_types::{ResolvedEntityPathFilter, ResolvedEntityPathRule}; use slotmap::SlotMap; use smallvec::SmallVec; use re_entity_db::{external::re_chunk_store::LatestAtQuery, EntityDb, EntityTree}; -use re_log_types::{ - path::RuleEffect, EntityPath, EntityPathFilter, EntityPathRule, EntityPathSubs, Timeline, -}; +use re_log_types::{path::RuleEffect, EntityPath, EntityPathFilter, EntityPathSubs, Timeline}; use re_types::blueprint::components::VisualizerOverrides; use re_types::{ blueprint::{ @@ -26,7 +25,7 @@ use crate::{ViewBlueprint, ViewProperty}; /// Data to be added to a view, built from a [`blueprint_archetypes::ViewContents`]. /// /// During execution, it will walk an [`EntityTree`] and return a [`DataResultTree`] -/// containing any entities that match a [`EntityPathFilter`]. +/// containing any entities that match a [`ResolvedEntityPathFilter`]. /// /// Note: [`ViewContents`] doesn't implement Clone because it depends on its parent's [`ViewId`] /// used for identifying the path of its data in the blueprint store. It's ambiguous @@ -39,7 +38,7 @@ pub struct ViewContents { pub blueprint_entity_path: EntityPath, pub view_class_identifier: ViewClassIdentifier, - pub entity_path_filter: EntityPathFilter, + pub entity_path_filter: ResolvedEntityPathFilter, } impl ViewContents { @@ -76,7 +75,7 @@ impl ViewContents { pub fn new( id: ViewId, view_class_identifier: ViewClassIdentifier, - entity_path_filter: EntityPathFilter, + entity_path_filter: ResolvedEntityPathFilter, ) -> Self { // Don't use `entity_path_for_view_sub_archetype` here because this will do a search in the future, // thus needing the entity tree. @@ -97,7 +96,7 @@ impl ViewContents { blueprint_db: &EntityDb, query: &LatestAtQuery, view_class_identifier: ViewClassIdentifier, - space_env: &EntityPathSubs, + subst_env: &EntityPathSubs, ) -> Self { let property = ViewProperty::from_archetype::( blueprint_db, @@ -118,9 +117,8 @@ impl ViewContents { } }; let query = expressions.iter().map(|qe| qe.0.as_str()); - let entity_path_filter = - EntityPathFilter::from_query_expressions_forgiving(query, space_env); + EntityPathFilter::from_query_expressions(query).resolve_forgiving(subst_env); Self { blueprint_entity_path: property.blueprint_store_path, @@ -138,7 +136,9 @@ impl ViewContents { pub fn save_to_blueprint_store(&self, ctx: &ViewerContext<'_>) { ctx.save_blueprint_archetype( &self.blueprint_entity_path, - &blueprint_archetypes::ViewContents::new(self.entity_path_filter.iter_expressions()), + &blueprint_archetypes::ViewContents::new( + self.entity_path_filter.iter_unresolved_expressions(), + ), ); } @@ -150,7 +150,7 @@ impl ViewContents { ctx: &ViewerContext<'_>, new_entity_path_filter: &EntityPathFilter, ) { - if &self.entity_path_filter == new_entity_path_filter { + if &self.entity_path_filter.unresolved() == new_entity_path_filter { return; } @@ -196,11 +196,11 @@ impl ViewContents { pub fn mutate_entity_path_filter( &self, ctx: &ViewerContext<'_>, - f: impl FnOnce(&mut EntityPathFilter), + f: impl FnOnce(&mut ResolvedEntityPathFilter), ) { 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); + self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); } /// Remove a subtree and any existing rules that it would match. WARNING: a single mutation is @@ -214,7 +214,7 @@ impl ViewContents { 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); + self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); } /// Directly add an exclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is @@ -225,10 +225,10 @@ impl ViewContents { /// /// 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) { + pub fn raw_add_entity_exclusion(&self, ctx: &ViewerContext<'_>, rule: ResolvedEntityPathRule) { 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); + self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); } /// Directly add an inclusion rule to the [`EntityPathFilter`]. WARNING: a single mutation is @@ -237,10 +237,10 @@ impl ViewContents { /// 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) { + pub fn raw_add_entity_inclusion(&self, ctx: &ViewerContext<'_>, rule: ResolvedEntityPathRule) { 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); + self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); } /// Remove rules for a given entity. WARNING: a single mutation is allowed per frame, or data @@ -249,7 +249,7 @@ impl ViewContents { 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); - self.set_entity_path_filter(ctx, &new_entity_path_filter); + self.set_entity_path_filter(ctx, &new_entity_path_filter.unresolved()); } /// Build up the initial [`DataQueryResult`] for this [`ViewContents`] @@ -334,7 +334,7 @@ impl ViewContents { /// to a pure recursive evaluation. struct QueryExpressionEvaluator<'a> { visualizable_entities_for_visualizer_systems: &'a PerVisualizer, - entity_path_filter: EntityPathFilter, + entity_path_filter: ResolvedEntityPathFilter, recursive_override_base_path: EntityPath, individual_override_base_path: EntityPath, } @@ -622,7 +622,7 @@ mod tests { #[test] fn test_query_results() { - let space_env = Default::default(); + let space_env = EntityPathSubs::empty(); let mut recording = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); let blueprint = EntityDb::new(StoreId::random(re_log_types::StoreKind::Blueprint)); @@ -759,7 +759,7 @@ mod tests { let contents = ViewContents::new( view_id, "3D".into(), - EntityPathFilter::parse_forgiving(filter, &space_env), + EntityPathFilter::parse_forgiving(filter).resolve_forgiving(&space_env), ); let query_result = contents.execute_query( diff --git a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs index f26dc39cda30..e6eac31e1fda 100644 --- a/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs +++ b/crates/viewer/re_viewport_blueprint/src/viewport_blueprint.rs @@ -10,6 +10,7 @@ use ahash::HashMap; use egui_tiles::{SimplificationOptions, TileId}; use nohash_hasher::IntSet; use parking_lot::Mutex; +use re_log_types::EntityPathSubs; use smallvec::SmallVec; use re_chunk_store::LatestAtQuery; @@ -350,6 +351,17 @@ impl ViewportBlueprint { ); } + // Resolve query filters for the recommended views. + let mut recommended_views = recommended_views + .into_iter() + .map(|view| { + // Today this looks trivial and something like we could do during recommendation-creation. But in the future variable substitutions might become more complex! + let path_subs = EntityPathSubs::new_with_origin(&view.origin); + let query_filter = view.query_filter.clone().resolve_forgiving(&path_subs); + (query_filter, view) + }) + .collect::>(); + // Remove all views that have all the entities we already have on screen. let existing_path_filters = self .views @@ -357,10 +369,10 @@ impl ViewportBlueprint { .filter(|view| view.class_identifier() == class_id) .map(|view| &view.contents.entity_path_filter) .collect::>(); - recommended_views.retain(|recommended_view| { - existing_path_filters.iter().all(|existing_filter| { - !existing_filter.is_superset_of(&recommended_view.query_filter) - }) + recommended_views.retain(|(query_filter, _)| { + existing_path_filters + .iter() + .all(|existing_filter| !existing_filter.is_superset_of(&query_filter)) }); // Remove all views that are redundant within the remaining recommendation. @@ -368,16 +380,20 @@ impl ViewportBlueprint { let final_recommendations = recommended_views .iter() .enumerate() - .filter(|(j, candidate)| { - recommended_views.iter().enumerate().all(|(i, other)| { - i == *j || !other.query_filter.is_superset_of(&candidate.query_filter) - }) + .filter(|(j, (candidate_query_filter, _))| { + recommended_views + .iter() + .enumerate() + .all(|(i, (other_query_filter, _))| { + i == *j || !other_query_filter.is_superset_of(&candidate_query_filter) + }) }) .map(|(_, recommendation)| recommendation); self.add_views( - final_recommendations - .map(|recommendation| ViewBlueprint::new(class_id, recommendation.clone())), + final_recommendations.map(|(_, recommendation)| { + ViewBlueprint::new(class_id, recommendation.clone()) + }), None, None, ); diff --git a/rerun_py/src/dataframe.rs b/rerun_py/src/dataframe.rs index 3f75a459039e..383cb9dd6fc8 100644 --- a/rerun_py/src/dataframe.rs +++ b/rerun_py/src/dataframe.rs @@ -25,7 +25,7 @@ use re_chunk_store::{ }; use re_dataframe::{QueryEngine, StorageEngine}; use re_log_encoding::VersionPolicy; -use re_log_types::{EntityPathFilter, ResolvedTimeRange, TimeType}; +use re_log_types::{EntityPathFilter, EntityPathSubs, ResolvedTimeRange, TimeType}; use re_sdk::{ComponentName, EntityPath, StoreId, StoreKind}; /// Register the `rerun.dataframe` module. @@ -1161,11 +1161,13 @@ impl PyRecording { // `str` let path_filter = - EntityPathFilter::parse_strict(&expr, &Default::default()).map_err(|err| { - PyValueError::new_err(format!( - "Could not interpret `contents` as a ViewContentsLike. Failed to parse {expr}: {err}.", - )) - })?; + EntityPathFilter::parse_strict(&expr) + .and_then(|epf| epf.resolve_strict(&EntityPathSubs::empty())) + .map_err(|err| { + PyValueError::new_err(format!( + "Could not interpret `contents` as a ViewContentsLike. Failed to parse {expr}: {err}.", + )) + })?; let contents = engine .iter_entity_paths_sorted(&path_filter) @@ -1185,7 +1187,7 @@ impl PyRecording { ) })?; - let path_filter = EntityPathFilter::parse_strict(&key, &Default::default()).map_err(|err| { + let path_filter = EntityPathFilter::parse_strict(&key).and_then(|epf| epf.resolve_strict(&EntityPathSubs::empty())).map_err(|err| { PyValueError::new_err(format!( "Could not interpret `contents` as a ViewContentsLike. Failed to parse {key}: {err}.", ))