From f170086c0ff4a1c861187e9ee64e69c6032e3480 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 12:23:05 +0100 Subject: [PATCH] move disconnected space detection into transform cache as well --- crates/viewer/re_view/src/query.rs | 26 ++++- .../src/contexts/transform_context.rs | 110 ++++++++---------- crates/viewer/re_view_spatial/src/lib.rs | 1 - .../re_view_spatial/src/transform_cache.rs | 39 +++++-- .../src/transform_component_tracker.rs | 104 ----------------- crates/viewer/re_view_spatial/src/view_2d.rs | 2 +- crates/viewer/re_view_spatial/src/view_3d.rs | 1 - 7 files changed, 108 insertions(+), 175 deletions(-) delete mode 100644 crates/viewer/re_view_spatial/src/transform_component_tracker.rs diff --git a/crates/viewer/re_view/src/query.rs b/crates/viewer/re_view/src/query.rs index 173a6e453831..a2c52107e112 100644 --- a/crates/viewer/re_view/src/query.rs +++ b/crates/viewer/re_view/src/query.rs @@ -214,6 +214,12 @@ pub trait DataResultQuery { latest_at_query: &'a LatestAtQuery, ) -> HybridLatestAtResults<'a>; + fn latest_at_with_blueprint_resolved_data_for_component<'a, C: re_types_core::Component>( + &'a self, + ctx: &'a ViewContext<'a>, + latest_at_query: &'a LatestAtQuery, + ) -> HybridLatestAtResults<'a>; + fn query_archetype_with_history<'a, A: re_types_core::Archetype>( &'a self, ctx: &'a ViewContext<'a>, @@ -234,14 +240,30 @@ impl DataResultQuery for DataResult { ctx: &'a ViewContext<'a>, latest_at_query: &'a LatestAtQuery, ) -> HybridLatestAtResults<'a> { - let query_shadowed_defaults = false; + let query_shadowed_components = false; latest_at_with_blueprint_resolved_data( ctx, None, latest_at_query, self, A::all_components().iter().map(|descr| descr.component_name), - query_shadowed_defaults, + query_shadowed_components, + ) + } + + fn latest_at_with_blueprint_resolved_data_for_component<'a, C: re_types_core::Component>( + &'a self, + ctx: &'a ViewContext<'a>, + latest_at_query: &'a LatestAtQuery, + ) -> HybridLatestAtResults<'a> { + let query_shadowed_components = false; + latest_at_with_blueprint_resolved_data( + ctx, + None, + latest_at_query, + self, + std::iter::once(C::name()), + query_shadowed_components, ) } diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs index b252715fb158..52948ccf6d84 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -1,25 +1,20 @@ -use itertools::Either; use nohash_hasher::IntMap; use re_chunk_store::LatestAtQuery; use re_entity_db::{EntityDb, EntityPath, EntityTree}; use re_types::{ - archetypes::{InstancePoses3D, Pinhole, Transform3D}, - components::{ - DisconnectedSpace, ImagePlaneDistance, PinholeProjection, PoseRotationAxisAngle, - PoseRotationQuat, PoseScale3D, PoseTransformMat3x3, PoseTranslation3D, ViewCoordinates, - }, + archetypes::{InstancePoses3D, Transform3D}, + components::{DisconnectedSpace, ImagePlaneDistance, PinholeProjection}, Archetype, Component as _, ComponentNameSet, }; use re_view::DataResultQuery as _; -use re_viewer_context::{IdentifiedViewSystem, ViewContext, ViewContextSystem}; +use re_viewer_context::{DataResultTree, IdentifiedViewSystem, ViewContext, ViewContextSystem}; use vec1::smallvec_v1::SmallVec1; use crate::{ transform_cache::{ CachedTransformsPerTimeline, ResolvedPinholeProjection, TransformCacheStoreSubscriber, }, - transform_component_tracker::TransformComponentTrackerStoreSubscriber, visualizers::image_view_coordinates, }; @@ -190,6 +185,8 @@ impl ViewContextSystem for TransformContext { debug_assert_transform_field_order(ctx.viewer_ctx.reflection); let entity_tree = ctx.recording().tree(); + let query_result = ctx.viewer_ctx.lookup_query_result(query.view_id); + let data_result_tree = &query_result.tree; self.space_origin = query.space_origin.clone(); @@ -212,21 +209,25 @@ impl ViewContextSystem for TransformContext { }; // Child transforms of this space - self.gather_descendants_transforms( - ctx, - query, - current_tree, - ctx.recording(), - &time_query, - // Ignore potential pinhole camera at the root of the view, since it regarded as being "above" this root. - TransformInfo::default(), - transforms_per_timeline, - ); + { + re_tracing::profile_scope!("gather_descendants_transforms"); + + self.gather_descendants_transforms( + ctx, + data_result_tree, + current_tree, + ctx.recording(), + &time_query, + // Ignore potential pinhole camera at the root of the view, since it regarded as being "above" this root. + TransformInfo::default(), + transforms_per_timeline, + ); + } // Walk up from the reference to the highest reachable parent. self.gather_parent_transforms( ctx, - query, + data_result_tree, current_tree, &time_query, transforms_per_timeline, @@ -245,7 +246,7 @@ impl TransformContext { fn gather_parent_transforms<'a>( &mut self, ctx: &'a ViewContext<'a>, - query: &re_viewer_context::ViewQuery<'_>, + data_result_tree: &DataResultTree, mut current_tree: &'a EntityTree, time_query: &LatestAtQuery, transforms_per_timeline: &mut CachedTransformsPerTimeline, @@ -260,9 +261,8 @@ impl TransformContext { let Some(parent_tree) = entity_tree.subtree(&parent_path) else { // Unlike not having the space path in the hierarchy, this should be impossible. re_log::error_once!( - "Path {} is not part of the global entity tree whereas its child {} is", - parent_path, - query.space_origin + "Path {} is not part of the global entity tree whereas its child is", + parent_path ); return; }; @@ -295,7 +295,7 @@ impl TransformContext { // (this skips over everything at and under `current_tree` automatically) self.gather_descendants_transforms( ctx, - query, + data_result_tree, parent_tree, ctx.recording(), time_query, @@ -311,7 +311,7 @@ impl TransformContext { fn gather_descendants_transforms( &mut self, ctx: &ViewContext<'_>, - view_query: &re_viewer_context::ViewQuery<'_>, + data_result_tree: &DataResultTree, subtree: &EntityTree, entity_db: &EntityDb, query: &LatestAtQuery, @@ -332,22 +332,8 @@ impl TransformContext { for child_tree in subtree.children.values() { let child_path = &child_tree.path; - let lookup_image_plane = |p: &_| { - let query_result = ctx.viewer_ctx.lookup_query_result(view_query.view_id); - - query_result - .tree - .lookup_result_by_path(p) - .cloned() - .map(|data_result| { - let results = data_result - .latest_at_with_blueprint_resolved_data::(ctx, query); - - results.get_mono_with_fallback::() - }) - .unwrap_or_default() - .into() - }; + let lookup_image_plane = + |p: &_| lookup_image_plane_distance(ctx, data_result_tree, p, query); let mut encountered_pinhole = twod_in_threed_info .as_ref() @@ -376,7 +362,7 @@ impl TransformContext { self.gather_descendants_transforms( ctx, - view_query, + data_result_tree, child_tree, entity_db, query, @@ -398,6 +384,28 @@ impl TransformContext { } } +fn lookup_image_plane_distance( + ctx: &ViewContext<'_>, + data_result_tree: &DataResultTree, + entity_path: &EntityPath, + query: &LatestAtQuery, +) -> f32 { + re_tracing::profile_function!(); + + data_result_tree + .lookup_result_by_path(entity_path) + .cloned() + .map(|data_result| { + data_result + .latest_at_with_blueprint_resolved_data_for_component::( + ctx, query, + ) + .get_mono_with_fallback::() + }) + .unwrap_or_default() + .into() +} + /// Compute transform info for when we walk up the tree from the reference. fn transform_info_for_upward_propagation( reference_from_ancestor: glam::Affine3A, @@ -622,18 +630,7 @@ fn transforms_at( ) -> Result { // This is called very frequently, don't put a profile scope here. - let potential_transform_components = - TransformComponentTrackerStoreSubscriber::access(&entity_db.store_id(), |tracker| { - tracker.potential_transform_components(entity_path).cloned() - }) - .flatten() - .unwrap_or_default(); - - // TODO: That's a lot of locking. - // TODO: pose transforms? - let Some(entity_transforms) = transforms_per_timeline.entity_transforms(entity_path) else { - // TODO: this skips over disconnected. return Ok(TransformsAtEntity::default()); }; @@ -677,12 +674,7 @@ fn transforms_at( .instance_from_pinhole_image_plane .is_none(); - if no_other_transforms - && potential_transform_components.disconnected_space - && entity_db - .latest_at_component::(entity_path, query) - .map_or(false, |(_index, res)| **res) - { + if no_other_transforms && entity_transforms.disconnected_space() { Err(UnreachableTransformReason::DisconnectedSpace) } else { Ok(transforms_at_entity) diff --git a/crates/viewer/re_view_spatial/src/lib.rs b/crates/viewer/re_view_spatial/src/lib.rs index 0335b61b5a84..1ce80145f306 100644 --- a/crates/viewer/re_view_spatial/src/lib.rs +++ b/crates/viewer/re_view_spatial/src/lib.rs @@ -19,7 +19,6 @@ mod proc_mesh; mod scene_bounding_boxes; mod space_camera_3d; mod spatial_topology; -mod transform_component_tracker; mod ui; mod ui_2d; mod ui_3d; diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 8fe584767d82..83a968aa1b2f 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -49,9 +49,12 @@ pub struct CachedTransformsPerTimeline { pub struct PerTimelinePerEntityTransforms { timeline: Timeline, entity_path: EntityPath, + + // TODO: some of these are exceedingly rare. do we need all that memory? tree_transforms: BTreeMap>, pose_transforms: BTreeMap>>, pinhole_projections: BTreeMap>, + disconnected_space: bool, } enum CacheEntry { @@ -181,6 +184,11 @@ impl PerTimelinePerEntityTransforms { CacheEntry::None => None, } } + + #[inline] + pub fn disconnected_space(&self) -> bool { + self.disconnected_space + } } impl TransformCacheStoreSubscriber { @@ -220,16 +228,17 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { re_tracing::profile_function!(); for event in events { - // TODO:??? - // if event.compacted.is_some() { - // // Compactions don't change the data. - // continue; - // } + if event.compacted.is_some() { + // Compactions don't change the data. + continue; + } if event.kind == re_chunk_store::ChunkStoreDiffKind::Deletion { // Not participating in GC for now. continue; } + // TODO: do a quicker check for nothing happening. + let has_tree_transforms = event .chunk .component_names() @@ -238,14 +247,22 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { .chunk .component_names() .any(|component_name| self.pose_components.contains(&component_name)); - let has_pinhole_or_view_coordinates = event.chunk.component_names().any(|component_name| { component_name == components::PinholeProjection::name() || component_name == components::ViewCoordinates::name() }); + // TODO(#7868): Disconnected space should be deprecated. + let has_disconnected_space = event + .chunk + .component_names() + .any(|component_name| component_name == components::DisconnectedSpace::name()); - if !has_instance_poses && !has_tree_transforms && !has_pinhole_or_view_coordinates { + if !has_instance_poses + && !has_tree_transforms + && !has_pinhole_or_view_coordinates + && !has_disconnected_space + { continue; } @@ -270,8 +287,16 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { tree_transforms: Default::default(), pose_transforms: Default::default(), pinhole_projections: Default::default(), + disconnected_space: has_disconnected_space, }); + // TODO: this still isn't quite right. Need to check if this was set to true? + // TODO(#7868): Disconnected space should be deprecated. + // TODO(#7121): Once a space is disconnected, it can't be re-connected again. + if has_disconnected_space { + per_entity.disconnected_space = true; + } + // Cache lazily since all of these require complex latest-at queries that... // - we don't want to do more often than needed // - would require a lot more context (we could inject that here, but it's not entirely straight forward) diff --git a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs b/crates/viewer/re_view_spatial/src/transform_component_tracker.rs deleted file mode 100644 index 93aef7a9b256..000000000000 --- a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs +++ /dev/null @@ -1,104 +0,0 @@ -use once_cell::sync::OnceCell; - -use nohash_hasher::IntMap; -use re_chunk_store::{ - ChunkStore, ChunkStoreDiffKind, ChunkStoreEvent, ChunkStoreSubscriberHandle, - PerStoreChunkSubscriber, -}; -use re_log_types::{EntityPath, EntityPathHash, StoreId}; -use re_types::Component as _; - -// --- - -// TODO: remove this file - -/// Set of components that an entity ever had over its known lifetime. -#[derive(Default, Clone)] -pub struct PotentialTransformComponentSet { - /// Whether the entity ever had a disconnected space component. - pub disconnected_space: bool, -} - -/// Keeps track of which entities have had any `Transform3D`-related data on any timeline at any -/// point in time. -/// -/// This is used to optimize queries in the `TransformContext`, so that we don't unnecessarily pay -/// for the fixed overhead of all the query layers when we know for a fact that there won't be any -/// data there. -/// This is a huge performance improvement in practice, especially in recordings with many entities. -pub struct TransformComponentTrackerStoreSubscriber { - components_per_entity: IntMap, -} - -impl Default for TransformComponentTrackerStoreSubscriber { - #[inline] - fn default() -> Self { - Self { - components_per_entity: Default::default(), - } - } -} - -impl TransformComponentTrackerStoreSubscriber { - /// Accesses the global store subscriber. - /// - /// Lazily registers the subscriber if it hasn't been registered yet. - pub fn subscription_handle() -> ChunkStoreSubscriberHandle { - static SUBSCRIPTION: OnceCell = OnceCell::new(); - *SUBSCRIPTION.get_or_init(ChunkStore::register_per_store_subscriber::) - } - - /// Accesses the transform component tracking data for a given store. - #[inline] - pub fn access(store_id: &StoreId, f: impl FnOnce(&Self) -> T) -> Option { - ChunkStore::with_per_store_subscriber_once(Self::subscription_handle(), store_id, f) - } - - pub fn potential_transform_components( - &self, - entity_path: &EntityPath, - ) -> Option<&PotentialTransformComponentSet> { - self.components_per_entity.get(&entity_path.hash()) - } -} - -impl PerStoreChunkSubscriber for TransformComponentTrackerStoreSubscriber { - #[inline] - fn name() -> String { - "rerun.store_subscriber.TransformComponentTracker".into() - } - - fn on_events<'a>(&mut self, events: impl Iterator) { - re_tracing::profile_function!(); - - for event in events - // This is only additive, don't care about removals. - .filter(|e| e.kind == ChunkStoreDiffKind::Addition) - { - let entity_path_hash = event.chunk.entity_path().hash(); - - let contains_non_zero_component_array = |component_name| { - event - .chunk - .components() - .get(&component_name) - .map_or(false, |per_desc| { - per_desc - .values() - .any(|list_array| list_array.offsets().lengths().any(|len| len > 0)) - }) - }; - - for component_name in event.chunk.component_names() { - if component_name == re_types::components::DisconnectedSpace::name() - && contains_non_zero_component_array(component_name) - { - self.components_per_entity - .entry(entity_path_hash) - .or_default() - .disconnected_space = true; - } - } - } - } -} diff --git a/crates/viewer/re_view_spatial/src/view_2d.rs b/crates/viewer/re_view_spatial/src/view_2d.rs index 89fb14ea39a3..7954ad130694 100644 --- a/crates/viewer/re_view_spatial/src/view_2d.rs +++ b/crates/viewer/re_view_spatial/src/view_2d.rs @@ -68,7 +68,7 @@ impl ViewClass for SpatialView2D { ) -> Result<(), ViewClassRegistryError> { // Ensure spatial topology & max image dimension is registered. crate::spatial_topology::SpatialTopologyStoreSubscriber::subscription_handle(); - crate::transform_component_tracker::TransformComponentTrackerStoreSubscriber::subscription_handle(); + crate::transform_cache::TransformCacheStoreSubscriber::subscription_handle(); crate::max_image_dimension_subscriber::MaxImageDimensionsStoreSubscriber::subscription_handle(); register_spatial_contexts(system_registry)?; diff --git a/crates/viewer/re_view_spatial/src/view_3d.rs b/crates/viewer/re_view_spatial/src/view_3d.rs index 682c4ff5f510..7d86679728d7 100644 --- a/crates/viewer/re_view_spatial/src/view_3d.rs +++ b/crates/viewer/re_view_spatial/src/view_3d.rs @@ -74,7 +74,6 @@ impl ViewClass for SpatialView3D { ) -> Result<(), ViewClassRegistryError> { // Ensure spatial topology is registered. crate::spatial_topology::SpatialTopologyStoreSubscriber::subscription_handle(); - crate::transform_component_tracker::TransformComponentTrackerStoreSubscriber::subscription_handle(); crate::transform_cache::TransformCacheStoreSubscriber::subscription_handle(); register_spatial_contexts(system_registry)?;