From 7c88b9c78058f68f8fd7a6ead90a6e2ac86a81a9 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 20 Dec 2023 21:54:34 +0100 Subject: [PATCH 01/12] Remove spammy debug message in space_view_heuristics --- crates/re_viewport/src/space_view_heuristics.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 93002b1ecd9f..7a0756ca5d1d 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -58,18 +58,6 @@ pub fn all_possible_space_views( ) -> Vec<(SpaceViewBlueprint, DataQueryResult)> { re_tracing::profile_function!(); - for (class_identifier, entities_per_system) in entities_per_system_per_class { - for (system_name, entities) in entities_per_system { - if entities.is_empty() { - re_log::debug!( - "SpaceViewClassRegistry: No entities for system {:?} of class {:?}", - system_name, - class_identifier - ); - } - } - } - // For each candidate, create space views for all possible classes. candidate_space_view_paths(ctx, spaces_info) .flat_map(|candidate_space_path| { From 6be9931479116b51b89b7a09116b041a291ca4cd Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 22:48:31 +0100 Subject: [PATCH 02/12] Make it possible to leave properties unset --- .../re_space_view/src/data_query_blueprint.rs | 14 +++++------ .../src/space_view_class.rs | 2 +- .../src/contexts/non_interactive_entities.rs | 2 +- .../src/contexts/transform_context.rs | 2 +- .../src/parts/cameras.rs | 2 +- .../src/parts/entity_iterator.rs | 4 ++-- .../src/parts/transform3d_arrows.rs | 4 ++-- .../src/view_part_system.rs | 6 ++--- crates/re_viewer/src/ui/selection_panel.rs | 4 ++-- .../src/space_view/view_query.rs | 13 ++++++++-- crates/re_viewport/src/space_view.rs | 24 +++++++++---------- .../re_viewport/src/viewport_blueprint_ui.rs | 2 +- 12 files changed, 44 insertions(+), 35 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 98e22070e2b6..7a6277a02de7 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -421,10 +421,10 @@ impl<'a> QueryExpressionEvaluator<'a> { Default::default() }; - let mut resolved_properties = inherited.clone(); + let mut accumulated_properties = inherited.clone(); if let Some(props) = overrides.group.get_opt(&entity_path) { - resolved_properties = resolved_properties.with_child(props); + accumulated_properties = accumulated_properties.with_child(props); } let base_entity_path = self.blueprint.id.as_entity_path().clone(); @@ -438,10 +438,10 @@ impl<'a> QueryExpressionEvaluator<'a> { let self_leaf = if !view_parts.is_empty() || exact_include { let individual_props = overrides.individual.get_opt(&entity_path); - let mut leaf_resolved_properties = resolved_properties.clone(); + let mut leaf_accumulated_properties = accumulated_properties.clone(); if let Some(props) = individual_props { - leaf_resolved_properties = leaf_resolved_properties.with_child(props); + leaf_accumulated_properties = leaf_accumulated_properties.with_child(props); } Some(data_results.insert(DataResultNode { data_result: DataResult { @@ -450,7 +450,7 @@ impl<'a> QueryExpressionEvaluator<'a> { is_group: false, direct_included: any_match, individual_properties: overrides.individual.get_opt(&entity_path).cloned(), - resolved_properties: leaf_resolved_properties, + accumulated_properties: Some(leaf_accumulated_properties), override_path: individual_override_path, }, children: Default::default(), @@ -470,7 +470,7 @@ impl<'a> QueryExpressionEvaluator<'a> { self.add_entity_tree_to_data_results_recursive( subtree, overrides, - &resolved_properties, + &accumulated_properties, data_results, recursive_include, // Once we have hit a recursive match, it's always propagated ) @@ -490,7 +490,7 @@ impl<'a> QueryExpressionEvaluator<'a> { is_group: true, direct_included: any_match, individual_properties, - resolved_properties, + accumulated_properties: Some(accumulated_properties), override_path: recursive_override_path, }, children, diff --git a/crates/re_space_view_dataframe/src/space_view_class.rs b/crates/re_space_view_dataframe/src/space_view_class.rs index 868512bb3eae..124323400d47 100644 --- a/crates/re_space_view_dataframe/src/space_view_class.rs +++ b/crates/re_space_view_dataframe/src/space_view_class.rs @@ -87,7 +87,7 @@ impl SpaceViewClass for DataframeSpaceView { // These are the entity paths whose content we must display. let sorted_entity_paths: BTreeSet<_> = query .iter_all_data_results() - .filter(|data_result| data_result.resolved_properties.visible) + .filter(|data_result| data_result.resolved_properties().visible) .map(|data_result| &data_result.entity_path) .cloned() .collect(); diff --git a/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs b/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs index f08c93dbc4d8..8175457cb5c0 100644 --- a/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs +++ b/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs @@ -28,7 +28,7 @@ impl ViewContextSystem for NonInteractiveEntities { self.0 = query .iter_all_data_results() .filter_map(|data_result| { - if data_result.resolved_properties.interactive { + if data_result.resolved_properties().interactive { None } else { Some(data_result.entity_path.hash()) diff --git a/crates/re_space_view_spatial/src/contexts/transform_context.rs b/crates/re_space_view_spatial/src/contexts/transform_context.rs index 34b81319d1b6..03cbc42d0056 100644 --- a/crates/re_space_view_spatial/src/contexts/transform_context.rs +++ b/crates/re_space_view_spatial/src/contexts/transform_context.rs @@ -99,7 +99,7 @@ impl ViewContextSystem for TransformContext { .map(|results| { results .iter() - .map(|r| (r.entity_path.clone(), r.resolved_properties.clone())) + .map(|r| (r.entity_path.clone(), r.resolved_properties().clone())) .collect() }) .unwrap_or_default(); diff --git a/crates/re_space_view_spatial/src/parts/cameras.rs b/crates/re_space_view_spatial/src/parts/cameras.rs index 22b741bbf821..474594fcfc57 100644 --- a/crates/re_space_view_spatial/src/parts/cameras.rs +++ b/crates/re_space_view_spatial/src/parts/cameras.rs @@ -216,7 +216,7 @@ impl ViewPartSystem for CamerasPart { transforms, shared_render_builders, &data_result.entity_path, - &data_result.resolved_properties, + data_result.resolved_properties(), &pinhole, store .query_latest_component::( diff --git a/crates/re_space_view_spatial/src/parts/entity_iterator.rs b/crates/re_space_view_spatial/src/parts/entity_iterator.rs index 977755e196b8..e42b88ade493 100644 --- a/crates/re_space_view_spatial/src/parts/entity_iterator.rs +++ b/crates/re_space_view_spatial/src/parts/entity_iterator.rs @@ -78,7 +78,7 @@ where ctx.store_db.store(), &query.timeline, &query.latest_at, - &data_result.resolved_properties.visible_history, + &data_result.resolved_properties().visible_history, &data_result.entity_path, ) .and_then(|entity_views| { @@ -91,7 +91,7 @@ where fun( ctx, &data_result.entity_path, - &data_result.resolved_properties, + data_result.resolved_properties(), ent_view, &entity_context, )?; diff --git a/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs b/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs index b373f8a562f1..35e220370c48 100644 --- a/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs +++ b/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs @@ -67,7 +67,7 @@ impl ViewPartSystem for Transform3DArrowsPart { continue; } - if !*data_result.resolved_properties.transform_3d_visible { + if !*data_result.resolved_properties().transform_3d_visible { continue; } @@ -91,7 +91,7 @@ impl ViewPartSystem for Transform3DArrowsPart { &mut line_builder, world_from_obj, Some(&data_result.entity_path), - *data_result.resolved_properties.transform_3d_size, + *data_result.resolved_properties().transform_3d_size, query .highlights .entity_outline_mask(data_result.entity_path.hash()) diff --git a/crates/re_space_view_time_series/src/view_part_system.rs b/crates/re_space_view_time_series/src/view_part_system.rs index 671f235c1929..ea541195e340 100644 --- a/crates/re_space_view_time_series/src/view_part_system.rs +++ b/crates/re_space_view_time_series/src/view_part_system.rs @@ -132,14 +132,14 @@ impl TimeSeriesSystem { let visible_history = match query.timeline.typ() { re_log_types::TimeType::Time => { - data_result.resolved_properties.visible_history.nanos + data_result.resolved_properties().visible_history.nanos } re_log_types::TimeType::Sequence => { - data_result.resolved_properties.visible_history.sequences + data_result.resolved_properties().visible_history.sequences } }; - let (from, to) = if data_result.resolved_properties.visible_history.enabled { + let (from, to) = if data_result.resolved_properties().visible_history.enabled { ( visible_history.from(query.latest_at), visible_history.to(query.latest_at), diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 227656fa51b3..ba36633b6ef1 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -676,7 +676,7 @@ fn blueprint_ui( &space_view_class, Some(entity_path), &mut props, - &data_result.resolved_properties, + data_result.resolved_properties(), ); data_result.save_override(Some(props), ctx); } @@ -707,7 +707,7 @@ fn blueprint_ui( &space_view_class, None, &mut props, - &data_result.resolved_properties, + data_result.resolved_properties(), ); data_result.save_override(Some(props), ctx); } diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index 737ac30e571c..8d16f3a850e4 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use itertools::Itertools; +use once_cell::sync::Lazy; use re_arrow_store::LatestAtQuery; use re_data_store::{EntityPath, EntityProperties, EntityPropertiesComponent, TimeInt, Timeline}; use re_log_types::{DataCell, DataRow, RowId, TimePoint}; @@ -40,7 +41,7 @@ pub struct DataResult { /// The resolved properties (including any hierarchical flattening) to apply. // TODO(jleibs): Eventually this goes away and becomes implicit as an override layer in the StoreView. // For now, bundling this here acts as a good proxy for that future data-override mechanism. - pub resolved_properties: EntityProperties, + pub accumulated_properties: Option, /// The individual property set in this `DataResult`, if any. pub individual_properties: Option, @@ -49,6 +50,8 @@ pub struct DataResult { pub override_path: EntityPath, } +static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); + impl DataResult { /// Write the [`EntityProperties`] for this result back to the Blueprint store. pub fn save_override(&self, props: Option, ctx: &ViewerContext<'_>) { @@ -104,6 +107,12 @@ impl DataResult { vec![row], )); } + + pub fn resolved_properties(&self) -> &EntityProperties { + self.accumulated_properties + .as_ref() + .unwrap_or(&DEFAULT_PROPS) + } } pub type PerSystemDataResults<'a> = BTreeMap>; @@ -144,7 +153,7 @@ impl<'s> ViewQuery<'s> { itertools::Either::Right( results .iter() - .filter(|result| result.resolved_properties.visible) + .filter(|result| result.resolved_properties().visible) .copied(), ) }, diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 306d135b2e4c..39cdb8fa1c01 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -403,7 +403,7 @@ impl SpaceViewBlueprint { .query_timeless_component_quiet::(&self.entity_path()) .map(|result| result.value.0); - let resolved_properties = individual_properties.clone().unwrap_or_else(|| { + let accumulated_properties = individual_properties.clone().unwrap_or_else(|| { let mut props = EntityProperties::default(); // better defaults for the time series space view // TODO(#4194, jleibs, ab): Per-space-view-class property defaults should be factored in @@ -419,7 +419,7 @@ impl SpaceViewBlueprint { view_parts: Default::default(), is_group: true, direct_included: true, - resolved_properties, + accumulated_properties: Some(accumulated_properties), individual_properties, override_path: entity_path, } @@ -555,7 +555,7 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert_eq!(result.resolved_properties, EntityProperties::default(),); + assert_eq!(result.resolved_properties(), &EntityProperties::default(),); } // Now, override visibility on parent but not group @@ -592,10 +592,10 @@ mod tests { .lookup_result_by_path_and_group(&EntityPath::from("parent/skip/child2"), false) .unwrap(); - assert!(!parent.resolved_properties.visible); + assert!(!parent.resolved_properties().visible); for result in [child1, child2] { - assert!(result.resolved_properties.visible); + assert!(result.resolved_properties().visible); } // Override visibility on parent group @@ -632,7 +632,7 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert!(!result.resolved_properties.visible); + assert!(!result.resolved_properties().visible); } } @@ -674,9 +674,9 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert!(result.resolved_properties.visible_history.enabled); + assert!(result.resolved_properties().visible_history.enabled); assert_eq!( - result.resolved_properties.visible_history.nanos, + result.resolved_properties().visible_history.nanos, VisibleHistory::ALL ); } @@ -711,16 +711,16 @@ mod tests { .unwrap(); for result in [parent, child1] { - assert!(result.resolved_properties.visible_history.enabled); + assert!(result.resolved_properties().visible_history.enabled); assert_eq!( - result.resolved_properties.visible_history.nanos, + result.resolved_properties().visible_history.nanos, VisibleHistory::ALL ); } - assert!(child2.resolved_properties.visible_history.enabled); + assert!(child2.resolved_properties().visible_history.enabled); assert_eq!( - child2.resolved_properties.visible_history.nanos, + child2.resolved_properties().visible_history.nanos, VisibleHistory::OFF ); } diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 44dffa4ab821..e50ddaf5dd58 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -222,7 +222,7 @@ impl Viewport<'_, '_> { }; let group_is_visible = - top_node.data_result.resolved_properties.visible && space_view_visible; + top_node.data_result.resolved_properties().visible && space_view_visible; // Always real children ahead of groups for child in top_node From bfba166a35c781c3ddb8ddf8ff6d0dcfbf005809 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 23:24:07 +0100 Subject: [PATCH 03/12] Add some scopes and take an optmization pass on query-execution --- crates/re_space_view/src/data_query.rs | 20 +-- .../re_space_view/src/data_query_blueprint.rs | 146 ++++++++++++++++-- crates/re_space_view/src/lib.rs | 2 +- crates/re_viewer/src/app_state.rs | 8 +- .../src/space_view/view_query.rs | 25 ++- crates/re_viewport/src/space_view.rs | 10 +- .../re_viewport/src/space_view_heuristics.rs | 9 +- 7 files changed, 168 insertions(+), 52 deletions(-) diff --git a/crates/re_space_view/src/data_query.rs b/crates/re_space_view/src/data_query.rs index 107752355439..5941b58b2a29 100644 --- a/crates/re_space_view/src/data_query.rs +++ b/crates/re_space_view/src/data_query.rs @@ -15,20 +15,6 @@ pub trait PropertyResolver { fn resolve_entity_overrides(&self, ctx: &StoreContext<'_>) -> EntityOverrides; } -pub struct NoopResolver {} - -impl PropertyResolver for NoopResolver { - fn resolve_entity_overrides(&self, _ctx: &StoreContext<'_>) -> EntityOverrides { - EntityOverrides { - root: EntityProperties::default(), - individual: EntityPropertyMap::default(), - group: EntityPropertyMap::default(), - } - } -} - -pub static NOOP_RESOLVER: NoopResolver = NoopResolver {}; - /// The common trait implemented for data queries /// /// Both interfaces return [`re_viewer_context::DataResult`]s, which are self-contained description of the data @@ -45,4 +31,10 @@ pub trait DataQuery { ctx: &StoreContext<'_>, entities_per_system: &EntitiesPerSystem, ) -> DataQueryResult; + + fn execute_query_fast( + &self, + ctx: &StoreContext<'_>, + entities_per_system: &EntitiesPerSystem, + ) -> DataQueryResult; } diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 7a6277a02de7..502d2d5c89a3 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -3,6 +3,7 @@ use re_data_store::{ EntityProperties, EntityPropertiesComponent, EntityPropertyMap, EntityTree, StoreDb, }; use re_log_types::{DataRow, EntityPath, EntityPathExpr, RowId, TimePoint}; +use re_tracing::profile_scope; use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataQueryResult, DataResult, DataResultHandle, DataResultNode, DataResultTree, @@ -51,9 +52,6 @@ impl DataQueryBlueprint { } impl DataQueryBlueprint { - pub const INDIVIDUAL_OVERRIDES_PREFIX: &'static str = "individual_overrides"; - pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; - /// Creates a new [`DataQueryBlueprint`]. /// /// This [`DataQueryBlueprint`] is ephemeral. It must be saved by calling @@ -122,11 +120,11 @@ impl DataQueryBlueprint { individual_override_root: self .id .as_entity_path() - .join(&Self::INDIVIDUAL_OVERRIDES_PREFIX.into()), + .join(&DataResult::INDIVIDUAL_OVERRIDES_PREFIX.into()), recursive_override_root: self .id .as_entity_path() - .join(&Self::RECURSIVE_OVERRIDES_PREFIX.into()), + .join(&DataResult::RECURSIVE_OVERRIDES_PREFIX.into()), } } @@ -304,6 +302,31 @@ impl DataQuery for DataQueryBlueprint { tree: DataResultTree::new(data_results, root_handle), } } + + fn execute_query_fast( + &self, + ctx: &re_viewer_context::StoreContext<'_>, + entities_per_system: &EntitiesPerSystem, + ) -> DataQueryResult { + re_tracing::profile_function!(); + + let mut data_results = SlotMap::::default(); + + let executor = QueryExpressionEvaluator::new(self, entities_per_system); + + let root_handle = ctx.recording.and_then(|store| { + executor.add_entity_tree_to_data_results_recursive_fast( + store.tree(), + &mut data_results, + false, + ) + }); + + DataQueryResult { + id: self.id, + tree: DataResultTree::new(data_results, root_handle), + } + } } /// Helper struct for executing the query from [`DataQueryBlueprint`] @@ -326,6 +349,7 @@ impl<'a> QueryExpressionEvaluator<'a> { blueprint: &'a DataQueryBlueprint, per_system_entity_list: &'a EntitiesPerSystem, ) -> Self { + profile_scope!("expression pre-processing"); let inclusions: Vec = blueprint .expressions .inclusions @@ -427,14 +451,7 @@ impl<'a> QueryExpressionEvaluator<'a> { accumulated_properties = accumulated_properties.with_child(props); } - let base_entity_path = self.blueprint.id.as_entity_path().clone(); - - let individual_override_path = base_entity_path - .join(&DataQueryBlueprint::INDIVIDUAL_OVERRIDES_PREFIX.into()) - .join(&entity_path); - let recursive_override_path = base_entity_path - .join(&DataQueryBlueprint::RECURSIVE_OVERRIDES_PREFIX.into()) - .join(&entity_path); + let base_override_path = self.blueprint.id.as_entity_path().clone(); let self_leaf = if !view_parts.is_empty() || exact_include { let individual_props = overrides.individual.get_opt(&entity_path); @@ -451,7 +468,7 @@ impl<'a> QueryExpressionEvaluator<'a> { direct_included: any_match, individual_properties: overrides.individual.get_opt(&entity_path).cloned(), accumulated_properties: Some(leaf_accumulated_properties), - override_path: individual_override_path, + base_override_path: base_override_path.clone(), }, children: Default::default(), })) @@ -491,7 +508,106 @@ impl<'a> QueryExpressionEvaluator<'a> { direct_included: any_match, individual_properties, accumulated_properties: Some(accumulated_properties), - override_path: recursive_override_path, + base_override_path, + }, + children, + })) + } + } + + fn add_entity_tree_to_data_results_recursive_fast( + &self, + tree: &EntityTree, + data_results: &mut SlotMap, + from_recursive: bool, + ) -> Option { + // If we hit a prefix that is not allowed, we terminate. This is + // a pruned branch of the tree. Can come from either an explicit + // recursive exclusion, or an implicit missing inclusion. + // TODO(jleibs): If this space is disconnected, we should terminate here + if self.recursive_exclusions.contains(&tree.path) + || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) + { + return None; + } + + let entity_path = tree.path.clone(); + + // Pre-compute our matches + let exact_include = self.exact_inclusions.contains(&entity_path); + let recursive_include = self.recursive_inclusions.contains(&entity_path) || from_recursive; + let exact_exclude = self.exact_exclusions.contains(&entity_path); + let any_match = (exact_include || recursive_include) && !exact_exclude; + + // Only populate view_parts if this is a match + // Note that allowed prefixes that aren't matches can still create groups + let view_parts: SmallVec<_> = if any_match { + profile_scope!("test_view_parts"); + self.per_system_entity_list + .iter() + .filter_map(|(part, ents)| { + if ents.contains(&entity_path) { + Some(*part) + } else { + None + } + }) + .collect() + } else { + Default::default() + }; + + let base_override_path = self.blueprint.id.as_entity_path().clone(); + + let self_leaf = if !view_parts.is_empty() || exact_include { + profile_scope!("leaf insertion"); + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path: entity_path.clone(), + view_parts, + is_group: false, + direct_included: any_match, + individual_properties: None, + accumulated_properties: None, + base_override_path: base_override_path.clone(), + }, + children: Default::default(), + })) + } else { + None + }; + + let maybe_self_iter = if let Some(self_leaf) = self_leaf { + itertools::Either::Left(std::iter::once(self_leaf)) + } else { + itertools::Either::Right(std::iter::empty()) + }; + + let children: SmallVec<_> = maybe_self_iter + .chain(tree.children.values().filter_map(|subtree| { + self.add_entity_tree_to_data_results_recursive_fast( + subtree, + data_results, + recursive_include, // Once we have hit a recursive match, it's always propagated + ) + })) + .collect(); + + // If the only child is the self-leaf, then we don't need to create a group + if children.is_empty() || children.len() == 1 && self_leaf.is_some() { + self_leaf + } else { + profile_scope!("node insertion"); + // The 'individual' properties of a group are the group overrides + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path, + view_parts: Default::default(), + is_group: true, + direct_included: any_match, + individual_properties: None, + accumulated_properties: None, + base_override_path, }, children, })) diff --git a/crates/re_space_view/src/lib.rs b/crates/re_space_view/src/lib.rs index af4d0b2efbb0..f06bd107fdc4 100644 --- a/crates/re_space_view/src/lib.rs +++ b/crates/re_space_view/src/lib.rs @@ -9,7 +9,7 @@ mod data_query_blueprint; mod screenshot; mod unreachable_transform_reason; -pub use data_query::{DataQuery, EntityOverrides, PropertyResolver, NOOP_RESOLVER}; +pub use data_query::{DataQuery, EntityOverrides, PropertyResolver}; pub use data_query_blueprint::DataQueryBlueprint; pub use screenshot::ScreenshotMode; pub use unreachable_transform_reason::UnreachableTransformReason; diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 26d5c15e3cd0..d6a0594c2dbc 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -147,18 +147,12 @@ impl AppState { .values() .flat_map(|space_view| { space_view.queries.iter().filter_map(|query| { - let props = viewport.state.space_view_props(space_view.id); - let resolver = query.build_resolver(space_view.id, props); entities_per_system_per_class .get(&query.space_view_class_identifier) .map(|entities_per_system| { ( query.id, - query.execute_query( - &resolver, - store_context, - entities_per_system, - ), + query.execute_query_fast(store_context, entities_per_system), ) }) }) diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index 8d16f3a850e4..dd52db76a951 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -47,18 +47,35 @@ pub struct DataResult { pub individual_properties: Option, /// `EntityPath` in the Blueprint store where updated overrides should be written back. - pub override_path: EntityPath, + pub base_override_path: EntityPath, } static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); impl DataResult { + pub const INDIVIDUAL_OVERRIDES_PREFIX: &'static str = "individual_overrides"; + pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; + + pub fn override_path(&self) -> EntityPath { + if self.is_group { + self.base_override_path + .join(&Self::INDIVIDUAL_OVERRIDES_PREFIX.into()) + .join(&self.entity_path) + } else { + self.base_override_path + .join(&Self::RECURSIVE_OVERRIDES_PREFIX.into()) + .join(&self.entity_path) + } + } + /// Write the [`EntityProperties`] for this result back to the Blueprint store. pub fn save_override(&self, props: Option, ctx: &ViewerContext<'_>) { + let override_path = self.override_path(); + let cell = match props { None => { if self.individual_properties.is_some() { - re_log::debug!("Clearing {:?}", self.override_path); + re_log::debug!("Clearing {:?}", override_path); Some(DataCell::from_arrow_empty( EntityPropertiesComponent::name(), @@ -77,7 +94,7 @@ impl DataResult { .as_ref() .unwrap_or(&EntityProperties::default()), ) { - re_log::debug!("Overriding {:?} with {:?}", self.override_path, props); + re_log::debug!("Overriding {:?} with {:?}", override_path, props); let component = EntityPropertiesComponent(props); @@ -94,7 +111,7 @@ impl DataResult { let row = DataRow::from_cells1_sized( RowId::new(), - self.override_path.clone(), + override_path.clone(), TimePoint::timeless(), 1, cell, diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 39cdb8fa1c01..2503a5f47757 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -421,7 +421,7 @@ impl SpaceViewBlueprint { direct_included: true, accumulated_properties: Some(accumulated_properties), individual_properties, - override_path: entity_path, + base_override_path: entity_path, } } @@ -562,7 +562,7 @@ mod tests { let mut overrides = parent.individual_properties.clone().unwrap_or_default(); overrides.visible = false; - save_override(overrides, &parent.override_path, &mut blueprint); + save_override(overrides, &parent.override_path(), &mut blueprint); } // Parent is not visible, but children are @@ -605,7 +605,7 @@ mod tests { .unwrap_or_default(); overrides.visible = false; - save_override(overrides, &parent_group.override_path, &mut blueprint); + save_override(overrides, &parent_group.override_path(), &mut blueprint); } // Nobody is visible @@ -647,7 +647,7 @@ mod tests { overrides.visible_history.enabled = true; overrides.visible_history.nanos = VisibleHistory::ALL; - save_override(overrides, &root.override_path, &mut blueprint); + save_override(overrides, &root.override_path(), &mut blueprint); } // Everyone has visible history @@ -684,7 +684,7 @@ mod tests { let mut overrides = child2.individual_properties.clone().unwrap_or_default(); overrides.visible_history.enabled = true; - save_override(overrides, &child2.override_path, &mut blueprint); + save_override(overrides, &child2.override_path(), &mut blueprint); } // Child2 has its own visible history diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 7a0756ca5d1d..360bde89741f 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -5,7 +5,7 @@ use nohash_hasher::{IntMap, IntSet}; use re_arrow_store::{LatestAtQuery, Timeline}; use re_data_store::{EntityPath, EntityTree}; use re_log_types::{EntityPathExpr, TimeInt}; -use re_space_view::{DataQuery as _, DataQueryBlueprint, NOOP_RESOLVER}; +use re_space_view::{DataQuery as _, DataQueryBlueprint}; use re_types::components::{DisconnectedSpace, TensorData}; use re_types::ComponentNameSet; use re_viewer_context::{ @@ -86,11 +86,8 @@ pub fn all_possible_space_views( std::iter::once(EntityPathExpr::Recursive(candidate_space_path.clone())), ); - let results = candidate_query.execute_query( - &NOOP_RESOLVER, - ctx.store_context, - entities_per_system, - ); + let results = + candidate_query.execute_query_fast(ctx.store_context, entities_per_system); if !results.is_empty() { Some(( From 0a0f6280980c1b2b15637fc03af05baea748a5ce Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Sat, 16 Dec 2023 16:28:06 +0100 Subject: [PATCH 04/12] More optimization --- .../re_space_view/src/data_query_blueprint.rs | 247 +++++++++++------- .../re_viewer_context/src/selection_state.rs | 2 +- 2 files changed, 156 insertions(+), 93 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 502d2d5c89a3..8c87aae5a0c9 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -310,21 +310,34 @@ impl DataQuery for DataQueryBlueprint { ) -> DataQueryResult { re_tracing::profile_function!(); - let mut data_results = SlotMap::::default(); + let mut data_results = { + profile_scope!("create slotmap"); + SlotMap::::default() + }; - let executor = QueryExpressionEvaluator::new(self, entities_per_system); + let executor = { + profile_scope!("build evaluator"); + QueryExpressionEvaluator::new(self, entities_per_system) + }; - let root_handle = ctx.recording.and_then(|store| { - executor.add_entity_tree_to_data_results_recursive_fast( - store.tree(), - &mut data_results, - false, - ) - }); + let root_handle = { + profile_scope!("run queries"); + ctx.recording.and_then(|store| { + executor.add_entity_tree_to_data_results_recursive_fast( + store.tree(), + &mut data_results, + false, + ) + }) + }; - DataQueryResult { - id: self.id, - tree: DataResultTree::new(data_results, root_handle), + { + profile_scope!("return results"); + + DataQueryResult { + id: self.id, + tree: DataResultTree::new(data_results, root_handle), + } } } } @@ -349,7 +362,6 @@ impl<'a> QueryExpressionEvaluator<'a> { blueprint: &'a DataQueryBlueprint, per_system_entity_list: &'a EntitiesPerSystem, ) -> Self { - profile_scope!("expression pre-processing"); let inclusions: Vec = blueprint .expressions .inclusions @@ -483,15 +495,24 @@ impl<'a> QueryExpressionEvaluator<'a> { }; let children: SmallVec<_> = maybe_self_iter - .chain(tree.children.values().filter_map(|subtree| { - self.add_entity_tree_to_data_results_recursive( - subtree, - overrides, - &accumulated_properties, - data_results, - recursive_include, // Once we have hit a recursive match, it's always propagated - ) - })) + .chain( + tree.children + .values() + .filter(|subtree| { + !(self.recursive_exclusions.contains(&subtree.path) + || !(recursive_include + || self.allowed_prefixes.contains(&subtree.path))) + }) + .filter_map(|subtree| { + self.add_entity_tree_to_data_results_recursive( + subtree, + overrides, + &accumulated_properties, + data_results, + recursive_include, // Once we have hit a recursive match, it's always propagated + ) + }), + ) .collect(); // If the only child is the self-leaf, then we don't need to create a group @@ -521,96 +542,138 @@ impl<'a> QueryExpressionEvaluator<'a> { data_results: &mut SlotMap, from_recursive: bool, ) -> Option { + //re_tracing::profile_function!(); // If we hit a prefix that is not allowed, we terminate. This is // a pruned branch of the tree. Can come from either an explicit // recursive exclusion, or an implicit missing inclusion. // TODO(jleibs): If this space is disconnected, we should terminate here - if self.recursive_exclusions.contains(&tree.path) - || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) { - return None; + //profile_scope!("early_return"); + if self.recursive_exclusions.contains(&tree.path) + || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) + { + return None; + } } - let entity_path = tree.path.clone(); + let entity_path = &tree.path; // Pre-compute our matches - let exact_include = self.exact_inclusions.contains(&entity_path); - let recursive_include = self.recursive_inclusions.contains(&entity_path) || from_recursive; - let exact_exclude = self.exact_exclusions.contains(&entity_path); - let any_match = (exact_include || recursive_include) && !exact_exclude; + let (exact_include, recursive_include, any_match) = { + //profile_scope!("matchers"); + + let exact_include = self.exact_inclusions.contains(entity_path); + let recursive_include = + self.recursive_inclusions.contains(entity_path) || from_recursive; + let exact_exclude = self.exact_exclusions.contains(entity_path); + let any_match = (exact_include || recursive_include) && !exact_exclude; + (exact_include, recursive_include, any_match) + }; // Only populate view_parts if this is a match // Note that allowed prefixes that aren't matches can still create groups - let view_parts: SmallVec<_> = if any_match { - profile_scope!("test_view_parts"); - self.per_system_entity_list - .iter() - .filter_map(|(part, ents)| { - if ents.contains(&entity_path) { - Some(*part) - } else { - None - } - }) - .collect() - } else { - Default::default() + let view_parts: SmallVec<_> = { + //profile_scope!("test_view_parts"); + if any_match { + self.per_system_entity_list + .iter() + .filter_map(|(part, ents)| { + if ents.contains(entity_path) { + Some(*part) + } else { + None + } + }) + .collect() + } else { + Default::default() + } }; - let base_override_path = self.blueprint.id.as_entity_path().clone(); + let self_leaf = { + //profile_scope!("leaf insertion"); + if !view_parts.is_empty() || exact_include { + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path: entity_path.clone(), + view_parts, + is_group: false, + direct_included: any_match, + individual_properties: None, + accumulated_properties: None, + base_override_path: entity_path.clone(), // This is the wrong path but it will never be written to since this is the fast query + }, + children: Default::default(), + })) + } else { + None + } + }; - let self_leaf = if !view_parts.is_empty() || exact_include { - profile_scope!("leaf insertion"); - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path: entity_path.clone(), - view_parts, - is_group: false, - direct_included: any_match, - individual_properties: None, - accumulated_properties: None, - base_override_path: base_override_path.clone(), - }, - children: Default::default(), - })) - } else { - None + let maybe_self_iter = { + //profile_scope!("maybe iter"); + if let Some(self_leaf) = self_leaf { + itertools::Either::Left(std::iter::once(self_leaf)) + } else { + itertools::Either::Right(std::iter::empty()) + } }; - let maybe_self_iter = if let Some(self_leaf) = self_leaf { - itertools::Either::Left(std::iter::once(self_leaf)) - } else { - itertools::Either::Right(std::iter::empty()) + let interesting_children: Vec<_> = { + if recursive_include { + //profile_scope!("recursive pre-filter"); + tree.children + .values() + .filter(|subtree| { + !(self.recursive_exclusions.contains(&subtree.path) + || !(recursive_include + || self.allowed_prefixes.contains(&subtree.path))) + }) + .collect() + } else { + //profile_scope!("allowed-path pre-filter"); + self.allowed_prefixes + .iter() + .filter(|prefix| prefix.is_child_of(entity_path)) + .filter_map(|prefix| prefix.last().and_then(|part| tree.children.get(part))) + .collect() + } }; - let children: SmallVec<_> = maybe_self_iter - .chain(tree.children.values().filter_map(|subtree| { - self.add_entity_tree_to_data_results_recursive_fast( - subtree, - data_results, - recursive_include, // Once we have hit a recursive match, it's always propagated - ) - })) - .collect(); + let children: SmallVec<_> = { + //profile_scope!("tail recursion"); + maybe_self_iter + .chain(interesting_children.iter().filter_map(|subtree| { + self.add_entity_tree_to_data_results_recursive_fast( + subtree, + data_results, + recursive_include, // Once we have hit a recursive match, it's always propagated + ) + })) + .collect() + }; - // If the only child is the self-leaf, then we don't need to create a group - if children.is_empty() || children.len() == 1 && self_leaf.is_some() { - self_leaf - } else { - profile_scope!("node insertion"); - // The 'individual' properties of a group are the group overrides - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path, - view_parts: Default::default(), - is_group: true, - direct_included: any_match, - individual_properties: None, - accumulated_properties: None, - base_override_path, - }, - children, - })) + { + //profile_scope!("final result insertion"); + + // If the only child is the self-leaf, then we don't need to create a group + if children.is_empty() || children.len() == 1 && self_leaf.is_some() { + self_leaf + } else { + // The 'individual' properties of a group are the group overrides + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path: entity_path.clone(), + view_parts: Default::default(), + is_group: true, + direct_included: any_match, + individual_properties: None, + accumulated_properties: None, + base_override_path: entity_path.clone(), // This is the wrong path but it will never be written to since this is the fast query, + }, + children, + })) + } } } } diff --git a/crates/re_viewer_context/src/selection_state.rs b/crates/re_viewer_context/src/selection_state.rs index 5502da850cf7..9f0cdd29e0da 100644 --- a/crates/re_viewer_context/src/selection_state.rs +++ b/crates/re_viewer_context/src/selection_state.rs @@ -117,7 +117,7 @@ pub struct SelectionState { impl SelectionState { /// Called at the start of each frame pub fn on_frame_start(&mut self, item_retain_condition: impl Fn(&Item) -> bool) { - re_tracing::profile_function!(); + re_tracing::profile_scope!("SelectionState::on_frame_start"); let history = self.history.get_mut(); history.retain(&item_retain_condition); From 899d077a939149ebcc2f63f956b1efeaa3251599 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 20 Dec 2023 23:17:06 +0100 Subject: [PATCH 05/12] Refactor PropertyOverrides as a more separable struct --- .../re_space_view/src/data_query_blueprint.rs | 34 ++++--- crates/re_viewer/src/ui/selection_panel.rs | 12 +-- crates/re_viewer_context/src/lib.rs | 6 +- .../re_viewer_context/src/space_view/mod.rs | 2 +- .../src/space_view/view_query.rs | 93 ++++++++++++------- crates/re_viewport/src/space_view.rs | 40 ++++---- .../re_viewport/src/viewport_blueprint_ui.rs | 4 +- 7 files changed, 110 insertions(+), 81 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 8c87aae5a0c9..11593261263f 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -7,8 +7,8 @@ use re_tracing::profile_scope; use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataQueryResult, DataResult, DataResultHandle, DataResultNode, DataResultTree, - EntitiesPerSystem, SpaceViewClassIdentifier, SpaceViewId, StoreContext, SystemCommand, - SystemCommandSender as _, ViewerContext, + EntitiesPerSystem, PropertyOverrides, SpaceViewClassIdentifier, SpaceViewId, StoreContext, + SystemCommand, SystemCommandSender as _, ViewerContext, }; use slotmap::SlotMap; use smallvec::SmallVec; @@ -348,13 +348,13 @@ impl DataQuery for DataQueryBlueprint { /// used to efficiently determine if we should continue the walk or switch /// to a pure recursive evaluation. struct QueryExpressionEvaluator<'a> { - blueprint: &'a DataQueryBlueprint, per_system_entity_list: &'a EntitiesPerSystem, exact_inclusions: IntSet, recursive_inclusions: IntSet, exact_exclusions: IntSet, recursive_exclusions: IntSet, allowed_prefixes: IntSet, + base_override_path: EntityPath, } impl<'a> QueryExpressionEvaluator<'a> { @@ -404,13 +404,13 @@ impl<'a> QueryExpressionEvaluator<'a> { .collect(); Self { - blueprint, per_system_entity_list, exact_inclusions, recursive_inclusions, exact_exclusions, recursive_exclusions, allowed_prefixes, + base_override_path: blueprint.id.as_entity_path().clone(), } } @@ -463,8 +463,6 @@ impl<'a> QueryExpressionEvaluator<'a> { accumulated_properties = accumulated_properties.with_child(props); } - let base_override_path = self.blueprint.id.as_entity_path().clone(); - let self_leaf = if !view_parts.is_empty() || exact_include { let individual_props = overrides.individual.get_opt(&entity_path); let mut leaf_accumulated_properties = accumulated_properties.clone(); @@ -478,9 +476,11 @@ impl<'a> QueryExpressionEvaluator<'a> { view_parts, is_group: false, direct_included: any_match, - individual_properties: overrides.individual.get_opt(&entity_path).cloned(), - accumulated_properties: Some(leaf_accumulated_properties), - base_override_path: base_override_path.clone(), + property_overrides: Some(PropertyOverrides { + individual_properties: overrides.individual.get_opt(&entity_path).cloned(), + accumulated_properties: leaf_accumulated_properties, + base_override_path: self.base_override_path.clone(), + }), }, children: Default::default(), })) @@ -527,9 +527,11 @@ impl<'a> QueryExpressionEvaluator<'a> { view_parts: Default::default(), is_group: true, direct_included: any_match, - individual_properties, - accumulated_properties: Some(accumulated_properties), - base_override_path, + property_overrides: Some(PropertyOverrides { + individual_properties, + accumulated_properties, + base_override_path: self.base_override_path.clone(), + }), }, children, })) @@ -599,9 +601,7 @@ impl<'a> QueryExpressionEvaluator<'a> { view_parts, is_group: false, direct_included: any_match, - individual_properties: None, - accumulated_properties: None, - base_override_path: entity_path.clone(), // This is the wrong path but it will never be written to since this is the fast query + property_overrides: None, }, children: Default::default(), })) @@ -667,9 +667,7 @@ impl<'a> QueryExpressionEvaluator<'a> { view_parts: Default::default(), is_group: true, direct_included: any_match, - individual_properties: None, - accumulated_properties: None, - base_override_path: entity_path.clone(), // This is the wrong path but it will never be written to since this is the fast query, + property_overrides: None, }, children, })) diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index ba36633b6ef1..7c2edaed298c 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -603,8 +603,8 @@ fn blueprint_ui( let root_data_result = space_view.root_data_result(ctx.store_context); let mut props = root_data_result - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or(resolved_entity_props.clone()); let cursor = ui.cursor(); @@ -667,8 +667,8 @@ fn blueprint_ui( .cloned() { let mut props = data_result - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or_default(); entity_props_ui( ctx, @@ -697,8 +697,8 @@ fn blueprint_ui( { let space_view_class = *space_view.class_identifier(); let mut props = data_result - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or_default(); entity_props_ui( diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index da3045fd3afb..bb34fba38cf3 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -44,9 +44,9 @@ pub use selection_state::{ pub use space_view::{ default_heuristic_filter, AutoSpawnHeuristic, DataResult, DynSpaceViewClass, HeuristicFilterContext, IdentifiedViewSystem, PerSystemDataResults, PerSystemEntities, - SpaceViewClass, SpaceViewClassIdentifier, SpaceViewClassLayoutPriority, SpaceViewClassRegistry, - SpaceViewClassRegistryError, SpaceViewEntityHighlight, SpaceViewHighlights, - SpaceViewOutlineMasks, SpaceViewState, SpaceViewSystemExecutionError, + PropertyOverrides, SpaceViewClass, SpaceViewClassIdentifier, SpaceViewClassLayoutPriority, + SpaceViewClassRegistry, SpaceViewClassRegistryError, SpaceViewEntityHighlight, + SpaceViewHighlights, SpaceViewOutlineMasks, SpaceViewState, SpaceViewSystemExecutionError, SpaceViewSystemRegistrator, SystemExecutionOutput, ViewContextCollection, ViewContextSystem, ViewPartCollection, ViewPartSystem, ViewQuery, ViewSystemIdentifier, }; diff --git a/crates/re_viewer_context/src/space_view/mod.rs b/crates/re_viewer_context/src/space_view/mod.rs index 2063e4efc492..d4a5385167c8 100644 --- a/crates/re_viewer_context/src/space_view/mod.rs +++ b/crates/re_viewer_context/src/space_view/mod.rs @@ -32,7 +32,7 @@ pub use view_context_system::{ViewContextCollection, ViewContextSystem}; pub use view_part_system::{ default_heuristic_filter, HeuristicFilterContext, ViewPartCollection, ViewPartSystem, }; -pub use view_query::{DataResult, PerSystemDataResults, ViewQuery}; +pub use view_query::{DataResult, PerSystemDataResults, PropertyOverrides, ViewQuery}; // --------------------------------------------------------------------------- diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index dd52db76a951..e415dcfe76c5 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -13,12 +13,26 @@ use crate::{ ViewSystemIdentifier, ViewerContext, }; +#[derive(Clone, Debug, PartialEq)] +pub struct PropertyOverrides { + /// The accumulated properties (including any hierarchical flattening) to apply. + // TODO(jleibs): Eventually this goes away and becomes implicit as an override layer in the StoreView. + // For now, bundling this here acts as a good proxy for that future data-override mechanism. + pub accumulated_properties: EntityProperties, + + /// The individual property set in this `DataResult`, if any. + pub individual_properties: Option, + + /// `EntityPath` in the Blueprint store where updated overrides should be written back. + pub base_override_path: EntityPath, +} + /// This is the primary mechanism through which data is passed to a `SpaceView`. /// /// It contains everything necessary to properly use this data in the context of the /// `ViewSystem`s that it is a part of. /// -/// In the future `resolved_properties` will be replaced by a `StoreView` that contains +/// In the future `accumulated_properties` will be replaced by a `StoreView` that contains /// the relevant data overrides for the given query. #[derive(Clone, Debug, PartialEq)] pub struct DataResult { @@ -38,16 +52,8 @@ pub struct DataResult { // exists due to a common prefix. pub direct_included: bool, - /// The resolved properties (including any hierarchical flattening) to apply. - // TODO(jleibs): Eventually this goes away and becomes implicit as an override layer in the StoreView. - // For now, bundling this here acts as a good proxy for that future data-override mechanism. - pub accumulated_properties: Option, - - /// The individual property set in this `DataResult`, if any. - pub individual_properties: Option, - - /// `EntityPath` in the Blueprint store where updated overrides should be written back. - pub base_override_path: EntityPath, + /// The accumulated property overrides for this `DataResult`. + pub property_overrides: Option, } static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); @@ -56,41 +62,54 @@ impl DataResult { pub const INDIVIDUAL_OVERRIDES_PREFIX: &'static str = "individual_overrides"; pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; - pub fn override_path(&self) -> EntityPath { - if self.is_group { - self.base_override_path - .join(&Self::INDIVIDUAL_OVERRIDES_PREFIX.into()) - .join(&self.entity_path) - } else { - self.base_override_path - .join(&Self::RECURSIVE_OVERRIDES_PREFIX.into()) - .join(&self.entity_path) - } + pub fn override_path(&self) -> Option { + self.property_overrides.as_ref().map(|property_overrides| { + if self.is_group { + property_overrides + .base_override_path + .join(&Self::INDIVIDUAL_OVERRIDES_PREFIX.into()) + .join(&self.entity_path) + } else { + property_overrides + .base_override_path + .join(&Self::RECURSIVE_OVERRIDES_PREFIX.into()) + .join(&self.entity_path) + } + }) } /// Write the [`EntityProperties`] for this result back to the Blueprint store. pub fn save_override(&self, props: Option, ctx: &ViewerContext<'_>) { - let override_path = self.override_path(); + // TODO(jleibs): Make it impossible for this to happen with different type structure + let Some(override_path) = self.override_path() else { + re_log::warn!( + "Tried to save override for {:?} but it has no override path", + self.entity_path + ); + return; + }; + + // This should never happen if the above didn't return early. + let Some(property_overrides) = &self.property_overrides else { + return; + }; let cell = match props { None => { - if self.individual_properties.is_some() { - re_log::debug!("Clearing {:?}", override_path); + re_log::debug!("Clearing {:?}", override_path); - Some(DataCell::from_arrow_empty( - EntityPropertiesComponent::name(), - EntityPropertiesComponent::arrow_datatype(), - )) - } else { - None - } + Some(DataCell::from_arrow_empty( + EntityPropertiesComponent::name(), + EntityPropertiesComponent::arrow_datatype(), + )) } Some(props) => { // A value of `None` in the data store means "use the default value", so if // `self.individual_properties` is `None`, we only must save if `props` is different // from the default. if props.has_edits( - self.individual_properties + property_overrides + .individual_properties .as_ref() .unwrap_or(&EntityProperties::default()), ) { @@ -126,9 +145,15 @@ impl DataResult { } pub fn resolved_properties(&self) -> &EntityProperties { - self.accumulated_properties + self.property_overrides + .as_ref() + .map_or(&DEFAULT_PROPS, |p| &p.accumulated_properties) + } + + pub fn individual_properties(&self) -> Option<&EntityProperties> { + self.property_overrides .as_ref() - .unwrap_or(&DEFAULT_PROPS) + .and_then(|p| p.individual_properties.as_ref()) } } diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 2503a5f47757..d8c8228f0d7a 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -11,9 +11,9 @@ use re_types::blueprint::components::{EntitiesDeterminedByUser, Name, SpaceViewO use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataResult, DynSpaceViewClass, PerSystemDataResults, PerSystemEntities, - SpaceViewClass, SpaceViewClassIdentifier, SpaceViewHighlights, SpaceViewId, SpaceViewState, - StoreContext, SystemCommand, SystemCommandSender as _, SystemExecutionOutput, ViewQuery, - ViewerContext, + PropertyOverrides, SpaceViewClass, SpaceViewClassIdentifier, SpaceViewHighlights, SpaceViewId, + SpaceViewState, StoreContext, SystemCommand, SystemCommandSender as _, SystemExecutionOutput, + ViewQuery, ViewerContext, }; use crate::system_execution::create_and_run_space_view_systems; @@ -372,8 +372,8 @@ impl SpaceViewBlueprint { let root_data_result = self.root_data_result(ctx.store_context); let props = root_data_result - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or_default(); ui.scope(|ui| { @@ -419,9 +419,11 @@ impl SpaceViewBlueprint { view_parts: Default::default(), is_group: true, direct_included: true, - accumulated_properties: Some(accumulated_properties), - individual_properties, - base_override_path: entity_path, + property_overrides: Some(PropertyOverrides { + accumulated_properties, + individual_properties, + base_override_path: entity_path, + }), } } @@ -559,10 +561,10 @@ mod tests { } // Now, override visibility on parent but not group - let mut overrides = parent.individual_properties.clone().unwrap_or_default(); + let mut overrides = parent.individual_properties().cloned().unwrap_or_default(); overrides.visible = false; - save_override(overrides, &parent.override_path(), &mut blueprint); + save_override(overrides, &parent.override_path().unwrap(), &mut blueprint); } // Parent is not visible, but children are @@ -600,12 +602,16 @@ mod tests { // Override visibility on parent group let mut overrides = parent_group - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or_default(); overrides.visible = false; - save_override(overrides, &parent_group.override_path(), &mut blueprint); + save_override( + overrides, + &parent_group.override_path().unwrap(), + &mut blueprint, + ); } // Nobody is visible @@ -643,11 +649,11 @@ mod tests { recording: Some(&recording), all_recordings: vec![], }); - let mut overrides = root.individual_properties.clone().unwrap_or_default(); + let mut overrides = root.individual_properties().cloned().unwrap_or_default(); overrides.visible_history.enabled = true; overrides.visible_history.nanos = VisibleHistory::ALL; - save_override(overrides, &root.override_path(), &mut blueprint); + save_override(overrides, &root.override_path().unwrap(), &mut blueprint); } // Everyone has visible history @@ -681,10 +687,10 @@ mod tests { ); } - let mut overrides = child2.individual_properties.clone().unwrap_or_default(); + let mut overrides = child2.individual_properties().cloned().unwrap_or_default(); overrides.visible_history.enabled = true; - save_override(overrides, &child2.override_path(), &mut blueprint); + save_override(overrides, &child2.override_path().unwrap(), &mut blueprint); } // Child2 has its own visible history diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index e50ddaf5dd58..16f717ab9cb1 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -265,8 +265,8 @@ impl Viewport<'_, '_> { ctx.selection_state().highlight_for_ui_element(&item) == HoverHighlight::Hovered; let mut properties = data_result - .individual_properties - .clone() + .individual_properties() + .cloned() .unwrap_or_default(); let name = entity_path From 54530cead2140b4be484838180c2b8dc3604a140 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 20 Dec 2023 23:19:04 +0100 Subject: [PATCH 06/12] Rename resolved -> accumulated --- .../src/space_view_class.rs | 2 +- .../src/contexts/non_interactive_entities.rs | 2 +- .../src/contexts/transform_context.rs | 2 +- .../src/parts/cameras.rs | 2 +- .../src/parts/entity_iterator.rs | 4 ++-- .../src/parts/transform3d_arrows.rs | 4 ++-- .../src/view_part_system.rs | 9 +++++--- crates/re_viewer/src/ui/selection_panel.rs | 4 ++-- .../src/space_view/view_query.rs | 7 ++++-- crates/re_viewport/src/space_view.rs | 23 +++++++++++-------- .../re_viewport/src/viewport_blueprint_ui.rs | 2 +- 11 files changed, 35 insertions(+), 26 deletions(-) diff --git a/crates/re_space_view_dataframe/src/space_view_class.rs b/crates/re_space_view_dataframe/src/space_view_class.rs index 124323400d47..2aafa002ef41 100644 --- a/crates/re_space_view_dataframe/src/space_view_class.rs +++ b/crates/re_space_view_dataframe/src/space_view_class.rs @@ -87,7 +87,7 @@ impl SpaceViewClass for DataframeSpaceView { // These are the entity paths whose content we must display. let sorted_entity_paths: BTreeSet<_> = query .iter_all_data_results() - .filter(|data_result| data_result.resolved_properties().visible) + .filter(|data_result| data_result.accumulated_properties().visible) .map(|data_result| &data_result.entity_path) .cloned() .collect(); diff --git a/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs b/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs index 8175457cb5c0..d1842b1a99e6 100644 --- a/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs +++ b/crates/re_space_view_spatial/src/contexts/non_interactive_entities.rs @@ -28,7 +28,7 @@ impl ViewContextSystem for NonInteractiveEntities { self.0 = query .iter_all_data_results() .filter_map(|data_result| { - if data_result.resolved_properties().interactive { + if data_result.accumulated_properties().interactive { None } else { Some(data_result.entity_path.hash()) diff --git a/crates/re_space_view_spatial/src/contexts/transform_context.rs b/crates/re_space_view_spatial/src/contexts/transform_context.rs index 03cbc42d0056..0a9213129da5 100644 --- a/crates/re_space_view_spatial/src/contexts/transform_context.rs +++ b/crates/re_space_view_spatial/src/contexts/transform_context.rs @@ -99,7 +99,7 @@ impl ViewContextSystem for TransformContext { .map(|results| { results .iter() - .map(|r| (r.entity_path.clone(), r.resolved_properties().clone())) + .map(|r| (r.entity_path.clone(), r.accumulated_properties().clone())) .collect() }) .unwrap_or_default(); diff --git a/crates/re_space_view_spatial/src/parts/cameras.rs b/crates/re_space_view_spatial/src/parts/cameras.rs index 474594fcfc57..9cc726bffb53 100644 --- a/crates/re_space_view_spatial/src/parts/cameras.rs +++ b/crates/re_space_view_spatial/src/parts/cameras.rs @@ -216,7 +216,7 @@ impl ViewPartSystem for CamerasPart { transforms, shared_render_builders, &data_result.entity_path, - data_result.resolved_properties(), + data_result.accumulated_properties(), &pinhole, store .query_latest_component::( diff --git a/crates/re_space_view_spatial/src/parts/entity_iterator.rs b/crates/re_space_view_spatial/src/parts/entity_iterator.rs index e42b88ade493..027cf4d584bf 100644 --- a/crates/re_space_view_spatial/src/parts/entity_iterator.rs +++ b/crates/re_space_view_spatial/src/parts/entity_iterator.rs @@ -78,7 +78,7 @@ where ctx.store_db.store(), &query.timeline, &query.latest_at, - &data_result.resolved_properties().visible_history, + &data_result.accumulated_properties().visible_history, &data_result.entity_path, ) .and_then(|entity_views| { @@ -91,7 +91,7 @@ where fun( ctx, &data_result.entity_path, - data_result.resolved_properties(), + data_result.accumulated_properties(), ent_view, &entity_context, )?; diff --git a/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs b/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs index 35e220370c48..31db5a5e3eef 100644 --- a/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs +++ b/crates/re_space_view_spatial/src/parts/transform3d_arrows.rs @@ -67,7 +67,7 @@ impl ViewPartSystem for Transform3DArrowsPart { continue; } - if !*data_result.resolved_properties().transform_3d_visible { + if !*data_result.accumulated_properties().transform_3d_visible { continue; } @@ -91,7 +91,7 @@ impl ViewPartSystem for Transform3DArrowsPart { &mut line_builder, world_from_obj, Some(&data_result.entity_path), - *data_result.resolved_properties().transform_3d_size, + *data_result.accumulated_properties().transform_3d_size, query .highlights .entity_outline_mask(data_result.entity_path.hash()) diff --git a/crates/re_space_view_time_series/src/view_part_system.rs b/crates/re_space_view_time_series/src/view_part_system.rs index ea541195e340..0d1fd78d1492 100644 --- a/crates/re_space_view_time_series/src/view_part_system.rs +++ b/crates/re_space_view_time_series/src/view_part_system.rs @@ -132,14 +132,17 @@ impl TimeSeriesSystem { let visible_history = match query.timeline.typ() { re_log_types::TimeType::Time => { - data_result.resolved_properties().visible_history.nanos + data_result.accumulated_properties().visible_history.nanos } re_log_types::TimeType::Sequence => { - data_result.resolved_properties().visible_history.sequences + data_result + .accumulated_properties() + .visible_history + .sequences } }; - let (from, to) = if data_result.resolved_properties().visible_history.enabled { + let (from, to) = if data_result.accumulated_properties().visible_history.enabled { ( visible_history.from(query.latest_at), visible_history.to(query.latest_at), diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 7c2edaed298c..5479ddc7e021 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -676,7 +676,7 @@ fn blueprint_ui( &space_view_class, Some(entity_path), &mut props, - data_result.resolved_properties(), + data_result.accumulated_properties(), ); data_result.save_override(Some(props), ctx); } @@ -707,7 +707,7 @@ fn blueprint_ui( &space_view_class, None, &mut props, - data_result.resolved_properties(), + data_result.accumulated_properties(), ); data_result.save_override(Some(props), ctx); } diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index e415dcfe76c5..f0166e77a057 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -62,6 +62,7 @@ impl DataResult { pub const INDIVIDUAL_OVERRIDES_PREFIX: &'static str = "individual_overrides"; pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; + #[inline] pub fn override_path(&self) -> Option { self.property_overrides.as_ref().map(|property_overrides| { if self.is_group { @@ -144,12 +145,14 @@ impl DataResult { )); } - pub fn resolved_properties(&self) -> &EntityProperties { + #[inline] + pub fn accumulated_properties(&self) -> &EntityProperties { self.property_overrides .as_ref() .map_or(&DEFAULT_PROPS, |p| &p.accumulated_properties) } + #[inline] pub fn individual_properties(&self) -> Option<&EntityProperties> { self.property_overrides .as_ref() @@ -195,7 +198,7 @@ impl<'s> ViewQuery<'s> { itertools::Either::Right( results .iter() - .filter(|result| result.resolved_properties().visible) + .filter(|result| result.accumulated_properties().visible) .copied(), ) }, diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index d8c8228f0d7a..a30e1a0065ef 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -557,7 +557,10 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert_eq!(result.resolved_properties(), &EntityProperties::default(),); + assert_eq!( + result.accumulated_properties(), + &EntityProperties::default(), + ); } // Now, override visibility on parent but not group @@ -594,10 +597,10 @@ mod tests { .lookup_result_by_path_and_group(&EntityPath::from("parent/skip/child2"), false) .unwrap(); - assert!(!parent.resolved_properties().visible); + assert!(!parent.accumulated_properties().visible); for result in [child1, child2] { - assert!(result.resolved_properties().visible); + assert!(result.accumulated_properties().visible); } // Override visibility on parent group @@ -638,7 +641,7 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert!(!result.resolved_properties().visible); + assert!(!result.accumulated_properties().visible); } } @@ -680,9 +683,9 @@ mod tests { .unwrap(); for result in [parent, child1, child2] { - assert!(result.resolved_properties().visible_history.enabled); + assert!(result.accumulated_properties().visible_history.enabled); assert_eq!( - result.resolved_properties().visible_history.nanos, + result.accumulated_properties().visible_history.nanos, VisibleHistory::ALL ); } @@ -717,16 +720,16 @@ mod tests { .unwrap(); for result in [parent, child1] { - assert!(result.resolved_properties().visible_history.enabled); + assert!(result.accumulated_properties().visible_history.enabled); assert_eq!( - result.resolved_properties().visible_history.nanos, + result.accumulated_properties().visible_history.nanos, VisibleHistory::ALL ); } - assert!(child2.resolved_properties().visible_history.enabled); + assert!(child2.accumulated_properties().visible_history.enabled); assert_eq!( - child2.resolved_properties().visible_history.nanos, + child2.accumulated_properties().visible_history.nanos, VisibleHistory::OFF ); } diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 16f717ab9cb1..43e742533ecb 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -222,7 +222,7 @@ impl Viewport<'_, '_> { }; let group_is_visible = - top_node.data_result.resolved_properties().visible && space_view_visible; + top_node.data_result.accumulated_properties().visible && space_view_visible; // Always real children ahead of groups for child in top_node From a2e9ce3e73e2267db7470109ec59ebd56024bea1 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 20 Dec 2023 23:22:00 +0100 Subject: [PATCH 07/12] Warn on inappropriate usage --- .../src/space_view/view_query.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index f0166e77a057..7a1c363b5a4f 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -82,6 +82,8 @@ impl DataResult { /// Write the [`EntityProperties`] for this result back to the Blueprint store. pub fn save_override(&self, props: Option, ctx: &ViewerContext<'_>) { // TODO(jleibs): Make it impossible for this to happen with different type structure + // This should never happen unless we're doing something with a partially processed + // query. let Some(override_path) = self.override_path() else { re_log::warn!( "Tried to save override for {:?} but it has no override path", @@ -147,9 +149,18 @@ impl DataResult { #[inline] pub fn accumulated_properties(&self) -> &EntityProperties { - self.property_overrides - .as_ref() - .map_or(&DEFAULT_PROPS, |p| &p.accumulated_properties) + // TODO(jleibs): Make it impossible for this to happen with different type structure + // This should never happen unless we're doing something with a partially processed + // query. + let Some(property_overrides) = &self.property_overrides else { + re_log::warn!( + "Tried to get accumulated properties for {:?} but it has no property overrides", + self.entity_path + ); + return &DEFAULT_PROPS; + }; + + &property_overrides.accumulated_properties } #[inline] From 54d356361744b1a0fa7ac9d6b3f20d4986fa1fde Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 21 Dec 2023 02:09:51 +0100 Subject: [PATCH 08/12] Do property-resolving separately from the query --- crates/re_space_view/src/data_query.rs | 9 +- .../re_space_view/src/data_query_blueprint.rs | 320 ++++++------------ crates/re_viewer/src/app_state.rs | 59 ++-- crates/re_viewer_context/src/query_context.rs | 5 + .../src/space_view/view_query.rs | 18 +- crates/re_viewport/src/space_view.rs | 27 +- .../re_viewport/src/space_view_heuristics.rs | 2 +- 7 files changed, 166 insertions(+), 274 deletions(-) diff --git a/crates/re_space_view/src/data_query.rs b/crates/re_space_view/src/data_query.rs index 5941b58b2a29..3f52a0639be0 100644 --- a/crates/re_space_view/src/data_query.rs +++ b/crates/re_space_view/src/data_query.rs @@ -12,7 +12,7 @@ pub struct EntityOverrides { /// The `SpaceViewBlueprint` is the only thing that likely implements this today /// but we use a trait here so we don't have to pick up a full dependency on `re_viewport`. pub trait PropertyResolver { - fn resolve_entity_overrides(&self, ctx: &StoreContext<'_>) -> EntityOverrides; + fn update_overrides(&self, ctx: &StoreContext<'_>, query_result: &mut DataQueryResult); } /// The common trait implemented for data queries @@ -26,13 +26,6 @@ pub trait DataQuery { /// /// This is used when building up the contents for a `SpaceView`. fn execute_query( - &self, - property_resolver: &impl PropertyResolver, - ctx: &StoreContext<'_>, - entities_per_system: &EntitiesPerSystem, - ) -> DataQueryResult; - - fn execute_query_fast( &self, ctx: &StoreContext<'_>, entities_per_system: &EntitiesPerSystem, diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 11593261263f..dc7a63387384 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -114,17 +114,16 @@ impl DataQueryBlueprint { container: SpaceViewId, auto_properties: &'a EntityPropertyMap, ) -> DataQueryPropertyResolver<'a> { + let base_override_root = self.id.as_entity_path().clone(); + let individual_override_root = + base_override_root.join(&DataResult::INDIVIDUAL_OVERRIDES_PREFIX.into()); + let recursive_override_root = + base_override_root.join(&DataResult::RECURSIVE_OVERRIDES_PREFIX.into()); DataQueryPropertyResolver { auto_properties, default_stack: vec![container.as_entity_path(), self.id.as_entity_path()], - individual_override_root: self - .id - .as_entity_path() - .join(&DataResult::INDIVIDUAL_OVERRIDES_PREFIX.into()), - recursive_override_root: self - .id - .as_entity_path() - .join(&DataResult::RECURSIVE_OVERRIDES_PREFIX.into()), + individual_override_root, + recursive_override_root, } } @@ -274,36 +273,6 @@ impl DataQueryBlueprint { impl DataQuery for DataQueryBlueprint { fn execute_query( - &self, - property_resolver: &impl PropertyResolver, - ctx: &re_viewer_context::StoreContext<'_>, - entities_per_system: &EntitiesPerSystem, - ) -> DataQueryResult { - re_tracing::profile_function!(); - - let mut data_results = SlotMap::::default(); - - let overrides = property_resolver.resolve_entity_overrides(ctx); - - let executor = QueryExpressionEvaluator::new(self, entities_per_system); - - let root_handle = ctx.recording.and_then(|store| { - executor.add_entity_tree_to_data_results_recursive( - store.tree(), - &overrides, - &overrides.root, - &mut data_results, - false, - ) - }); - - DataQueryResult { - id: self.id, - tree: DataResultTree::new(data_results, root_handle), - } - } - - fn execute_query_fast( &self, ctx: &re_viewer_context::StoreContext<'_>, entities_per_system: &EntitiesPerSystem, @@ -354,7 +323,6 @@ struct QueryExpressionEvaluator<'a> { exact_exclusions: IntSet, recursive_exclusions: IntSet, allowed_prefixes: IntSet, - base_override_path: EntityPath, } impl<'a> QueryExpressionEvaluator<'a> { @@ -410,131 +378,6 @@ impl<'a> QueryExpressionEvaluator<'a> { exact_exclusions, recursive_exclusions, allowed_prefixes, - base_override_path: blueprint.id.as_entity_path().clone(), - } - } - - fn add_entity_tree_to_data_results_recursive( - &self, - tree: &EntityTree, - overrides: &EntityOverrides, - inherited: &EntityProperties, - data_results: &mut SlotMap, - from_recursive: bool, - ) -> Option { - // If we hit a prefix that is not allowed, we terminate. This is - // a pruned branch of the tree. Can come from either an explicit - // recursive exclusion, or an implicit missing inclusion. - // TODO(jleibs): If this space is disconnected, we should terminate here - if self.recursive_exclusions.contains(&tree.path) - || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) - { - return None; - } - - let entity_path = tree.path.clone(); - - // Pre-compute our matches - let exact_include = self.exact_inclusions.contains(&entity_path); - let recursive_include = self.recursive_inclusions.contains(&entity_path) || from_recursive; - let exact_exclude = self.exact_exclusions.contains(&entity_path); - let any_match = (exact_include || recursive_include) && !exact_exclude; - - // Only populate view_parts if this is a match - // Note that allowed prefixes that aren't matches can still create groups - let view_parts: SmallVec<_> = if any_match { - self.per_system_entity_list - .iter() - .filter_map(|(part, ents)| { - if ents.contains(&entity_path) { - Some(*part) - } else { - None - } - }) - .collect() - } else { - Default::default() - }; - - let mut accumulated_properties = inherited.clone(); - - if let Some(props) = overrides.group.get_opt(&entity_path) { - accumulated_properties = accumulated_properties.with_child(props); - } - - let self_leaf = if !view_parts.is_empty() || exact_include { - let individual_props = overrides.individual.get_opt(&entity_path); - let mut leaf_accumulated_properties = accumulated_properties.clone(); - - if let Some(props) = individual_props { - leaf_accumulated_properties = leaf_accumulated_properties.with_child(props); - } - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path: entity_path.clone(), - view_parts, - is_group: false, - direct_included: any_match, - property_overrides: Some(PropertyOverrides { - individual_properties: overrides.individual.get_opt(&entity_path).cloned(), - accumulated_properties: leaf_accumulated_properties, - base_override_path: self.base_override_path.clone(), - }), - }, - children: Default::default(), - })) - } else { - None - }; - - let maybe_self_iter = if let Some(self_leaf) = self_leaf { - itertools::Either::Left(std::iter::once(self_leaf)) - } else { - itertools::Either::Right(std::iter::empty()) - }; - - let children: SmallVec<_> = maybe_self_iter - .chain( - tree.children - .values() - .filter(|subtree| { - !(self.recursive_exclusions.contains(&subtree.path) - || !(recursive_include - || self.allowed_prefixes.contains(&subtree.path))) - }) - .filter_map(|subtree| { - self.add_entity_tree_to_data_results_recursive( - subtree, - overrides, - &accumulated_properties, - data_results, - recursive_include, // Once we have hit a recursive match, it's always propagated - ) - }), - ) - .collect(); - - // If the only child is the self-leaf, then we don't need to create a group - if children.is_empty() || children.len() == 1 && self_leaf.is_some() { - self_leaf - } else { - // The 'individual' properties of a group are the group overrides - let individual_properties = overrides.group.get_opt(&entity_path).cloned(); - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path, - view_parts: Default::default(), - is_group: true, - direct_included: any_match, - property_overrides: Some(PropertyOverrides { - individual_properties, - accumulated_properties, - base_override_path: self.base_override_path.clone(), - }), - }, - children, - })) } } @@ -684,33 +527,6 @@ pub struct DataQueryPropertyResolver<'a> { } impl DataQueryPropertyResolver<'_> { - fn resolve_entity_overrides_for_path( - &self, - ctx: &StoreContext<'_>, - props_path: &EntityPath, - ) -> EntityPropertyMap { - re_tracing::profile_function!(); - let blueprint = ctx.blueprint; - - let mut prop_map = self.auto_properties.clone(); - - if let Some(tree) = blueprint.tree().subtree(props_path) { - tree.visit_children_recursively(&mut |path: &EntityPath| { - if let Some(props) = blueprint - .store() - .query_timeless_component_quiet::(path) - { - let overridden_path = - EntityPath::from(&path.as_slice()[props_path.len()..path.len()]); - prop_map.update(overridden_path, props.value.0); - } - }); - } - prop_map - } -} - -impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> { /// Helper function to lookup the properties for a given entity path. /// /// We start with the auto properties for the `SpaceView` as the base layer and @@ -752,6 +568,109 @@ impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> { group: self.resolve_entity_overrides_for_path(ctx, &self.recursive_override_root), } } + + fn resolve_entity_overrides_for_path( + &self, + ctx: &StoreContext<'_>, + props_path: &EntityPath, + ) -> EntityPropertyMap { + re_tracing::profile_function!(); + let blueprint = ctx.blueprint; + + let mut prop_map = self.auto_properties.clone(); + + if let Some(tree) = blueprint.tree().subtree(props_path) { + tree.visit_children_recursively(&mut |path: &EntityPath| { + if let Some(props) = blueprint + .store() + .query_timeless_component_quiet::(path) + { + let overridden_path = + EntityPath::from(&path.as_slice()[props_path.len()..path.len()]); + prop_map.update(overridden_path, props.value.0); + } + }); + } + prop_map + } + + fn update_overrides_recursive( + &self, + query_result: &mut DataQueryResult, + entity_overrides: &EntityOverrides, + accumulated: &EntityProperties, + handle: DataResultHandle, + ) { + if let Some((child_handles, accumulated)) = + query_result.tree.lookup_node_mut(handle).and_then(|node| { + if node.data_result.is_group { + let overridden_properties = entity_overrides + .group + .get_opt(&node.data_result.entity_path); + + let accumulated_properties = if let Some(overridden) = overridden_properties { + accumulated.with_child(overridden) + } else { + accumulated.clone() + }; + + node.data_result.property_overrides = Some(PropertyOverrides { + individual_properties: overridden_properties.cloned(), + accumulated_properties: accumulated_properties.clone(), + override_path: self + .recursive_override_root + .join(&node.data_result.entity_path), + }); + + Some((node.children.clone(), accumulated_properties)) + } else { + let overridden_properties = entity_overrides + .individual + .get_opt(&node.data_result.entity_path); + + let accumulated_properties = if let Some(overidden) = overridden_properties { + accumulated.with_child(overidden) + } else { + accumulated.clone() + }; + + node.data_result.property_overrides = Some(PropertyOverrides { + individual_properties: overridden_properties.cloned(), + accumulated_properties: accumulated_properties.clone(), + override_path: self + .individual_override_root + .join(&node.data_result.entity_path), + }); + + None + } + }) + { + for child in child_handles { + self.update_overrides_recursive( + query_result, + entity_overrides, + &accumulated, + child, + ); + } + } + } +} + +impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> { + fn update_overrides(&self, ctx: &StoreContext<'_>, query_result: &mut DataQueryResult) { + let entity_overrides = self.resolve_entity_overrides(ctx); + + if let Some(root) = query_result.tree.root_handle() { + self.update_overrides_recursive( + query_result, + &entity_overrides, + &entity_overrides.root, + root, + ); + } + } } #[cfg(feature = "testing")] @@ -763,28 +682,11 @@ mod tests { use super::*; - struct StaticPropertyResolver { - overrides: EntityPropertyMap, - } - - impl PropertyResolver for StaticPropertyResolver { - fn resolve_entity_overrides(&self, _ctx: &StoreContext<'_>) -> EntityOverrides { - EntityOverrides { - root: Default::default(), - individual: self.overrides.clone(), - group: self.overrides.clone(), - } - } - } - #[test] fn test_query_results() { let mut recording = StoreDb::new(StoreId::random(re_log_types::StoreKind::Recording)); let blueprint = StoreDb::new(StoreId::random(re_log_types::StoreKind::Blueprint)); - let overrides = EntityPropertyMap::default(); - let resolver = StaticPropertyResolver { overrides }; - let timeline_frame = Timeline::new_sequence("frame"); let timepoint = TimePoint::from_iter([(timeline_frame, 10.into())]); @@ -927,7 +829,7 @@ mod tests { ), }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let query_result = query.execute_query(&ctx, &entities_per_system); let mut visited = vec![]; query_result.tree.visit(&mut |handle| { diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index d6a0594c2dbc..f0c491b10848 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -3,7 +3,7 @@ use ahash::HashMap; use re_data_store::StoreDb; use re_log_types::{LogMsg, StoreId, TimeRangeF}; use re_smart_channel::ReceiveSet; -use re_space_view::DataQuery as _; +use re_space_view::{DataQuery as _, PropertyResolver as _}; use re_viewer_context::{ AppOptions, Caches, CommandSender, ComponentUiRegistry, PlayState, RecordingConfig, SelectionState, SpaceViewClassRegistry, StoreContext, SystemCommandSender as _, ViewerContext, @@ -139,7 +139,7 @@ impl AppState { ); // Execute the queries for every `SpaceView` - let query_results = { + let mut query_results = { re_tracing::profile_scope!("query_results"); viewport .blueprint @@ -152,7 +152,7 @@ impl AppState { .map(|entities_per_system| { ( query.id, - query.execute_query_fast(store_context, entities_per_system), + query.execute_query(store_context, entities_per_system), ) }) }) @@ -160,7 +160,7 @@ impl AppState { .collect::<_>() }; - let mut ctx = ViewerContext { + let ctx = ViewerContext { app_options, cache, space_view_class_registry, @@ -182,36 +182,35 @@ impl AppState { viewport.on_frame_start(&ctx, &spaces_info); - // TODO(jleibs): Running the queries a second time is annoying, but we need - // to do this or else the auto_properties aren't right since they get populated - // in on_frame_start, but on_frame_start also needs the queries. - let updated_query_results = { + { re_tracing::profile_scope!("updated_query_results"); - viewport - .blueprint - .space_views - .values() - .flat_map(|space_view| { - space_view.queries.iter().filter_map(|query| { + for space_view in viewport.blueprint.space_views.values() { + for query in &space_view.queries { + if let Some(query_result) = query_results.get_mut(&query.id) { let props = viewport.state.space_view_props(space_view.id); let resolver = query.build_resolver(space_view.id, props); - entities_per_system_per_class - .get(&query.space_view_class_identifier) - .map(|entities_per_system| { - ( - query.id, - query.execute_query( - &resolver, - store_context, - entities_per_system, - ), - ) - }) - }) - }) - .collect::<_>() + resolver.update_overrides(store_context, query_result); + } + } + } + }; + + // TODO(jleibs): The need to rebuild this after updating the queries is kind of annoying, + // but it's just a bunch of refs so not really that big of a deal in practice. + let ctx = ViewerContext { + app_options, + cache, + space_view_class_registry, + component_ui_registry, + store_db, + store_context, + entities_per_system_per_class: &entities_per_system_per_class, + query_results: &query_results, + rec_cfg, + re_ui, + render_ctx, + command_sender, }; - ctx.query_results = &updated_query_results; time_panel.show_panel(&ctx, ui, app_blueprint.time_panel_expanded); selection_panel.show_panel( diff --git a/crates/re_viewer_context/src/query_context.rs b/crates/re_viewer_context/src/query_context.rs index 81180e51ec91..4caba9946e4b 100644 --- a/crates/re_viewer_context/src/query_context.rs +++ b/crates/re_viewer_context/src/query_context.rs @@ -136,6 +136,11 @@ impl DataResultTree { self.data_results.get(handle) } + /// Look up a [`DataResultNode`] in the tree based on its handle. + pub fn lookup_node_mut(&mut self, handle: DataResultHandle) -> Option<&mut DataResultNode> { + self.data_results.get_mut(handle) + } + /// Look up a [`DataResultNode`] in the tree based on an [`EntityPath`]. pub fn lookup_result_by_path_and_group( &self, diff --git a/crates/re_viewer_context/src/space_view/view_query.rs b/crates/re_viewer_context/src/space_view/view_query.rs index 7a1c363b5a4f..5e787aabf281 100644 --- a/crates/re_viewer_context/src/space_view/view_query.rs +++ b/crates/re_viewer_context/src/space_view/view_query.rs @@ -24,7 +24,7 @@ pub struct PropertyOverrides { pub individual_properties: Option, /// `EntityPath` in the Blueprint store where updated overrides should be written back. - pub base_override_path: EntityPath, + pub override_path: EntityPath, } /// This is the primary mechanism through which data is passed to a `SpaceView`. @@ -63,20 +63,8 @@ impl DataResult { pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; #[inline] - pub fn override_path(&self) -> Option { - self.property_overrides.as_ref().map(|property_overrides| { - if self.is_group { - property_overrides - .base_override_path - .join(&Self::INDIVIDUAL_OVERRIDES_PREFIX.into()) - .join(&self.entity_path) - } else { - property_overrides - .base_override_path - .join(&Self::RECURSIVE_OVERRIDES_PREFIX.into()) - .join(&self.entity_path) - } - }) + pub fn override_path(&self) -> Option<&EntityPath> { + self.property_overrides.as_ref().map(|p| &p.override_path) } /// Write the [`EntityProperties`] for this result back to the Blueprint store. diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index a30e1a0065ef..fa71a3319a41 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -422,7 +422,7 @@ impl SpaceViewBlueprint { property_overrides: Some(PropertyOverrides { accumulated_properties, individual_properties, - base_override_path: entity_path, + override_path: entity_path, }), } } @@ -463,7 +463,7 @@ impl SpaceViewBlueprint { mod tests { use re_data_store::StoreDb; use re_log_types::{DataCell, DataRow, RowId, StoreId, TimePoint}; - use re_space_view::DataQuery as _; + use re_space_view::{DataQuery as _, PropertyResolver as _}; use re_types::archetypes::Points3D; use re_viewer_context::{EntitiesPerSystem, StoreContext}; @@ -541,7 +541,8 @@ mod tests { all_recordings: vec![], }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let mut query_result = query.execute_query(&ctx, &entities_per_system); + resolver.update_overrides(&ctx, &mut query_result); let parent = query_result .tree @@ -567,7 +568,7 @@ mod tests { let mut overrides = parent.individual_properties().cloned().unwrap_or_default(); overrides.visible = false; - save_override(overrides, &parent.override_path().unwrap(), &mut blueprint); + save_override(overrides, parent.override_path().unwrap(), &mut blueprint); } // Parent is not visible, but children are @@ -578,7 +579,8 @@ mod tests { all_recordings: vec![], }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let mut query_result = query.execute_query(&ctx, &entities_per_system); + resolver.update_overrides(&ctx, &mut query_result); let parent_group = query_result .tree @@ -612,7 +614,7 @@ mod tests { save_override( overrides, - &parent_group.override_path().unwrap(), + parent_group.override_path().unwrap(), &mut blueprint, ); } @@ -625,7 +627,8 @@ mod tests { all_recordings: vec![], }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let mut query_result = query.execute_query(&ctx, &entities_per_system); + resolver.update_overrides(&ctx, &mut query_result); let parent = query_result .tree @@ -656,7 +659,7 @@ mod tests { overrides.visible_history.enabled = true; overrides.visible_history.nanos = VisibleHistory::ALL; - save_override(overrides, &root.override_path().unwrap(), &mut blueprint); + save_override(overrides, root.override_path().unwrap(), &mut blueprint); } // Everyone has visible history @@ -667,7 +670,8 @@ mod tests { all_recordings: vec![], }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let mut query_result = query.execute_query(&ctx, &entities_per_system); + resolver.update_overrides(&ctx, &mut query_result); let parent = query_result .tree @@ -693,7 +697,7 @@ mod tests { let mut overrides = child2.individual_properties().cloned().unwrap_or_default(); overrides.visible_history.enabled = true; - save_override(overrides, &child2.override_path().unwrap(), &mut blueprint); + save_override(overrides, child2.override_path().unwrap(), &mut blueprint); } // Child2 has its own visible history @@ -704,7 +708,8 @@ mod tests { all_recordings: vec![], }; - let query_result = query.execute_query(&resolver, &ctx, &entities_per_system); + let mut query_result = query.execute_query(&ctx, &entities_per_system); + resolver.update_overrides(&ctx, &mut query_result); let parent = query_result .tree diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 360bde89741f..bf64526e2527 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -87,7 +87,7 @@ pub fn all_possible_space_views( ); let results = - candidate_query.execute_query_fast(ctx.store_context, entities_per_system); + candidate_query.execute_query(ctx.store_context, entities_per_system); if !results.is_empty() { Some(( From e6f616b663a6b4587ac69f74785a7228afa113b4 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 21 Dec 2023 02:14:09 +0100 Subject: [PATCH 09/12] Cleanup gratuitous profile scopes from debugging --- .../re_space_view/src/data_query_blueprint.rs | 137 +++++++----------- 1 file changed, 54 insertions(+), 83 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index dc7a63387384..dddd5bea7ab0 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -279,20 +279,14 @@ impl DataQuery for DataQueryBlueprint { ) -> DataQueryResult { re_tracing::profile_function!(); - let mut data_results = { - profile_scope!("create slotmap"); - SlotMap::::default() - }; + let mut data_results = SlotMap::::default(); - let executor = { - profile_scope!("build evaluator"); - QueryExpressionEvaluator::new(self, entities_per_system) - }; + let executor = QueryExpressionEvaluator::new(self, entities_per_system); let root_handle = { profile_scope!("run queries"); ctx.recording.and_then(|store| { - executor.add_entity_tree_to_data_results_recursive_fast( + executor.add_entity_tree_to_data_results_recursive( store.tree(), &mut data_results, false, @@ -330,6 +324,7 @@ impl<'a> QueryExpressionEvaluator<'a> { blueprint: &'a DataQueryBlueprint, per_system_entity_list: &'a EntitiesPerSystem, ) -> Self { + re_tracing::profile_function!(); let inclusions: Vec = blueprint .expressions .inclusions @@ -381,80 +376,63 @@ impl<'a> QueryExpressionEvaluator<'a> { } } - fn add_entity_tree_to_data_results_recursive_fast( + fn add_entity_tree_to_data_results_recursive( &self, tree: &EntityTree, data_results: &mut SlotMap, from_recursive: bool, ) -> Option { - //re_tracing::profile_function!(); // If we hit a prefix that is not allowed, we terminate. This is // a pruned branch of the tree. Can come from either an explicit // recursive exclusion, or an implicit missing inclusion. // TODO(jleibs): If this space is disconnected, we should terminate here + if self.recursive_exclusions.contains(&tree.path) + || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) { - //profile_scope!("early_return"); - if self.recursive_exclusions.contains(&tree.path) - || !(from_recursive || self.allowed_prefixes.contains(&tree.path)) - { - return None; - } + return None; } let entity_path = &tree.path; // Pre-compute our matches - let (exact_include, recursive_include, any_match) = { - //profile_scope!("matchers"); - - let exact_include = self.exact_inclusions.contains(entity_path); - let recursive_include = - self.recursive_inclusions.contains(entity_path) || from_recursive; - let exact_exclude = self.exact_exclusions.contains(entity_path); - let any_match = (exact_include || recursive_include) && !exact_exclude; - (exact_include, recursive_include, any_match) - }; + let exact_include = self.exact_inclusions.contains(entity_path); + let recursive_include = self.recursive_inclusions.contains(entity_path) || from_recursive; + let exact_exclude = self.exact_exclusions.contains(entity_path); + let any_match = (exact_include || recursive_include) && !exact_exclude; // Only populate view_parts if this is a match // Note that allowed prefixes that aren't matches can still create groups - let view_parts: SmallVec<_> = { - //profile_scope!("test_view_parts"); - if any_match { - self.per_system_entity_list - .iter() - .filter_map(|(part, ents)| { - if ents.contains(entity_path) { - Some(*part) - } else { - None - } - }) - .collect() - } else { - Default::default() - } + let view_parts: SmallVec<_> = if any_match { + self.per_system_entity_list + .iter() + .filter_map(|(part, ents)| { + if ents.contains(entity_path) { + Some(*part) + } else { + None + } + }) + .collect() + } else { + Default::default() }; - let self_leaf = { - //profile_scope!("leaf insertion"); - if !view_parts.is_empty() || exact_include { - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path: entity_path.clone(), - view_parts, - is_group: false, - direct_included: any_match, - property_overrides: None, - }, - children: Default::default(), - })) - } else { - None - } + let self_leaf = if !view_parts.is_empty() || exact_include { + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path: entity_path.clone(), + view_parts, + is_group: false, + direct_included: any_match, + property_overrides: None, + }, + children: Default::default(), + })) + } else { + None }; let maybe_self_iter = { - //profile_scope!("maybe iter"); if let Some(self_leaf) = self_leaf { itertools::Either::Left(std::iter::once(self_leaf)) } else { @@ -464,7 +442,6 @@ impl<'a> QueryExpressionEvaluator<'a> { let interesting_children: Vec<_> = { if recursive_include { - //profile_scope!("recursive pre-filter"); tree.children .values() .filter(|subtree| { @@ -474,7 +451,6 @@ impl<'a> QueryExpressionEvaluator<'a> { }) .collect() } else { - //profile_scope!("allowed-path pre-filter"); self.allowed_prefixes .iter() .filter(|prefix| prefix.is_child_of(entity_path)) @@ -484,10 +460,9 @@ impl<'a> QueryExpressionEvaluator<'a> { }; let children: SmallVec<_> = { - //profile_scope!("tail recursion"); maybe_self_iter .chain(interesting_children.iter().filter_map(|subtree| { - self.add_entity_tree_to_data_results_recursive_fast( + self.add_entity_tree_to_data_results_recursive( subtree, data_results, recursive_include, // Once we have hit a recursive match, it's always propagated @@ -496,25 +471,21 @@ impl<'a> QueryExpressionEvaluator<'a> { .collect() }; - { - //profile_scope!("final result insertion"); - - // If the only child is the self-leaf, then we don't need to create a group - if children.is_empty() || children.len() == 1 && self_leaf.is_some() { - self_leaf - } else { - // The 'individual' properties of a group are the group overrides - Some(data_results.insert(DataResultNode { - data_result: DataResult { - entity_path: entity_path.clone(), - view_parts: Default::default(), - is_group: true, - direct_included: any_match, - property_overrides: None, - }, - children, - })) - } + // If the only child is the self-leaf, then we don't need to create a group + if children.is_empty() || children.len() == 1 && self_leaf.is_some() { + self_leaf + } else { + // The 'individual' properties of a group are the group overrides + Some(data_results.insert(DataResultNode { + data_result: DataResult { + entity_path: entity_path.clone(), + view_parts: Default::default(), + is_group: true, + direct_included: any_match, + property_overrides: None, + }, + children, + })) } } } From a9c36de86dfe383632c9f356f4b42194fcbd8ece Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 21 Dec 2023 02:14:42 +0100 Subject: [PATCH 10/12] typo --- crates/re_space_view/src/data_query_blueprint.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index dddd5bea7ab0..f37963b9fed2 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -599,8 +599,8 @@ impl DataQueryPropertyResolver<'_> { .individual .get_opt(&node.data_result.entity_path); - let accumulated_properties = if let Some(overidden) = overridden_properties { - accumulated.with_child(overidden) + let accumulated_properties = if let Some(overridden) = overridden_properties { + accumulated.with_child(overridden) } else { accumulated.clone() }; From b822d31935dcc75c01cabf4e2dd876d96489dc51 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 21 Dec 2023 02:43:02 +0100 Subject: [PATCH 11/12] More cleanup and comments --- crates/re_space_view/src/data_query.rs | 2 +- .../re_space_view/src/data_query_blueprint.rs | 103 ++++++++---------- crates/re_space_view/src/lib.rs | 2 +- .../re_viewer_context/src/selection_state.rs | 1 + 4 files changed, 51 insertions(+), 57 deletions(-) diff --git a/crates/re_space_view/src/data_query.rs b/crates/re_space_view/src/data_query.rs index 3f52a0639be0..6413f382abe1 100644 --- a/crates/re_space_view/src/data_query.rs +++ b/crates/re_space_view/src/data_query.rs @@ -1,7 +1,7 @@ use re_data_store::{EntityProperties, EntityPropertyMap}; use re_viewer_context::{DataQueryResult, EntitiesPerSystem, StoreContext}; -pub struct EntityOverrides { +pub struct EntityOverrideContext { pub root: EntityProperties, pub individual: EntityPropertyMap, pub group: EntityPropertyMap, diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index f37963b9fed2..b489d3950c7c 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -3,7 +3,6 @@ use re_data_store::{ EntityProperties, EntityPropertiesComponent, EntityPropertyMap, EntityTree, StoreDb, }; use re_log_types::{DataRow, EntityPath, EntityPathExpr, RowId, TimePoint}; -use re_tracing::profile_scope; use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataQueryResult, DataResult, DataResultHandle, DataResultNode, DataResultTree, @@ -14,7 +13,7 @@ use slotmap::SlotMap; use smallvec::SmallVec; use crate::{ - blueprint::components::QueryExpressions, DataQuery, EntityOverrides, PropertyResolver, + blueprint::components::QueryExpressions, DataQuery, EntityOverrideContext, PropertyResolver, }; /// An implementation of [`DataQuery`] that is built from a collection of [`QueryExpressions`] @@ -272,6 +271,11 @@ impl DataQueryBlueprint { } impl DataQuery for DataQueryBlueprint { + /// Build up the initial [`DataQueryResult`] for this [`DataQueryBlueprint`] + /// + /// Note that this result will not have any resolved [`PropertyOverrides`]. Those can + /// be added by separately calling [`DataQueryPropertyResolver::update_overrides`] on + /// the result. fn execute_query( &self, ctx: &re_viewer_context::StoreContext<'_>, @@ -283,24 +287,17 @@ impl DataQuery for DataQueryBlueprint { let executor = QueryExpressionEvaluator::new(self, entities_per_system); - let root_handle = { - profile_scope!("run queries"); - ctx.recording.and_then(|store| { - executor.add_entity_tree_to_data_results_recursive( - store.tree(), - &mut data_results, - false, - ) - }) - }; - - { - profile_scope!("return results"); + let root_handle = ctx.recording.and_then(|store| { + executor.add_entity_tree_to_data_results_recursive( + store.tree(), + &mut data_results, + false, + ) + }); - DataQueryResult { - id: self.id, - tree: DataResultTree::new(data_results, root_handle), - } + DataQueryResult { + id: self.id, + tree: DataResultTree::new(data_results, root_handle), } } } @@ -432,12 +429,10 @@ impl<'a> QueryExpressionEvaluator<'a> { None }; - let maybe_self_iter = { - if let Some(self_leaf) = self_leaf { - itertools::Either::Left(std::iter::once(self_leaf)) - } else { - itertools::Either::Right(std::iter::empty()) - } + let maybe_self_iter = if let Some(self_leaf) = self_leaf { + itertools::Either::Left(std::iter::once(self_leaf)) + } else { + itertools::Either::Right(std::iter::empty()) }; let interesting_children: Vec<_> = { @@ -498,13 +493,15 @@ pub struct DataQueryPropertyResolver<'a> { } impl DataQueryPropertyResolver<'_> { - /// Helper function to lookup the properties for a given entity path. + /// Helper function to build the [`EntityOverrideContext`] for this [`DataQuery`] /// - /// We start with the auto properties for the `SpaceView` as the base layer and - /// then incrementally override from there. - fn resolve_entity_overrides(&self, ctx: &StoreContext<'_>) -> EntityOverrides { + /// The context is made up of 3 parts: + /// - The root properties are build by merging a stack of paths from the Blueprint Tree. This + /// may include properties from the `SpaceView` or `DataQuery`. + /// - The individual overrides are found by walking an override subtree under the `data_query//individual_overrides` + /// - The group overrides are found by walking an override subtree under the `data_query//group_overrides` + fn build_override_context(&self, ctx: &StoreContext<'_>) -> EntityOverrideContext { re_tracing::profile_function!(); - let blueprint = ctx.blueprint; let mut root: EntityProperties = Default::default(); for prefix in &self.default_stack { @@ -517,47 +514,37 @@ impl DataQueryPropertyResolver<'_> { } } - let mut individual = self.auto_properties.clone(); - - if let Some(tree) = blueprint.tree().subtree(&self.individual_override_root) { - tree.visit_children_recursively(&mut |path: &EntityPath| { - if let Some(props) = blueprint - .store() - .query_timeless_component::(path) - { - let overridden_path = EntityPath::from( - &path.as_slice()[self.individual_override_root.len()..path.len()], - ); - individual.update(overridden_path, props.value.0); - } - }); - } - - EntityOverrides { + EntityOverrideContext { root, individual: self.resolve_entity_overrides_for_path(ctx, &self.individual_override_root), group: self.resolve_entity_overrides_for_path(ctx, &self.recursive_override_root), } } + /// Find all of the overrides for a given path. + /// + /// These overrides are full entity-paths prefixed by the override root. + /// + /// For example the individual override for `world/points` in the context of the query-id `1234` + /// would be found at: `data_query/1234/individual_overrides/world/points`. fn resolve_entity_overrides_for_path( &self, ctx: &StoreContext<'_>, - props_path: &EntityPath, + override_root: &EntityPath, ) -> EntityPropertyMap { re_tracing::profile_function!(); let blueprint = ctx.blueprint; let mut prop_map = self.auto_properties.clone(); - if let Some(tree) = blueprint.tree().subtree(props_path) { + if let Some(tree) = blueprint.tree().subtree(override_root) { tree.visit_children_recursively(&mut |path: &EntityPath| { if let Some(props) = blueprint .store() .query_timeless_component_quiet::(path) { let overridden_path = - EntityPath::from(&path.as_slice()[props_path.len()..path.len()]); + EntityPath::from(&path.as_slice()[override_root.len()..path.len()]); prop_map.update(overridden_path, props.value.0); } }); @@ -565,17 +552,21 @@ impl DataQueryPropertyResolver<'_> { prop_map } + /// Recursively walk the [`DataResultTree`] and update the [`PropertyOverrides`] for each node. + /// + /// This will accumulate the group properties at each step down the tree, and then finally merge + /// with individual overrides at the leafs. fn update_overrides_recursive( &self, query_result: &mut DataQueryResult, - entity_overrides: &EntityOverrides, + override_context: &EntityOverrideContext, accumulated: &EntityProperties, handle: DataResultHandle, ) { if let Some((child_handles, accumulated)) = query_result.tree.lookup_node_mut(handle).and_then(|node| { if node.data_result.is_group { - let overridden_properties = entity_overrides + let overridden_properties = override_context .group .get_opt(&node.data_result.entity_path); @@ -595,7 +586,7 @@ impl DataQueryPropertyResolver<'_> { Some((node.children.clone(), accumulated_properties)) } else { - let overridden_properties = entity_overrides + let overridden_properties = override_context .individual .get_opt(&node.data_result.entity_path); @@ -620,7 +611,7 @@ impl DataQueryPropertyResolver<'_> { for child in child_handles { self.update_overrides_recursive( query_result, - entity_overrides, + override_context, &accumulated, child, ); @@ -630,8 +621,10 @@ impl DataQueryPropertyResolver<'_> { } impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> { + /// Recursively walk the [`DataResultTree`] and update the [`PropertyOverrides`] for each node. fn update_overrides(&self, ctx: &StoreContext<'_>, query_result: &mut DataQueryResult) { - let entity_overrides = self.resolve_entity_overrides(ctx); + re_tracing::profile_function!(); + let entity_overrides = self.build_override_context(ctx); if let Some(root) = query_result.tree.root_handle() { self.update_overrides_recursive( diff --git a/crates/re_space_view/src/lib.rs b/crates/re_space_view/src/lib.rs index f06bd107fdc4..06724825d8e4 100644 --- a/crates/re_space_view/src/lib.rs +++ b/crates/re_space_view/src/lib.rs @@ -9,7 +9,7 @@ mod data_query_blueprint; mod screenshot; mod unreachable_transform_reason; -pub use data_query::{DataQuery, EntityOverrides, PropertyResolver}; +pub use data_query::{DataQuery, EntityOverrideContext, PropertyResolver}; pub use data_query_blueprint::DataQueryBlueprint; pub use screenshot::ScreenshotMode; pub use unreachable_transform_reason::UnreachableTransformReason; diff --git a/crates/re_viewer_context/src/selection_state.rs b/crates/re_viewer_context/src/selection_state.rs index 9f0cdd29e0da..c02d31f4dc04 100644 --- a/crates/re_viewer_context/src/selection_state.rs +++ b/crates/re_viewer_context/src/selection_state.rs @@ -117,6 +117,7 @@ pub struct SelectionState { impl SelectionState { /// Called at the start of each frame pub fn on_frame_start(&mut self, item_retain_condition: impl Fn(&Item) -> bool) { + // Use a different name so we don't get a collision in puffin. re_tracing::profile_scope!("SelectionState::on_frame_start"); let history = self.history.get_mut(); From def055e32c466a530ea2c3761b42ae01440f6c20 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 21 Dec 2023 02:57:28 +0100 Subject: [PATCH 12/12] doclink --- crates/re_space_view/src/data_query_blueprint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index b489d3950c7c..c8d75932b919 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -274,7 +274,7 @@ impl DataQuery for DataQueryBlueprint { /// Build up the initial [`DataQueryResult`] for this [`DataQueryBlueprint`] /// /// Note that this result will not have any resolved [`PropertyOverrides`]. Those can - /// be added by separately calling [`DataQueryPropertyResolver::update_overrides`] on + /// be added by separately calling [`PropertyResolver::update_overrides`] on /// the result. fn execute_query( &self,