From b89680dbda2a5a9ab9ad1237dd04100785a7725b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 12 Dec 2024 16:07:51 +0100 Subject: [PATCH 01/31] first somewhat working version of transform cache --- .../src/contexts/transform_context.rs | 81 +----- crates/viewer/re_view_spatial/src/lib.rs | 2 + .../re_view_spatial/src/transform_cache.rs | 263 ++++++++++++++++++ .../src/transform_component_tracker.rs | 2 + crates/viewer/re_view_spatial/src/view_3d.rs | 1 + 5 files changed, 277 insertions(+), 72 deletions(-) create mode 100644 crates/viewer/re_view_spatial/src/transform_cache.rs 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 4e7cbc27eb5c..a440d8a891dc 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -8,7 +8,7 @@ use re_types::{ components::{ ImagePlaneDistance, PinholeProjection, PoseRotationAxisAngle, PoseRotationQuat, PoseScale3D, PoseTransformMat3x3, PoseTranslation3D, RotationAxisAngle, RotationQuat, - Scale3D, TransformMat3x3, TransformRelation, Translation3D, ViewCoordinates, + Scale3D, TransformMat3x3, Translation3D, ViewCoordinates, }, Archetype, Component as _, ComponentNameSet, }; @@ -17,6 +17,7 @@ use re_viewer_context::{IdentifiedViewSystem, ViewContext, ViewContextSystem}; use vec1::smallvec_v1::SmallVec1; use crate::{ + transform_cache::TransformCacheStoreSubscriber, transform_component_tracker::TransformComponentTrackerStoreSubscriber, visualizers::image_view_coordinates, }; @@ -528,66 +529,6 @@ But they are instead ordered like this:\n{actual_order:?}" #[cfg(not(debug_assertions))] fn debug_assert_transform_field_order(_: &re_types::reflection::Reflection) {} -fn query_and_resolve_tree_transform_at_entity( - entity_path: &EntityPath, - entity_db: &EntityDb, - query: &LatestAtQuery, - transform3d_components: impl Iterator, -) -> Option { - // TODO(#6743): Doesn't take into account overrides. - let result = entity_db.latest_at(query, entity_path, transform3d_components); - if result.components.is_empty() { - return None; - } - - let mut transform = glam::Affine3A::IDENTITY; - - // Order see `debug_assert_transform_field_order` - if let Some(translation) = result.component_instance::(0) { - transform = glam::Affine3A::from(translation); - } - if let Some(axis_angle) = result.component_instance::(0) { - if let Ok(axis_angle) = glam::Affine3A::try_from(axis_angle) { - transform *= axis_angle; - } else { - // Invalid transform. - return None; - } - } - if let Some(quaternion) = result.component_instance::(0) { - if let Ok(quaternion) = glam::Affine3A::try_from(quaternion) { - transform *= quaternion; - } else { - // Invalid transform. - return None; - } - } - if let Some(scale) = result.component_instance::(0) { - if scale.x() == 0.0 && scale.y() == 0.0 && scale.z() == 0.0 { - // Invalid scale. - return None; - } - transform *= glam::Affine3A::from(scale); - } - if let Some(mat3x3) = result.component_instance::(0) { - let affine_transform = glam::Affine3A::from(mat3x3); - if affine_transform.matrix3.determinant() == 0.0 { - // Invalid transform. - return None; - } - transform *= affine_transform; - } - - if result.component_instance::(0) == Some(TransformRelation::ChildFromParent) - // TODO(andreas): Should we warn? This might be intentionally caused by zero scale. - && transform.matrix3.determinant() != 0.0 - { - transform = transform.inverse(); - } - - Some(transform) -} - fn query_and_resolve_instance_poses_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, @@ -766,17 +707,13 @@ fn transforms_at( .flatten() .unwrap_or_default(); - let parent_from_entity_tree_transform = if potential_transform_components.transform3d.is_empty() - { - None - } else { - query_and_resolve_tree_transform_at_entity( - entity_path, - entity_db, - query, - potential_transform_components.transform3d.iter().copied(), - ) - }; + // TODO: That's a lot of locking. + // TODO: pose transforms? + let parent_from_entity_tree_transform = + TransformCacheStoreSubscriber::access(&entity_db.store_id(), |tracker| { + tracker.latest_at_transforms(entity_path, entity_db, query) + }); + let entity_from_instance_poses = if potential_transform_components.pose3d.is_empty() { Vec::new() } else { diff --git a/crates/viewer/re_view_spatial/src/lib.rs b/crates/viewer/re_view_spatial/src/lib.rs index 62cf77783d4f..0335b61b5a84 100644 --- a/crates/viewer/re_view_spatial/src/lib.rs +++ b/crates/viewer/re_view_spatial/src/lib.rs @@ -29,6 +29,8 @@ mod view_3d; mod view_3d_properties; mod visualizers; +mod transform_cache; + pub use view_2d::SpatialView2D; pub use view_3d::SpatialView3D; diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs new file mode 100644 index 000000000000..c32ab02f96c1 --- /dev/null +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -0,0 +1,263 @@ +use std::collections::BTreeMap; + +use ahash::HashMap; +use nohash_hasher::{IntMap, IntSet}; + +use once_cell::sync::OnceCell; +use re_chunk_store::{ + ChunkStore, ChunkStoreSubscriberHandle, LatestAtQuery, PerStoreChunkSubscriber, +}; +use re_entity_db::EntityDb; +use re_log_types::{EntityPath, EntityPathHash, StoreId, TimeInt, Timeline}; +use re_types::{ + components::{self}, + ComponentName, +}; + +/// Store subscriber that resolves all transform components at a given entity to an affine transform. +pub struct TransformCacheStoreSubscriber { + /// The components of interest. + transform_components: IntSet, + pose_components: IntSet, + transforms_per_timeline: HashMap, +} + +impl Default for TransformCacheStoreSubscriber { + #[inline] + fn default() -> Self { + use re_types::Archetype as _; + + Self { + transform_components: re_types::archetypes::Transform3D::all_components() + .iter() + .map(|descr| descr.component_name) + .collect(), + pose_components: re_types::archetypes::InstancePoses3D::all_components() + .iter() + .map(|descr| descr.component_name) + .collect(), + transforms_per_timeline: Default::default(), + } + } +} + +#[derive(Default)] +struct TransformsPerTimeline { + // Separate maps since we very often only have either for a given entity! + tree_transforms_per_entity_per_time: + IntMap>, + pose_transforms_per_entity_per_time: + IntMap>, +} + +#[derive(Default, Clone)] +pub enum ResolvedTreeTransform { + /// There is a tree transform, and we have a cached value. + Cached(glam::Affine3A), + + /// There is a tree transform, but we don't have anything cached. + Uncached, + + /// There is no tree transform. + #[default] + None, +} + +#[derive(Default, Clone)] +pub enum ResolvedInstancePoses { + /// There are instance poses, and we have a cached value. + Cached(Vec), + + /// There are instance poses, but we don't have anything cached. + Invalidated, + + /// There are no instance poses. + #[default] + None, +} + +impl TransformCacheStoreSubscriber { + /// 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. + // TODO: no mut plz + #[inline] + pub fn access(store_id: &StoreId, f: impl FnMut(&mut Self) -> T) -> Option { + ChunkStore::with_per_store_subscriber_mut(Self::subscription_handle(), store_id, f) + } + + pub fn latest_at_transforms( + &mut self, // TODO: make this immutable + entity_path: &EntityPath, + entity_db: &EntityDb, + query: &LatestAtQuery, + ) -> glam::Affine3A { + // TODO: also handle pose transforms + // TODO: do this only once for a batch of entities. + let Some(transforms_per_timeline) = self.transforms_per_timeline.get_mut(&query.timeline()) + else { + return glam::Affine3A::IDENTITY; + }; + + let Some(tree_transform) = transforms_per_timeline + .tree_transforms_per_entity_per_time + .get_mut(&entity_path.hash()) + .and_then(|transforms_per_time| { + transforms_per_time + .range_mut(..query.at()) + .next_back() + .map(|(_time, transform)| transform) + }) + else { + return glam::Affine3A::IDENTITY; + }; + + match tree_transform { + ResolvedTreeTransform::Cached(transform) => *transform, + ResolvedTreeTransform::Uncached => { + let transform = query_and_resolve_tree_transform_at_entity( + entity_path, + entity_db, + query, + self.transform_components.iter().copied(), //potential_transform_components.transform3d.iter().copied(), + ) + .unwrap_or(glam::Affine3A::IDENTITY); + *tree_transform = ResolvedTreeTransform::Cached(transform); + transform + } + ResolvedTreeTransform::None => glam::Affine3A::IDENTITY, + } + } +} + +impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { + fn name() -> String { + "rerun.TransformResolverStoreSubscriber".to_owned() + } + + fn on_events<'a>(&mut self, events: impl Iterator) { + re_tracing::profile_function!(); + + for event in events { + // TODO:??? + // 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; + } + + let has_tree_transforms = event + .chunk + .component_names() + .any(|component_name| self.transform_components.contains(&component_name)); + let has_instance_poses = event + .chunk + .component_names() + .any(|component_name| self.pose_components.contains(&component_name)); + + if !has_instance_poses && !has_tree_transforms { + continue; + } + + let entity_path_hash = event.chunk.entity_path().hash(); + + for (timeline, time_column) in event.diff.chunk.timelines() { + // Components may only show up on some of the timelines. + // But being overly conservative here is doesn't hurt us much and makes this a lot easier. + let transforms_per_entity = + self.transforms_per_timeline.entry(*timeline).or_default(); + + if has_tree_transforms { + let tree_transforms_per_time = transforms_per_entity + .tree_transforms_per_entity_per_time + .entry(entity_path_hash) + .or_default(); + + // TODO: invalidate things forward in time. + for time in time_column.times() { + tree_transforms_per_time.insert(time, ResolvedTreeTransform::Uncached); + } + } + if has_instance_poses { + let instance_poses_per_time = transforms_per_entity + .pose_transforms_per_entity_per_time + .entry(entity_path_hash) + .or_default(); + + // TODO: invalidate things forward in time. + for time in time_column.times() { + instance_poses_per_time.insert(time, ResolvedInstancePoses::Invalidated); + } + } + } + } + } +} + +fn query_and_resolve_tree_transform_at_entity( + entity_path: &EntityPath, + entity_db: &EntityDb, + query: &LatestAtQuery, + transform3d_components: impl Iterator, +) -> Option { + let result = entity_db.latest_at(query, entity_path, transform3d_components); + if result.components.is_empty() { + return None; + } + + let mut transform = glam::Affine3A::IDENTITY; + + // Order see `debug_assert_transform_field_order` + if let Some(translation) = result.component_instance::(0) { + transform = glam::Affine3A::from(translation); + } + if let Some(axis_angle) = result.component_instance::(0) { + if let Ok(axis_angle) = glam::Affine3A::try_from(axis_angle) { + transform *= axis_angle; + } else { + // Invalid transform. + return None; + } + } + if let Some(quaternion) = result.component_instance::(0) { + if let Ok(quaternion) = glam::Affine3A::try_from(quaternion) { + transform *= quaternion; + } else { + // Invalid transform. + return None; + } + } + if let Some(scale) = result.component_instance::(0) { + if scale.x() == 0.0 && scale.y() == 0.0 && scale.z() == 0.0 { + // Invalid scale. + return None; + } + transform *= glam::Affine3A::from(scale); + } + if let Some(mat3x3) = result.component_instance::(0) { + let affine_transform = glam::Affine3A::from(mat3x3); + if affine_transform.matrix3.determinant() == 0.0 { + // Invalid transform. + return None; + } + transform *= affine_transform; + } + + if result.component_instance::(0) == Some(components::TransformRelation::ChildFromParent) + // TODO(andreas): Should we warn? + && transform.matrix3.determinant() != 0.0 + { + transform = transform.inverse(); + } + + Some(transform) +} diff --git a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs b/crates/viewer/re_view_spatial/src/transform_component_tracker.rs index 7f6b564f10f6..da9a46ad0172 100644 --- a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs +++ b/crates/viewer/re_view_spatial/src/transform_component_tracker.rs @@ -10,6 +10,8 @@ use re_types::{Component as _, ComponentName}; // --- +// TODO: remove this file + /// Set of components that an entity ever had over its known lifetime. #[derive(Default, Clone)] pub struct PotentialTransformComponentSet { diff --git a/crates/viewer/re_view_spatial/src/view_3d.rs b/crates/viewer/re_view_spatial/src/view_3d.rs index 2af66bb20404..cc2c2df17d5f 100644 --- a/crates/viewer/re_view_spatial/src/view_3d.rs +++ b/crates/viewer/re_view_spatial/src/view_3d.rs @@ -75,6 +75,7 @@ impl ViewClass for SpatialView3D { // 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)?; register_3d_spatial_visualizers(system_registry)?; From 712a0f0f6ef261946d61146c8e088e43eac544fd Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 11:34:53 +0100 Subject: [PATCH 02/31] better structure: less lookups to build up transform tree --- .../src/contexts/transform_context.rs | 75 ++++++--- .../re_view_spatial/src/transform_cache.rs | 158 ++++++++++-------- 2 files changed, 141 insertions(+), 92 deletions(-) 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 a440d8a891dc..60ea4a0b69d5 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -7,8 +7,7 @@ use re_types::{ archetypes::{InstancePoses3D, Pinhole, Transform3D}, components::{ ImagePlaneDistance, PinholeProjection, PoseRotationAxisAngle, PoseRotationQuat, - PoseScale3D, PoseTransformMat3x3, PoseTranslation3D, RotationAxisAngle, RotationQuat, - Scale3D, TransformMat3x3, Translation3D, ViewCoordinates, + PoseScale3D, PoseTransformMat3x3, PoseTranslation3D, ViewCoordinates, }, Archetype, Component as _, ComponentNameSet, }; @@ -17,7 +16,7 @@ use re_viewer_context::{IdentifiedViewSystem, ViewContext, ViewContextSystem}; use vec1::smallvec_v1::SmallVec1; use crate::{ - transform_cache::TransformCacheStoreSubscriber, + transform_cache::{CachedTransformsPerTimeline, TransformCacheStoreSubscriber}, transform_component_tracker::TransformComponentTrackerStoreSubscriber, visualizers::image_view_coordinates, }; @@ -198,19 +197,36 @@ impl ViewContextSystem for TransformContext { let time_query = ctx.current_query(); - // 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(), - ); + // TODO: this holds a write lock for way too long! + TransformCacheStoreSubscriber::access(&ctx.recording().store_id(), |cache| { + let Some(transforms_per_timeline) = cache.transforms_per_timeline(query.timeline) + else { + // No transforms on this timeline at all. Nothing to do here! + return; + }; + + // 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, + ); - // Walk up from the reference to the highest reachable parent. - self.gather_parent_transforms(ctx, query, current_tree, &time_query); + // Walk up from the reference to the highest reachable parent. + self.gather_parent_transforms( + ctx, + query, + current_tree, + &time_query, + transforms_per_timeline, + ); + }) + .expect("No transform cache found"); } fn as_any(&self) -> &dyn std::any::Any { @@ -226,6 +242,7 @@ impl TransformContext { query: &re_viewer_context::ViewQuery<'_>, mut current_tree: &'a EntityTree, time_query: &LatestAtQuery, + transforms_per_timeline: &mut CachedTransformsPerTimeline, ) { re_tracing::profile_function!(); @@ -254,6 +271,7 @@ impl TransformContext { // and the fact that there is no meaningful image plane distance for 3D->2D views. |_| 500.0, &mut encountered_pinhole, + transforms_per_timeline, ) { Err(unreachable_reason) => { self.first_unreachable_parent = @@ -276,6 +294,7 @@ impl TransformContext { ctx.recording(), time_query, new_transform, + transforms_per_timeline, ); current_tree = parent_tree; @@ -291,6 +310,7 @@ impl TransformContext { entity_db: &EntityDb, query: &LatestAtQuery, transform: TransformInfo, + transforms_per_timeline: &mut CachedTransformsPerTimeline, ) { let twod_in_threed_info = transform.twod_in_threed_info.clone(); let reference_from_parent = transform.reference_from_entity; @@ -332,6 +352,7 @@ impl TransformContext { query, lookup_image_plane, &mut encountered_pinhole, + transforms_per_timeline, ) { Err(unreachable_reason) => { self.unreachable_descendants @@ -354,6 +375,7 @@ impl TransformContext { entity_db, query, new_transform, + transforms_per_timeline, ); } } @@ -492,15 +514,16 @@ fn transform_info_for_downward_propagation( #[cfg(debug_assertions)] fn debug_assert_transform_field_order(reflection: &re_types::reflection::Reflection) { + use re_types::{components, Archetype as _}; + let expected_order = vec![ - Translation3D::name(), - RotationAxisAngle::name(), - RotationQuat::name(), - Scale3D::name(), - TransformMat3x3::name(), + components::Translation3D::name(), + components::RotationAxisAngle::name(), + components::RotationQuat::name(), + components::Scale3D::name(), + components::TransformMat3x3::name(), ]; - use re_types::Archetype as _; let transform3d_reflection = reflection .archetypes .get(&re_types::archetypes::Transform3D::name()) @@ -697,6 +720,7 @@ fn transforms_at( query: &LatestAtQuery, pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, encountered_pinhole: &mut Option, + transforms_per_timeline: &mut CachedTransformsPerTimeline, ) -> Result { // This is called very frequently, don't put a profile scope here. @@ -709,10 +733,13 @@ fn transforms_at( // 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 pinhole & disconnected. + return Ok(TransformsAtEntity::default()); + }; let parent_from_entity_tree_transform = - TransformCacheStoreSubscriber::access(&entity_db.store_id(), |tracker| { - tracker.latest_at_transforms(entity_path, entity_db, query) - }); + entity_transforms.latest_at_tree_transform(entity_db, query); let entity_from_instance_poses = if potential_transform_components.pose3d.is_empty() { Vec::new() diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index c32ab02f96c1..2abab412b8b2 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -11,7 +11,7 @@ use re_entity_db::EntityDb; use re_log_types::{EntityPath, EntityPathHash, StoreId, TimeInt, Timeline}; use re_types::{ components::{self}, - ComponentName, + Archetype as _, ComponentName, }; /// Store subscriber that resolves all transform components at a given entity to an affine transform. @@ -19,7 +19,7 @@ pub struct TransformCacheStoreSubscriber { /// The components of interest. transform_components: IntSet, pose_components: IntSet, - transforms_per_timeline: HashMap, + per_timeline: HashMap, } impl Default for TransformCacheStoreSubscriber { @@ -36,18 +36,20 @@ impl Default for TransformCacheStoreSubscriber { .iter() .map(|descr| descr.component_name) .collect(), - transforms_per_timeline: Default::default(), + per_timeline: Default::default(), } } } -#[derive(Default)] -struct TransformsPerTimeline { - // Separate maps since we very often only have either for a given entity! - tree_transforms_per_entity_per_time: - IntMap>, - pose_transforms_per_entity_per_time: - IntMap>, +pub struct CachedTransformsPerTimeline { + per_entity: IntMap, +} + +pub struct PerTimelinePerEntityTransforms { + timeline: Timeline, + entity_path: EntityPath, + tree_transforms: BTreeMap, + pose_transforms: BTreeMap, } #[derive(Default, Clone)] @@ -76,6 +78,48 @@ pub enum ResolvedInstancePoses { None, } +impl CachedTransformsPerTimeline { + #[inline] + pub fn entity_transforms( + &mut self, + entity_path: &EntityPath, + ) -> Option<&mut PerTimelinePerEntityTransforms> { + self.per_entity.get_mut(&entity_path.hash()) + } +} + +impl PerTimelinePerEntityTransforms { + pub fn latest_at_tree_transform( + &mut self, // TODO: make this immutable + entity_db: &EntityDb, + query: &LatestAtQuery, + ) -> Option { + debug_assert!(query.timeline() == self.timeline); + + let tree_transform = self + .tree_transforms + .range_mut(..query.at()) + .next_back() + .map(|(_time, transform)| transform)?; + + match tree_transform { + ResolvedTreeTransform::Cached(transform) => Some(*transform), + ResolvedTreeTransform::Uncached => { + let transform = + query_and_resolve_tree_transform_at_entity(&self.entity_path, entity_db, query); + if let Some(transform) = transform { + *tree_transform = ResolvedTreeTransform::Cached(transform); + Some(transform) + } else { + *tree_transform = ResolvedTreeTransform::None; + None + } + } + ResolvedTreeTransform::None => None, + } + } +} + impl TransformCacheStoreSubscriber { /// Accesses the global store subscriber. /// @@ -92,47 +136,15 @@ impl TransformCacheStoreSubscriber { ChunkStore::with_per_store_subscriber_mut(Self::subscription_handle(), store_id, f) } - pub fn latest_at_transforms( - &mut self, // TODO: make this immutable - entity_path: &EntityPath, - entity_db: &EntityDb, - query: &LatestAtQuery, - ) -> glam::Affine3A { - // TODO: also handle pose transforms - // TODO: do this only once for a batch of entities. - let Some(transforms_per_timeline) = self.transforms_per_timeline.get_mut(&query.timeline()) - else { - return glam::Affine3A::IDENTITY; - }; - - let Some(tree_transform) = transforms_per_timeline - .tree_transforms_per_entity_per_time - .get_mut(&entity_path.hash()) - .and_then(|transforms_per_time| { - transforms_per_time - .range_mut(..query.at()) - .next_back() - .map(|(_time, transform)| transform) - }) - else { - return glam::Affine3A::IDENTITY; - }; - - match tree_transform { - ResolvedTreeTransform::Cached(transform) => *transform, - ResolvedTreeTransform::Uncached => { - let transform = query_and_resolve_tree_transform_at_entity( - entity_path, - entity_db, - query, - self.transform_components.iter().copied(), //potential_transform_components.transform3d.iter().copied(), - ) - .unwrap_or(glam::Affine3A::IDENTITY); - *tree_transform = ResolvedTreeTransform::Cached(transform); - transform - } - ResolvedTreeTransform::None => glam::Affine3A::IDENTITY, - } + /// Accesses the transform component tracking data for a given timeline. + /// + /// Returns `None` if the timeline doesn't have any transforms at all. + #[inline] + pub fn transforms_per_timeline( + &mut self, + timeline: Timeline, + ) -> Option<&mut CachedTransformsPerTimeline> { + self.per_timeline.get_mut(&timeline) } } @@ -168,34 +180,42 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { continue; } - let entity_path_hash = event.chunk.entity_path().hash(); + let entity_path = event.chunk.entity_path(); + let entity_path_hash = entity_path.hash(); for (timeline, time_column) in event.diff.chunk.timelines() { // Components may only show up on some of the timelines. // But being overly conservative here is doesn't hurt us much and makes this a lot easier. - let transforms_per_entity = - self.transforms_per_timeline.entry(*timeline).or_default(); + let per_timeline = self.per_timeline.entry(*timeline).or_insert_with(|| { + CachedTransformsPerTimeline { + per_entity: Default::default(), + } + }); + + let per_entity = per_timeline + .per_entity + .entry(entity_path_hash) + .or_insert_with(|| PerTimelinePerEntityTransforms { + entity_path: entity_path.clone(), + timeline: *timeline, + tree_transforms: Default::default(), + pose_transforms: Default::default(), + }); if has_tree_transforms { - let tree_transforms_per_time = transforms_per_entity - .tree_transforms_per_entity_per_time - .entry(entity_path_hash) - .or_default(); - // TODO: invalidate things forward in time. for time in time_column.times() { - tree_transforms_per_time.insert(time, ResolvedTreeTransform::Uncached); + per_entity + .tree_transforms + .insert(time, ResolvedTreeTransform::Uncached); } } if has_instance_poses { - let instance_poses_per_time = transforms_per_entity - .pose_transforms_per_entity_per_time - .entry(entity_path_hash) - .or_default(); - // TODO: invalidate things forward in time. for time in time_column.times() { - instance_poses_per_time.insert(time, ResolvedInstancePoses::Invalidated); + per_entity + .pose_transforms + .insert(time, ResolvedInstancePoses::Invalidated); } } } @@ -207,9 +227,11 @@ fn query_and_resolve_tree_transform_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, query: &LatestAtQuery, - transform3d_components: impl Iterator, ) -> Option { - let result = entity_db.latest_at(query, entity_path, transform3d_components); + // TODO(andreas): Filter out the components we're actually interested in? + let components = re_types::archetypes::Transform3D::all_components(); + let component_names = components.iter().map(|descr| descr.component_name); + let result = entity_db.latest_at(query, entity_path, component_names); if result.components.is_empty() { return None; } From 43fcc3abd903bb20ee5940970e67b3261307c723 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 11:49:53 +0100 Subject: [PATCH 03/31] use transform cache for pose transforms as well --- .../src/contexts/transform_context.rs | 111 +------------- .../re_view_spatial/src/transform_cache.rs | 142 +++++++++++++++++- .../src/transform_component_tracker.rs | 41 +---- 3 files changed, 143 insertions(+), 151 deletions(-) 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 60ea4a0b69d5..e9ee25141592 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -552,106 +552,6 @@ But they are instead ordered like this:\n{actual_order:?}" #[cfg(not(debug_assertions))] fn debug_assert_transform_field_order(_: &re_types::reflection::Reflection) {} -fn query_and_resolve_instance_poses_at_entity( - entity_path: &EntityPath, - entity_db: &EntityDb, - query: &LatestAtQuery, - pose3d_components: impl Iterator, -) -> Vec { - // TODO(#6743): Doesn't take into account overrides. - let result = entity_db.latest_at(query, entity_path, pose3d_components); - - let max_count = result - .components - .iter() - .map(|(name, row)| row.num_instances(name)) - .max() - .unwrap_or(0) as usize; - - if max_count == 0 { - return Vec::new(); - } - - #[inline] - pub fn clamped_or_nothing( - values: Vec, - clamped_len: usize, - ) -> impl Iterator { - let Some(last) = values.last() else { - return Either::Left(std::iter::empty()); - }; - let last = last.clone(); - Either::Right( - values - .into_iter() - .chain(std::iter::repeat(last)) - .take(clamped_len), - ) - } - - let mut iter_translation = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_rotation_quat = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_rotation_axis_angle = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_scale = clamped_or_nothing( - result.component_batch::().unwrap_or_default(), - max_count, - ); - let mut iter_mat3x3 = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - - let mut transforms = Vec::with_capacity(max_count); - for _ in 0..max_count { - // Order see `debug_assert_transform_field_order` - let mut transform = glam::Affine3A::IDENTITY; - if let Some(translation) = iter_translation.next() { - transform = glam::Affine3A::from(translation); - } - if let Some(rotation_quat) = iter_rotation_quat.next() { - if let Ok(rotation_quat) = glam::Affine3A::try_from(rotation_quat) { - transform *= rotation_quat; - } else { - transform = glam::Affine3A::ZERO; - } - } - if let Some(rotation_axis_angle) = iter_rotation_axis_angle.next() { - if let Ok(axis_angle) = glam::Affine3A::try_from(rotation_axis_angle) { - transform *= axis_angle; - } else { - transform = glam::Affine3A::ZERO; - } - } - if let Some(scale) = iter_scale.next() { - transform *= glam::Affine3A::from(scale); - } - if let Some(mat3x3) = iter_mat3x3.next() { - transform *= glam::Affine3A::from(mat3x3); - } - - transforms.push(transform); - } - - transforms -} - fn query_and_resolve_obj_from_pinhole_image_plane( entity_path: &EntityPath, entity_db: &EntityDb, @@ -740,17 +640,8 @@ fn transforms_at( }; let parent_from_entity_tree_transform = entity_transforms.latest_at_tree_transform(entity_db, query); + let entity_from_instance_poses = entity_transforms.latest_at_instance_poses(entity_db, query); - let entity_from_instance_poses = if potential_transform_components.pose3d.is_empty() { - Vec::new() - } else { - query_and_resolve_instance_poses_at_entity( - entity_path, - entity_db, - query, - potential_transform_components.pose3d.iter().copied(), - ) - }; let instance_from_pinhole_image_plane = if potential_transform_components.pinhole { query_and_resolve_obj_from_pinhole_image_plane( entity_path, diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 2abab412b8b2..51a4d23dab43 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use ahash::HashMap; +use itertools::Either; use nohash_hasher::{IntMap, IntSet}; use once_cell::sync::OnceCell; @@ -71,7 +72,7 @@ pub enum ResolvedInstancePoses { Cached(Vec), /// There are instance poses, but we don't have anything cached. - Invalidated, + Uncached, /// There are no instance poses. #[default] @@ -118,6 +119,40 @@ impl PerTimelinePerEntityTransforms { ResolvedTreeTransform::None => None, } } + + pub fn latest_at_instance_poses( + &mut self, // TODO: make this immutable + entity_db: &EntityDb, + query: &LatestAtQuery, + // TODO(andreas): A Cow or reference would be nice here instead of cloning a Vec. At least this is somewhat rare right now? + ) -> Vec { + debug_assert!(query.timeline() == self.timeline); + + let Some(pose_transforms) = self + .pose_transforms + .range_mut(..query.at()) + .next_back() + .map(|(_time, transform)| transform) + else { + return Vec::new(); + }; + + match pose_transforms { + ResolvedInstancePoses::Cached(poses) => poses.clone(), + ResolvedInstancePoses::Uncached => { + let poses = + query_and_resolve_instance_poses_at_entity(&self.entity_path, entity_db, query); + if !poses.is_empty() { + *pose_transforms = ResolvedInstancePoses::Cached(poses.clone()); + poses + } else { + *pose_transforms = ResolvedInstancePoses::None; + Vec::new() + } + } + ResolvedInstancePoses::None => Vec::new(), + } + } } impl TransformCacheStoreSubscriber { @@ -215,7 +250,7 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { for time in time_column.times() { per_entity .pose_transforms - .insert(time, ResolvedInstancePoses::Invalidated); + .insert(time, ResolvedInstancePoses::Uncached); } } } @@ -283,3 +318,106 @@ fn query_and_resolve_tree_transform_at_entity( Some(transform) } + +fn query_and_resolve_instance_poses_at_entity( + entity_path: &EntityPath, + entity_db: &EntityDb, + query: &LatestAtQuery, +) -> Vec { + // TODO(andreas): Filter out the components we're actually interested in? + let components = re_types::archetypes::InstancePoses3D::all_components(); + let component_names = components.iter().map(|descr| descr.component_name); + let result = entity_db.latest_at(query, entity_path, component_names); + + let max_count = result + .components + .iter() + .map(|(name, row)| row.num_instances(name)) + .max() + .unwrap_or(0) as usize; + + if max_count == 0 { + return Vec::new(); + } + + #[inline] + pub fn clamped_or_nothing( + values: Vec, + clamped_len: usize, + ) -> impl Iterator { + let Some(last) = values.last() else { + return Either::Left(std::iter::empty()); + }; + let last = last.clone(); + Either::Right( + values + .into_iter() + .chain(std::iter::repeat(last)) + .take(clamped_len), + ) + } + + let mut iter_translation = clamped_or_nothing( + result + .component_batch::() + .unwrap_or_default(), + max_count, + ); + let mut iter_rotation_quat = clamped_or_nothing( + result + .component_batch::() + .unwrap_or_default(), + max_count, + ); + let mut iter_rotation_axis_angle = clamped_or_nothing( + result + .component_batch::() + .unwrap_or_default(), + max_count, + ); + let mut iter_scale = clamped_or_nothing( + result + .component_batch::() + .unwrap_or_default(), + max_count, + ); + let mut iter_mat3x3 = clamped_or_nothing( + result + .component_batch::() + .unwrap_or_default(), + max_count, + ); + + let mut transforms = Vec::with_capacity(max_count); + for _ in 0..max_count { + // Order see `debug_assert_transform_field_order` + let mut transform = glam::Affine3A::IDENTITY; + if let Some(translation) = iter_translation.next() { + transform = glam::Affine3A::from(translation); + } + if let Some(rotation_quat) = iter_rotation_quat.next() { + if let Ok(rotation_quat) = glam::Affine3A::try_from(rotation_quat) { + transform *= rotation_quat; + } else { + transform = glam::Affine3A::ZERO; + } + } + if let Some(rotation_axis_angle) = iter_rotation_axis_angle.next() { + if let Ok(axis_angle) = glam::Affine3A::try_from(rotation_axis_angle) { + transform *= axis_angle; + } else { + transform = glam::Affine3A::ZERO; + } + } + if let Some(scale) = iter_scale.next() { + transform *= glam::Affine3A::from(scale); + } + if let Some(mat3x3) = iter_mat3x3.next() { + transform *= glam::Affine3A::from(mat3x3); + } + + transforms.push(transform); + } + + transforms +} diff --git a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs b/crates/viewer/re_view_spatial/src/transform_component_tracker.rs index da9a46ad0172..259ba714eb9c 100644 --- a/crates/viewer/re_view_spatial/src/transform_component_tracker.rs +++ b/crates/viewer/re_view_spatial/src/transform_component_tracker.rs @@ -1,12 +1,12 @@ use once_cell::sync::OnceCell; -use nohash_hasher::{IntMap, IntSet}; +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 _, ComponentName}; +use re_types::Component as _; // --- @@ -15,12 +15,6 @@ use re_types::{Component as _, ComponentName}; /// Set of components that an entity ever had over its known lifetime. #[derive(Default, Clone)] pub struct PotentialTransformComponentSet { - /// All transform components ever present. - pub transform3d: IntSet, - - /// All pose transform components ever present. - pub pose3d: IntSet, - /// Whether the entity ever had a pinhole camera. pub pinhole: bool, } @@ -33,26 +27,13 @@ pub struct PotentialTransformComponentSet { /// data there. /// This is a huge performance improvement in practice, especially in recordings with many entities. pub struct TransformComponentTrackerStoreSubscriber { - /// The components of interest. - transform_components: IntSet, - pose_components: IntSet, - components_per_entity: IntMap, } impl Default for TransformComponentTrackerStoreSubscriber { #[inline] fn default() -> Self { - use re_types::Archetype as _; Self { - transform_components: re_types::archetypes::Transform3D::all_components() - .iter() - .map(|descr| descr.component_name) - .collect(), - pose_components: re_types::archetypes::InstancePoses3D::all_components() - .iter() - .map(|descr| descr.component_name) - .collect(), components_per_entity: Default::default(), } } @@ -109,24 +90,6 @@ impl PerStoreChunkSubscriber for TransformComponentTrackerStoreSubscriber { }; for component_name in event.chunk.component_names() { - if self.transform_components.contains(&component_name) - && contains_non_zero_component_array(component_name) - { - self.components_per_entity - .entry(entity_path_hash) - .or_default() - .transform3d - .insert(component_name); - } - if self.pose_components.contains(&component_name) - && contains_non_zero_component_array(component_name) - { - self.components_per_entity - .entry(entity_path_hash) - .or_default() - .pose3d - .insert(component_name); - } if component_name == re_types::components::PinholeProjection::name() && contains_non_zero_component_array(component_name) { From 47235875358a8d957a5cda5697738274517cc24c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 12:06:34 +0100 Subject: [PATCH 04/31] move pinhole to transform cache --- .../src/contexts/transform_context.rs | 115 +++++++---------- crates/viewer/re_view_spatial/src/lib.rs | 1 - .../re_view_spatial/src/transform_cache.rs | 117 ++++++++++++------ .../src/transform_component_tracker.rs | 104 ---------------- crates/viewer/re_view_spatial/src/view_2d.rs | 1 - crates/viewer/re_view_spatial/src/view_3d.rs | 1 - 6 files changed, 129 insertions(+), 210 deletions(-) delete mode 100644 crates/viewer/re_view_spatial/src/transform_component_tracker.rs 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 e9ee25141592..3067b06343a6 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -1,14 +1,10 @@ -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::{ - ImagePlaneDistance, PinholeProjection, PoseRotationAxisAngle, PoseRotationQuat, - PoseScale3D, PoseTransformMat3x3, PoseTranslation3D, ViewCoordinates, - }, + components::{ImagePlaneDistance, PinholeProjection}, Archetype, Component as _, ComponentNameSet, }; use re_view::DataResultQuery as _; @@ -16,8 +12,9 @@ use re_viewer_context::{IdentifiedViewSystem, ViewContext, ViewContextSystem}; use vec1::smallvec_v1::SmallVec1; use crate::{ - transform_cache::{CachedTransformsPerTimeline, TransformCacheStoreSubscriber}, - transform_component_tracker::TransformComponentTrackerStoreSubscriber, + transform_cache::{ + CachedTransformsPerTimeline, ResolvedPinholeProjection, TransformCacheStoreSubscriber, + }, visualizers::image_view_coordinates, }; @@ -552,58 +549,50 @@ But they are instead ordered like this:\n{actual_order:?}" #[cfg(not(debug_assertions))] fn debug_assert_transform_field_order(_: &re_types::reflection::Reflection) {} -fn query_and_resolve_obj_from_pinhole_image_plane( +fn pinhole_with_image_plane_to_transform( entity_path: &EntityPath, - entity_db: &EntityDb, - query: &LatestAtQuery, + resolved_pinhole_projection: &ResolvedPinholeProjection, pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, -) -> Option { - entity_db - .latest_at_component::(entity_path, query) - .map(|(_index, image_from_camera)| { - ( - image_from_camera, - entity_db - .latest_at_component::(entity_path, query) - .map_or(ViewCoordinates::RDF, |(_index, res)| res), - ) - }) - .map(|(image_from_camera, view_coordinates)| { - // Everything under a pinhole camera is a 2D projection, thus doesn't actually have a proper 3D representation. - // Our visualization interprets this as looking at a 2D image plane from a single point (the pinhole). - - // Center the image plane and move it along z, scaling the further the image plane is. - let distance = pinhole_image_plane_distance(entity_path); - let focal_length = image_from_camera.focal_length_in_pixels(); - let focal_length = glam::vec2(focal_length.x(), focal_length.y()); - let scale = distance / focal_length; - let translation = (-image_from_camera.principal_point() * scale).extend(distance); - - let image_plane3d_from_2d_content = glam::Affine3A::from_translation(translation) +) -> glam::Affine3A { + let ResolvedPinholeProjection { + image_from_camera, + view_coordinates, + } = resolved_pinhole_projection; + + // Everything under a pinhole camera is a 2D projection, thus doesn't actually have a proper 3D representation. + // Our visualization interprets this as looking at a 2D image plane from a single point (the pinhole). + + // Center the image plane and move it along z, scaling the further the image plane is. + let distance = pinhole_image_plane_distance(entity_path); + let focal_length = image_from_camera.focal_length_in_pixels(); + let focal_length = glam::vec2(focal_length.x(), focal_length.y()); + let scale = distance / focal_length; + let translation = (-image_from_camera.principal_point() * scale).extend(distance); + + let image_plane3d_from_2d_content = glam::Affine3A::from_translation(translation) // We want to preserve any depth that might be on the pinhole image. // Use harmonic mean of x/y scale for those. * glam::Affine3A::from_scale( scale.extend(2.0 / (1.0 / scale.x + 1.0 / scale.y)), ); - // Our interpretation of the pinhole camera implies that the axis semantics, i.e. ViewCoordinates, - // determine how the image plane is oriented. - // (see also `CamerasPart` where the frustum lines are set up) - let obj_from_image_plane3d = view_coordinates.from_other(&image_view_coordinates()); + // Our interpretation of the pinhole camera implies that the axis semantics, i.e. ViewCoordinates, + // determine how the image plane is oriented. + // (see also `CamerasPart` where the frustum lines are set up) + let obj_from_image_plane3d = view_coordinates.from_other(&image_view_coordinates()); - glam::Affine3A::from_mat3(obj_from_image_plane3d) * image_plane3d_from_2d_content + glam::Affine3A::from_mat3(obj_from_image_plane3d) * image_plane3d_from_2d_content - // Above calculation is nice for a certain kind of visualizing a projected image plane, - // but the image plane distance is arbitrary and there might be other, better visualizations! + // Above calculation is nice for a certain kind of visualizing a projected image plane, + // but the image plane distance is arbitrary and there might be other, better visualizations! - // TODO(#1025): - // As such we don't ever want to invert this matrix! - // However, currently our 2D views require do to exactly that since we're forced to - // build a relationship between the 2D plane and the 3D world, when actually the 2D plane - // should have infinite depth! - // The inverse of this matrix *is* working for this, but quickly runs into precision issues. - // See also `ui_2d.rs#setup_target_config` - }) + // TODO(#1025): + // As such we don't ever want to invert this matrix! + // However, currently our 2D views require do to exactly that since we're forced to + // build a relationship between the 2D plane and the 3D world, when actually the 2D plane + // should have infinite depth! + // The inverse of this matrix *is* working for this, but quickly runs into precision issues. + // See also `ui_2d.rs#setup_target_config` } /// Resolved transforms at an entity. @@ -624,34 +613,26 @@ 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 pinhole & disconnected. + // TODO: this skips over disconnected. return Ok(TransformsAtEntity::default()); }; + let parent_from_entity_tree_transform = entity_transforms.latest_at_tree_transform(entity_db, query); let entity_from_instance_poses = entity_transforms.latest_at_instance_poses(entity_db, query); - - let instance_from_pinhole_image_plane = if potential_transform_components.pinhole { - query_and_resolve_obj_from_pinhole_image_plane( - entity_path, - entity_db, - query, - pinhole_image_plane_distance, - ) - } else { - None - }; + let instance_from_pinhole_image_plane = entity_transforms + .latest_at_pinhole(entity_db, query) + .map(|resolved_pinhole_projection| { + pinhole_with_image_plane_to_transform( + entity_path, + &resolved_pinhole_projection, + pinhole_image_plane_distance, + ) + }); let transforms_at_entity = TransformsAtEntity { parent_from_entity_tree_transform, 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 51a4d23dab43..315d6c29d0bc 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -12,7 +12,7 @@ use re_entity_db::EntityDb; use re_log_types::{EntityPath, EntityPathHash, StoreId, TimeInt, Timeline}; use re_types::{ components::{self}, - Archetype as _, ComponentName, + Archetype as _, Component, ComponentName, }; /// Store subscriber that resolves all transform components at a given entity to an affine transform. @@ -49,34 +49,22 @@ pub struct CachedTransformsPerTimeline { pub struct PerTimelinePerEntityTransforms { timeline: Timeline, entity_path: EntityPath, - tree_transforms: BTreeMap, - pose_transforms: BTreeMap, + tree_transforms: BTreeMap>, + pose_transforms: BTreeMap>>, + pinhole_projections: BTreeMap>, } -#[derive(Default, Clone)] -pub enum ResolvedTreeTransform { - /// There is a tree transform, and we have a cached value. - Cached(glam::Affine3A), - - /// There is a tree transform, but we don't have anything cached. +enum CacheEntry { + Cached(T), Uncached, - - /// There is no tree transform. - #[default] + // TODO: explain why we can't avoid this. None, } -#[derive(Default, Clone)] -pub enum ResolvedInstancePoses { - /// There are instance poses, and we have a cached value. - Cached(Vec), - - /// There are instance poses, but we don't have anything cached. - Uncached, - - /// There are no instance poses. - #[default] - None, +#[derive(Clone)] +pub struct ResolvedPinholeProjection { + pub image_from_camera: components::PinholeProjection, + pub view_coordinates: components::ViewCoordinates, } impl CachedTransformsPerTimeline { @@ -104,19 +92,19 @@ impl PerTimelinePerEntityTransforms { .map(|(_time, transform)| transform)?; match tree_transform { - ResolvedTreeTransform::Cached(transform) => Some(*transform), - ResolvedTreeTransform::Uncached => { + CacheEntry::Cached(transform) => Some(*transform), + CacheEntry::Uncached => { let transform = query_and_resolve_tree_transform_at_entity(&self.entity_path, entity_db, query); if let Some(transform) = transform { - *tree_transform = ResolvedTreeTransform::Cached(transform); + *tree_transform = CacheEntry::Cached(transform); Some(transform) } else { - *tree_transform = ResolvedTreeTransform::None; + *tree_transform = CacheEntry::None; None } } - ResolvedTreeTransform::None => None, + CacheEntry::None => None, } } @@ -138,19 +126,59 @@ impl PerTimelinePerEntityTransforms { }; match pose_transforms { - ResolvedInstancePoses::Cached(poses) => poses.clone(), - ResolvedInstancePoses::Uncached => { + CacheEntry::Cached(poses) => poses.clone(), + CacheEntry::Uncached => { let poses = query_and_resolve_instance_poses_at_entity(&self.entity_path, entity_db, query); if !poses.is_empty() { - *pose_transforms = ResolvedInstancePoses::Cached(poses.clone()); + *pose_transforms = CacheEntry::Cached(poses.clone()); poses } else { - *pose_transforms = ResolvedInstancePoses::None; + *pose_transforms = CacheEntry::None; Vec::new() } } - ResolvedInstancePoses::None => Vec::new(), + CacheEntry::None => Vec::new(), + } + } + + pub fn latest_at_pinhole( + &mut self, // TODO: make this immutable + entity_db: &EntityDb, + query: &LatestAtQuery, + ) -> Option { + debug_assert!(query.timeline() == self.timeline); + + let pinhole_projections = self + .pinhole_projections + .range_mut(..query.at()) + .next_back() + .map(|(_time, transform)| transform)?; + + match pinhole_projections { + CacheEntry::Cached(pinhole) => Some(pinhole.clone()), + CacheEntry::Uncached => { + // TODO: can we do more resolving than this? + if let Some(resolved_pinhole_projection) = entity_db + .latest_at_component::(&self.entity_path, query) + .map(|(_index, image_from_camera)| ResolvedPinholeProjection { + image_from_camera, + view_coordinates: entity_db + .latest_at_component::( + &self.entity_path, + query, + ) + .map_or(components::ViewCoordinates::RDF, |(_index, res)| res), + }) + { + *pinhole_projections = CacheEntry::Cached(resolved_pinhole_projection.clone()); + Some(resolved_pinhole_projection) + } else { + *pinhole_projections = CacheEntry::None; + None + } + } + CacheEntry::None => None, } } } @@ -211,7 +239,13 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { .component_names() .any(|component_name| self.pose_components.contains(&component_name)); - if !has_instance_poses && !has_tree_transforms { + let has_pinhole_or_view_coordinates = + event.chunk.component_names().any(|component_name| { + component_name == components::PinholeProjection::name() + || component_name == components::ViewCoordinates::name() + }); + + if !has_instance_poses && !has_tree_transforms && !has_pinhole_or_view_coordinates { continue; } @@ -235,14 +269,18 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { timeline: *timeline, tree_transforms: Default::default(), pose_transforms: Default::default(), + pinhole_projections: Default::default(), }); + // 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) if has_tree_transforms { // TODO: invalidate things forward in time. for time in time_column.times() { per_entity .tree_transforms - .insert(time, ResolvedTreeTransform::Uncached); + .insert(time, CacheEntry::Uncached); } } if has_instance_poses { @@ -250,7 +288,14 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { for time in time_column.times() { per_entity .pose_transforms - .insert(time, ResolvedInstancePoses::Uncached); + .insert(time, CacheEntry::Uncached); + } + } + if has_pinhole_or_view_coordinates { + for time in time_column.times() { + per_entity + .pinhole_projections + .insert(time, CacheEntry::Uncached); } } } 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 259ba714eb9c..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 pinhole camera. - pub pinhole: 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) - .is_some_and(|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::PinholeProjection::name() - && contains_non_zero_component_array(component_name) - { - self.components_per_entity - .entry(entity_path_hash) - .or_default() - .pinhole = true; - } - } - } - } -} diff --git a/crates/viewer/re_view_spatial/src/view_2d.rs b/crates/viewer/re_view_spatial/src/view_2d.rs index 1ed42c0201d7..e4b5d506ad79 100644 --- a/crates/viewer/re_view_spatial/src/view_2d.rs +++ b/crates/viewer/re_view_spatial/src/view_2d.rs @@ -68,7 +68,6 @@ 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::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 cc2c2df17d5f..c83faf828c5a 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)?; From eb4bd9f6350ccd7078a622c75ebc827630c87597 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 12:23:05 +0100 Subject: [PATCH 05/31] some cleanup --- crates/viewer/re_view/src/query.rs | 26 +++++- .../src/contexts/transform_context.rs | 89 ++++++++++--------- .../re_view_spatial/src/transform_cache.rs | 14 +-- crates/viewer/re_view_spatial/src/view_2d.rs | 1 + 4 files changed, 82 insertions(+), 48 deletions(-) diff --git a/crates/viewer/re_view/src/query.rs b/crates/viewer/re_view/src/query.rs index 7365c2324cc8..d997c2a94e35 100644 --- a/crates/viewer/re_view/src/query.rs +++ b/crates/viewer/re_view/src/query.rs @@ -215,6 +215,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>, @@ -235,14 +241,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 3067b06343a6..a73a405fac09 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -3,12 +3,12 @@ use nohash_hasher::IntMap; use re_chunk_store::LatestAtQuery; use re_entity_db::{EntityDb, EntityPath, EntityTree}; use re_types::{ - archetypes::{InstancePoses3D, Pinhole, Transform3D}, + archetypes::{InstancePoses3D, Transform3D}, components::{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::{ @@ -181,6 +181,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(); @@ -203,21 +205,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, @@ -236,7 +242,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, @@ -251,9 +257,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; }; @@ -286,7 +291,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, @@ -302,7 +307,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, @@ -323,22 +328,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() @@ -367,7 +358,7 @@ impl TransformContext { self.gather_descendants_transforms( ctx, - view_query, + data_result_tree, child_tree, entity_db, query, @@ -389,6 +380,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, @@ -613,11 +626,7 @@ fn transforms_at( ) -> Result { // This is called very frequently, don't put a profile scope here. - // 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()); }; diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 315d6c29d0bc..dabaccd36fc0 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -49,6 +49,8 @@ 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>, @@ -220,16 +222,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,7 +241,6 @@ 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() diff --git a/crates/viewer/re_view_spatial/src/view_2d.rs b/crates/viewer/re_view_spatial/src/view_2d.rs index e4b5d506ad79..1949cfa043cb 100644 --- a/crates/viewer/re_view_spatial/src/view_2d.rs +++ b/crates/viewer/re_view_spatial/src/view_2d.rs @@ -68,6 +68,7 @@ impl ViewClass for SpatialView2D { ) -> Result<(), ViewClassRegistryError> { // Ensure spatial topology & max image dimension is registered. crate::spatial_topology::SpatialTopologyStoreSubscriber::subscription_handle(); + crate::transform_cache::TransformCacheStoreSubscriber::subscription_handle(); crate::max_image_dimension_subscriber::MaxImageDimensionsStoreSubscriber::subscription_handle(); register_spatial_contexts(system_registry)?; From 68a289b6883cbef53ffeff489a05efe87c7d4f4a Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 18:52:23 +0100 Subject: [PATCH 06/31] better commentary on TransformContext, fix for missing transforms, remove unreachable handling --- .../src/contexts/transform_context.rs | 106 ++++++++---------- 1 file changed, 47 insertions(+), 59 deletions(-) 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 a73a405fac09..18c6adb0b233 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -8,7 +8,9 @@ use re_types::{ Archetype, Component as _, ComponentNameSet, }; use re_view::DataResultQuery as _; -use re_viewer_context::{DataResultTree, IdentifiedViewSystem, ViewContext, ViewContextSystem}; +use re_viewer_context::{ + DataResultNode, DataResultTree, IdentifiedViewSystem, ViewContext, ViewContextSystem, +}; use vec1::smallvec_v1::SmallVec1; use crate::{ @@ -104,34 +106,32 @@ impl TransformInfo { } } -#[derive(Clone, Copy)] -enum UnreachableTransformReason { - /// More than one pinhole camera between this and the reference space. - NestedPinholeCameras, -} - /// Provides transforms from an entity to a chosen reference space for all elements in the scene /// for the currently selected time & timeline. /// +/// The resulting transforms are are dependent on: +/// * tree, pose and pinhole transforms components as logged to the data store +/// * TODO(#6743): blueprint overrides aren't respected yet +/// * the view' spatial origin +/// * the query time +/// * TODO(#723): ranges aren't taken into account yet +/// * TODO(andreas): the queried entities. Right now we determine transforms for ALL entities in the scene. +/// since 3D views tend to display almost everything that's mostly fine, but it's very very wasteful when they don't. +/// /// The renderer then uses this reference space as its world space, /// making world and reference space equivalent for a given view. /// -/// Should be recomputed every frame. -/// -/// TODO(#7025): Alternative proposal to not have to deal with tree upwards walking & per-origin tree walking. +/// TODO(#7025): Right now we also do full tree traversal in here to resolve transforms to the root. +/// However, for views that share the same query, we can easily make all entities relative to the respective origin in a linear pass over all matrices. +/// (Note that right now the query IS always the same accross all views for a given frame since it's just latest-at controlled by the timeline, +/// but once we support range queries it may be not or only partially the case) #[derive(Clone)] pub struct TransformContext { /// All transforms provided are relative to this reference path. space_origin: EntityPath, /// All reachable entities. - transform_per_entity: IntMap, - - /// All unreachable descendant paths of `reference_path`. - unreachable_descendants: Vec<(EntityPath, UnreachableTransformReason)>, - - /// The first parent of `reference_path` that is no longer reachable. - first_unreachable_parent: Option<(EntityPath, UnreachableTransformReason)>, + transform_per_entity: IntMap, // TODO: make me a hash } impl IdentifiedViewSystem for TransformContext { @@ -145,8 +145,6 @@ impl Default for TransformContext { Self { space_origin: EntityPath::root(), transform_per_entity: Default::default(), - unreachable_descendants: Default::default(), - first_unreachable_parent: None, } } } @@ -200,7 +198,15 @@ impl ViewContextSystem for TransformContext { TransformCacheStoreSubscriber::access(&ctx.recording().store_id(), |cache| { let Some(transforms_per_timeline) = cache.transforms_per_timeline(query.timeline) else { - // No transforms on this timeline at all. Nothing to do here! + // No transforms on this timeline at all. In other words, everything is identity! + // TODO(andreas): why are query results a tree to begin with? We want a flat list not just here! + query_result.tree.visit(&mut |node: &DataResultNode| { + self.transform_per_entity.insert( + node.data_result.entity_path.clone(), + TransformInfo::default(), + ); + true + }); return; }; @@ -228,8 +234,7 @@ impl ViewContextSystem for TransformContext { &time_query, transforms_per_timeline, ); - }) - .expect("No transform cache found"); + }); // Note that this can return None if no event has happend for this timeline yet. } fn as_any(&self) -> &dyn std::any::Any { @@ -251,7 +256,6 @@ impl TransformContext { let entity_tree = ctx.recording().tree(); - let mut encountered_pinhole = None; let mut reference_from_ancestor = glam::Affine3A::IDENTITY; while let Some(parent_path) = current_tree.path.parent() { let Some(parent_tree) = entity_tree.subtree(&parent_path) else { @@ -265,26 +269,20 @@ impl TransformContext { // Note that the transform at the reference is the first that needs to be inverted to "break out" of its hierarchy. // Generally, the transform _at_ a node isn't relevant to it's children, but only to get to its parent in turn! - let new_transform = match transforms_at( + let transforms_at_entity = transforms_at( ¤t_tree.path, ctx.recording(), time_query, // TODO(#1025): See comment in transform_at. This is a workaround for precision issues // and the fact that there is no meaningful image plane distance for 3D->2D views. |_| 500.0, - &mut encountered_pinhole, + &mut None, // Don't care about pinhole encounters. transforms_per_timeline, - ) { - Err(unreachable_reason) => { - self.first_unreachable_parent = - Some((parent_tree.path.clone(), unreachable_reason)); - break; - } - Ok(transforms_at_entity) => transform_info_for_upward_propagation( - reference_from_ancestor, - transforms_at_entity, - ), - }; + ); + let new_transform = transform_info_for_upward_propagation( + reference_from_ancestor, + transforms_at_entity, + ); reference_from_ancestor = new_transform.reference_from_entity; @@ -334,27 +332,21 @@ impl TransformContext { let mut encountered_pinhole = twod_in_threed_info .as_ref() .map(|info| info.parent_pinhole.clone()); - let new_transform = match transforms_at( + + let transforms_at_entity = transforms_at( child_path, entity_db, query, lookup_image_plane, &mut encountered_pinhole, transforms_per_timeline, - ) { - Err(unreachable_reason) => { - self.unreachable_descendants - .push((child_path.clone(), unreachable_reason)); - continue; - } - - Ok(transforms_at_entity) => transform_info_for_downward_propagation( - child_path, - reference_from_parent, - twod_in_threed_info.clone(), - transforms_at_entity, - ), - }; + ); + let new_transform = transform_info_for_downward_propagation( + child_path, + reference_from_parent, + twod_in_threed_info.clone(), + transforms_at_entity, + ); self.gather_descendants_transforms( ctx, @@ -623,11 +615,11 @@ fn transforms_at( pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, encountered_pinhole: &mut Option, transforms_per_timeline: &mut CachedTransformsPerTimeline, -) -> Result { +) -> TransformsAtEntity { // This is called very frequently, don't put a profile scope here. let Some(entity_transforms) = transforms_per_timeline.entity_transforms(entity_path) else { - return Ok(TransformsAtEntity::default()); + return TransformsAtEntity::default(); }; let parent_from_entity_tree_transform = @@ -654,12 +646,8 @@ fn transforms_at( .instance_from_pinhole_image_plane .is_some() { - if encountered_pinhole.is_some() { - return Err(UnreachableTransformReason::NestedPinholeCameras); - } else { - *encountered_pinhole = Some(entity_path.clone()); - } + *encountered_pinhole = Some(entity_path.clone()); } - Ok(transforms_at_entity) + transforms_at_entity } From 5d121a8ec44c5137f9ca6e95add0c400c68c55e3 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 18:54:55 +0100 Subject: [PATCH 07/31] use entity path hash in TransformContext instead of stored paths --- .../re_view_spatial/src/contexts/transform_context.rs | 11 ++++++----- .../viewer/re_view_spatial/src/visualizers/cameras.rs | 2 +- .../src/visualizers/transform3d_arrows.rs | 2 +- .../src/visualizers/utilities/entity_iterator.rs | 3 ++- 4 files changed, 10 insertions(+), 8 deletions(-) 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 18c6adb0b233..5cfaaa56e103 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -2,6 +2,7 @@ use nohash_hasher::IntMap; use re_chunk_store::LatestAtQuery; use re_entity_db::{EntityDb, EntityPath, EntityTree}; +use re_log_types::EntityPathHash; use re_types::{ archetypes::{InstancePoses3D, Transform3D}, components::{ImagePlaneDistance, PinholeProjection}, @@ -131,7 +132,7 @@ pub struct TransformContext { space_origin: EntityPath, /// All reachable entities. - transform_per_entity: IntMap, // TODO: make me a hash + transform_per_entity: IntMap, } impl IdentifiedViewSystem for TransformContext { @@ -202,7 +203,7 @@ impl ViewContextSystem for TransformContext { // TODO(andreas): why are query results a tree to begin with? We want a flat list not just here! query_result.tree.visit(&mut |node: &DataResultNode| { self.transform_per_entity.insert( - node.data_result.entity_path.clone(), + node.data_result.entity_path.hash(), TransformInfo::default(), ); true @@ -314,7 +315,7 @@ impl TransformContext { ) { let twod_in_threed_info = transform.twod_in_threed_info.clone(); let reference_from_parent = transform.reference_from_entity; - match self.transform_per_entity.entry(subtree.path.clone()) { + match self.transform_per_entity.entry(subtree.path.hash()) { std::collections::hash_map::Entry::Occupied(_) => { return; } @@ -367,8 +368,8 @@ impl TransformContext { /// Retrieves transform information for a given entity. /// /// Returns `None` if it's not reachable from the view's origin. - pub fn transform_info_for_entity(&self, ent_path: &EntityPath) -> Option<&TransformInfo> { - self.transform_per_entity.get(ent_path) + pub fn transform_info_for_entity(&self, ent_path: EntityPathHash) -> Option<&TransformInfo> { + self.transform_per_entity.get(&ent_path) } } diff --git a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs index 709ccae686c4..03f9d108d8b7 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs @@ -71,7 +71,7 @@ impl CamerasVisualizer { } // The camera transform does not include the pinhole transform. - let Some(transform_info) = transforms.transform_info_for_entity(ent_path) else { + let Some(transform_info) = transforms.transform_info_for_entity(ent_path.hash()) else { return; }; let Some(twod_in_threed_info) = &transform_info.twod_in_threed_info else { diff --git a/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs b/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs index a2340f88fd85..48387f241bb9 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs @@ -95,7 +95,7 @@ impl VisualizerSystem for Transform3DArrowsVisualizer { for data_result in query.iter_visible_data_results(ctx, Self::identifier()) { // Use transform without potential pinhole, since we don't want to visualize image-space coordinates. let Some(transform_info) = - transforms.transform_info_for_entity(&data_result.entity_path) + transforms.transform_info_for_entity(data_result.entity_path.hash()) else { continue; }; diff --git a/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs b/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs index 2b5718a26e92..74b2a328c37f 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs @@ -93,7 +93,8 @@ where let system_identifier = System::identifier(); for data_result in query.iter_visible_data_results(ctx, system_identifier) { - let Some(transform_info) = transforms.transform_info_for_entity(&data_result.entity_path) + let Some(transform_info) = + transforms.transform_info_for_entity(data_result.entity_path.hash()) else { continue; }; From 1d84726dffea16e5807ed511789d8081b9a96e1c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 19:18:49 +0100 Subject: [PATCH 08/31] some notes for next-week me --- .../re_view_spatial/src/contexts/transform_context.rs | 2 +- crates/viewer/re_view_spatial/src/transform_cache.rs | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) 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 5cfaaa56e103..12ab74a4b9b1 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -111,7 +111,7 @@ impl TransformInfo { /// for the currently selected time & timeline. /// /// The resulting transforms are are dependent on: -/// * tree, pose and pinhole transforms components as logged to the data store +/// * tree, pose, pinhole and view-coordinates transforms components as logged to the data store /// * TODO(#6743): blueprint overrides aren't respected yet /// * the view' spatial origin /// * the query time diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index dabaccd36fc0..adccb00ca2cd 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -50,12 +50,16 @@ 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>>, + // Note that pinhole projections are fairly rare - it's worth considering storing them separately so we don't have this around for every entity. + // The flipside of that is of course that we'd have to do more lookups (unless we come up with a way to linearly iterate them) pinhole_projections: BTreeMap>, } +// TODO: the cache update idea doesn't quite work out. Instead note down all necessary updates and then process them centrally. Can use a parallel for loop for that if we fancy it. +// just need to find a simple data structure for telling it to update and a place where we do the processing call - maybe a "once per type & frame" call on the ViewClass? + enum CacheEntry { Cached(T), Uncached, From 3452e205b8808bd26e99fc448fedcb02a63700b6 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 13 Jan 2025 16:13:36 +0100 Subject: [PATCH 09/31] transform cache now records updates and applies them instead of populating cache on the fly --- .../src/contexts/transform_context.rs | 55 +-- .../re_view_spatial/src/transform_cache.rs | 337 +++++++++--------- 2 files changed, 202 insertions(+), 190 deletions(-) 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 12ab74a4b9b1..e692a8730651 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_context.rs @@ -1,7 +1,7 @@ use nohash_hasher::IntMap; use re_chunk_store::LatestAtQuery; -use re_entity_db::{EntityDb, EntityPath, EntityTree}; +use re_entity_db::{EntityPath, EntityTree}; use re_log_types::EntityPathHash; use re_types::{ archetypes::{InstancePoses3D, Transform3D}, @@ -179,6 +179,14 @@ impl ViewContextSystem for TransformContext { debug_assert_transform_field_order(ctx.viewer_ctx.reflection); + // Make sure transform cache is up to date. + // TODO(andreas): This is a rather annoying sync point between different views. + // We could alleviate this by introducing a per view class (not instance) method that is called + // before system execution. + TransformCacheStoreSubscriber::access_mut(&ctx.recording().store_id(), |cache| { + cache.apply_all_updates(ctx.recording()); + }); + 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; @@ -195,7 +203,6 @@ impl ViewContextSystem for TransformContext { let time_query = ctx.current_query(); - // TODO: this holds a write lock for way too long! TransformCacheStoreSubscriber::access(&ctx.recording().store_id(), |cache| { let Some(transforms_per_timeline) = cache.transforms_per_timeline(query.timeline) else { @@ -219,7 +226,6 @@ impl ViewContextSystem for TransformContext { 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(), @@ -251,7 +257,7 @@ impl TransformContext { data_result_tree: &DataResultTree, mut current_tree: &'a EntityTree, time_query: &LatestAtQuery, - transforms_per_timeline: &mut CachedTransformsPerTimeline, + transforms_per_timeline: &CachedTransformsPerTimeline, ) { re_tracing::profile_function!(); @@ -272,7 +278,6 @@ impl TransformContext { // Generally, the transform _at_ a node isn't relevant to it's children, but only to get to its parent in turn! let transforms_at_entity = transforms_at( ¤t_tree.path, - ctx.recording(), time_query, // TODO(#1025): See comment in transform_at. This is a workaround for precision issues // and the fact that there is no meaningful image plane distance for 3D->2D views. @@ -292,7 +297,6 @@ impl TransformContext { ctx, data_result_tree, parent_tree, - ctx.recording(), time_query, new_transform, transforms_per_timeline, @@ -308,10 +312,9 @@ impl TransformContext { ctx: &ViewContext<'_>, data_result_tree: &DataResultTree, subtree: &EntityTree, - entity_db: &EntityDb, query: &LatestAtQuery, transform: TransformInfo, - transforms_per_timeline: &mut CachedTransformsPerTimeline, + transforms_per_timeline: &CachedTransformsPerTimeline, ) { let twod_in_threed_info = transform.twod_in_threed_info.clone(); let reference_from_parent = transform.reference_from_entity; @@ -336,7 +339,6 @@ impl TransformContext { let transforms_at_entity = transforms_at( child_path, - entity_db, query, lookup_image_plane, &mut encountered_pinhole, @@ -353,7 +355,6 @@ impl TransformContext { ctx, data_result_tree, child_tree, - entity_db, query, new_transform, transforms_per_timeline, @@ -611,34 +612,34 @@ struct TransformsAtEntity { fn transforms_at( entity_path: &EntityPath, - entity_db: &EntityDb, query: &LatestAtQuery, pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, encountered_pinhole: &mut Option, - transforms_per_timeline: &mut CachedTransformsPerTimeline, + transforms_per_timeline: &CachedTransformsPerTimeline, ) -> TransformsAtEntity { // This is called very frequently, don't put a profile scope here. - let Some(entity_transforms) = transforms_per_timeline.entity_transforms(entity_path) else { + let Some(entity_transforms) = transforms_per_timeline.entity_transforms(entity_path.hash()) + else { return TransformsAtEntity::default(); }; - let parent_from_entity_tree_transform = - entity_transforms.latest_at_tree_transform(entity_db, query); - let entity_from_instance_poses = entity_transforms.latest_at_instance_poses(entity_db, query); - let instance_from_pinhole_image_plane = entity_transforms - .latest_at_pinhole(entity_db, query) - .map(|resolved_pinhole_projection| { - pinhole_with_image_plane_to_transform( - entity_path, - &resolved_pinhole_projection, - pinhole_image_plane_distance, - ) - }); + let parent_from_entity_tree_transform = entity_transforms.latest_at_tree_transform(query); + let entity_from_instance_poses = entity_transforms.latest_at_instance_poses(query); + let instance_from_pinhole_image_plane = + entity_transforms + .latest_at_pinhole(query) + .map(|resolved_pinhole_projection| { + pinhole_with_image_plane_to_transform( + entity_path, + resolved_pinhole_projection, + pinhole_image_plane_distance, + ) + }); let transforms_at_entity = TransformsAtEntity { - parent_from_entity_tree_transform, - entity_from_instance_poses, + parent_from_entity_tree_transform: parent_from_entity_tree_transform.copied(), + entity_from_instance_poses: entity_from_instance_poses.map_or(Vec::new(), |v| v.clone()), instance_from_pinhole_image_plane, }; diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index adccb00ca2cd..849dcd7abdf9 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -17,9 +17,12 @@ use re_types::{ /// Store subscriber that resolves all transform components at a given entity to an affine transform. pub struct TransformCacheStoreSubscriber { - /// The components of interest. transform_components: IntSet, pose_components: IntSet, + pinhole_components: IntSet, + /// All components of interest, a combination of the above. + all_interesting_components: IntSet, + per_timeline: HashMap, } @@ -28,43 +31,72 @@ impl Default for TransformCacheStoreSubscriber { fn default() -> Self { use re_types::Archetype as _; - Self { - transform_components: re_types::archetypes::Transform3D::all_components() - .iter() - .map(|descr| descr.component_name) - .collect(), - pose_components: re_types::archetypes::InstancePoses3D::all_components() + let transform_components: IntSet = + re_types::archetypes::Transform3D::all_components() .iter() .map(|descr| descr.component_name) - .collect(), + .collect(); + let pose_components = re_types::archetypes::InstancePoses3D::all_components() + .iter() + .map(|descr| descr.component_name) + .collect(); + let pinhole_components = [ + components::PinholeProjection::name(), + components::ViewCoordinates::name(), + ] + .into_iter() + .collect(); + + let all_interesting_components = transform_components + .union(&pose_components) + .union(&pinhole_components) + .collect(); + + Self { + transform_components, + pose_components, + pinhole_components, + all_interesting_components, + per_timeline: Default::default(), } } } +bitflags::bitflags! { + /// Flags for the different kinds of independent transforms that the transform cache handles. + #[derive(Debug, Clone, Copy)] + pub struct TransformAspect: u8 { + const TREE = 1 << 0; + const POSE = 1 << 1; + const PINHOLE = 1 << 2; + } +} + +/// Points in time that have changed for a given entity, +/// i.e. the cache is invalid for these times. +struct QueuedTransformUpdates { + entity_path: EntityPath, + times: Vec, + aspects: TransformAspect, +} + pub struct CachedTransformsPerTimeline { + /// Updates that should be applied to the cache. + /// I.e. times & entities at which the cache is invalid right now. + queued_updates: Vec, + per_entity: IntMap, } pub struct PerTimelinePerEntityTransforms { timeline: Timeline, - entity_path: EntityPath, - tree_transforms: BTreeMap>, - pose_transforms: BTreeMap>>, + tree_transforms: BTreeMap, + pose_transforms: BTreeMap>, // Note that pinhole projections are fairly rare - it's worth considering storing them separately so we don't have this around for every entity. // The flipside of that is of course that we'd have to do more lookups (unless we come up with a way to linearly iterate them) - pinhole_projections: BTreeMap>, -} - -// TODO: the cache update idea doesn't quite work out. Instead note down all necessary updates and then process them centrally. Can use a parallel for loop for that if we fancy it. -// just need to find a simple data structure for telling it to update and a place where we do the processing call - maybe a "once per type & frame" call on the ViewClass? - -enum CacheEntry { - Cached(T), - Uncached, - // TODO: explain why we can't avoid this. - None, + pinhole_projections: BTreeMap, } #[derive(Clone)] @@ -76,116 +108,39 @@ pub struct ResolvedPinholeProjection { impl CachedTransformsPerTimeline { #[inline] pub fn entity_transforms( - &mut self, - entity_path: &EntityPath, - ) -> Option<&mut PerTimelinePerEntityTransforms> { - self.per_entity.get_mut(&entity_path.hash()) + &self, + entity_path: EntityPathHash, + ) -> Option<&PerTimelinePerEntityTransforms> { + self.per_entity.get(&entity_path) } } impl PerTimelinePerEntityTransforms { - pub fn latest_at_tree_transform( - &mut self, // TODO: make this immutable - entity_db: &EntityDb, - query: &LatestAtQuery, - ) -> Option { + #[inline] + pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> Option<&glam::Affine3A> { debug_assert!(query.timeline() == self.timeline); - - let tree_transform = self - .tree_transforms - .range_mut(..query.at()) + self.tree_transforms + .range(..query.at()) .next_back() - .map(|(_time, transform)| transform)?; - - match tree_transform { - CacheEntry::Cached(transform) => Some(*transform), - CacheEntry::Uncached => { - let transform = - query_and_resolve_tree_transform_at_entity(&self.entity_path, entity_db, query); - if let Some(transform) = transform { - *tree_transform = CacheEntry::Cached(transform); - Some(transform) - } else { - *tree_transform = CacheEntry::None; - None - } - } - CacheEntry::None => None, - } + .map(|(_time, transform)| transform) } - pub fn latest_at_instance_poses( - &mut self, // TODO: make this immutable - entity_db: &EntityDb, - query: &LatestAtQuery, - // TODO(andreas): A Cow or reference would be nice here instead of cloning a Vec. At least this is somewhat rare right now? - ) -> Vec { + #[inline] + pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> Option<&Vec> { debug_assert!(query.timeline() == self.timeline); - - let Some(pose_transforms) = self - .pose_transforms - .range_mut(..query.at()) + self.pose_transforms + .range(..query.at()) .next_back() .map(|(_time, transform)| transform) - else { - return Vec::new(); - }; - - match pose_transforms { - CacheEntry::Cached(poses) => poses.clone(), - CacheEntry::Uncached => { - let poses = - query_and_resolve_instance_poses_at_entity(&self.entity_path, entity_db, query); - if !poses.is_empty() { - *pose_transforms = CacheEntry::Cached(poses.clone()); - poses - } else { - *pose_transforms = CacheEntry::None; - Vec::new() - } - } - CacheEntry::None => Vec::new(), - } } - pub fn latest_at_pinhole( - &mut self, // TODO: make this immutable - entity_db: &EntityDb, - query: &LatestAtQuery, - ) -> Option { + #[inline] + pub fn latest_at_pinhole(&self, query: &LatestAtQuery) -> Option<&ResolvedPinholeProjection> { debug_assert!(query.timeline() == self.timeline); - - let pinhole_projections = self - .pinhole_projections - .range_mut(..query.at()) + self.pinhole_projections + .range(..query.at()) .next_back() - .map(|(_time, transform)| transform)?; - - match pinhole_projections { - CacheEntry::Cached(pinhole) => Some(pinhole.clone()), - CacheEntry::Uncached => { - // TODO: can we do more resolving than this? - if let Some(resolved_pinhole_projection) = entity_db - .latest_at_component::(&self.entity_path, query) - .map(|(_index, image_from_camera)| ResolvedPinholeProjection { - image_from_camera, - view_coordinates: entity_db - .latest_at_component::( - &self.entity_path, - query, - ) - .map_or(components::ViewCoordinates::RDF, |(_index, res)| res), - }) - { - *pinhole_projections = CacheEntry::Cached(resolved_pinhole_projection.clone()); - Some(resolved_pinhole_projection) - } else { - *pinhole_projections = CacheEntry::None; - None - } - } - CacheEntry::None => None, - } + .map(|(_time, transform)| transform) } } @@ -199,9 +154,14 @@ impl TransformCacheStoreSubscriber { } /// Accesses the transform component tracking data for a given store. - // TODO: no mut plz #[inline] - pub fn access(store_id: &StoreId, f: impl FnMut(&mut Self) -> T) -> Option { + pub fn access(store_id: &StoreId, f: impl FnMut(&Self) -> T) -> Option { + ChunkStore::with_per_store_subscriber(Self::subscription_handle(), store_id, f) + } + + /// Accesses the transform component tracking data for a given store exclusively. + #[inline] + pub fn access_mut(store_id: &StoreId, f: impl FnMut(&mut Self) -> T) -> Option { ChunkStore::with_per_store_subscriber_mut(Self::subscription_handle(), store_id, f) } @@ -210,10 +170,69 @@ impl TransformCacheStoreSubscriber { /// Returns `None` if the timeline doesn't have any transforms at all. #[inline] pub fn transforms_per_timeline( - &mut self, + &self, timeline: Timeline, - ) -> Option<&mut CachedTransformsPerTimeline> { - self.per_timeline.get_mut(&timeline) + ) -> Option<&CachedTransformsPerTimeline> { + self.per_timeline.get(&timeline) + } + + /// Makes sure the transform cache is up to date with the latest data. + /// + /// This needs to be called once per frame. + pub fn apply_all_updates(&mut self, entity_db: &EntityDb) { + re_tracing::profile_function!(); + + for (timeline, per_timeline) in &mut self.per_timeline { + for queued_update in per_timeline.queued_updates.drain(..) { + let entity_path = &queued_update.entity_path; + let entity_entry = per_timeline + .per_entity + .entry(entity_path.hash()) + .or_insert_with(|| PerTimelinePerEntityTransforms { + timeline: *timeline, + tree_transforms: Default::default(), + pose_transforms: Default::default(), + pinhole_projections: Default::default(), + }); + + for time in queued_update.times { + let query = LatestAtQuery::new(*timeline, time); + + if queued_update.aspects.contains(TransformAspect::TREE) { + if let Some(transform) = query_and_resolve_tree_transform_at_entity( + entity_path, + entity_db, + &query, + ) { + entity_entry.tree_transforms.insert(time, transform); + } + } + if queued_update.aspects.contains(TransformAspect::POSE) { + let transforms = query_and_resolve_instance_poses_at_entity( + entity_path, + entity_db, + &query, + ); + if !transforms.is_empty() { + entity_entry.pose_transforms.insert(time, transforms); + } + } + if queued_update.aspects.contains(TransformAspect::PINHOLE) { + if let Some(resolved_pinhole_projection) = + query_and_resolve_pinhole_projection_at_entity( + entity_path, + entity_db, + &query, + ) + { + entity_entry + .pinhole_projections + .insert(time, resolved_pinhole_projection); + } + } + } + } + } } } @@ -251,59 +270,36 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { || component_name == components::ViewCoordinates::name() }); - if !has_instance_poses && !has_tree_transforms && !has_pinhole_or_view_coordinates { + let mut aspects = TransformAspect::empty(); + aspects.set(TransformAspect::TREE, has_tree_transforms); + aspects.set(TransformAspect::POSE, has_instance_poses); + aspects.set(TransformAspect::PINHOLE, has_pinhole_or_view_coordinates); + if aspects.is_empty() { continue; } let entity_path = event.chunk.entity_path(); - let entity_path_hash = entity_path.hash(); for (timeline, time_column) in event.diff.chunk.timelines() { - // Components may only show up on some of the timelines. - // But being overly conservative here is doesn't hurt us much and makes this a lot easier. + // The components we are interested in may only show up on some of the timelines. + // But that's fairly rare, so a few false positive entries here are fine. let per_timeline = self.per_timeline.entry(*timeline).or_insert_with(|| { CachedTransformsPerTimeline { + queued_updates: Default::default(), per_entity: Default::default(), } }); - let per_entity = per_timeline - .per_entity - .entry(entity_path_hash) - .or_insert_with(|| PerTimelinePerEntityTransforms { - entity_path: entity_path.clone(), - timeline: *timeline, - tree_transforms: Default::default(), - pose_transforms: Default::default(), - pinhole_projections: Default::default(), - }); - - // 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) - if has_tree_transforms { - // TODO: invalidate things forward in time. - for time in time_column.times() { - per_entity - .tree_transforms - .insert(time, CacheEntry::Uncached); - } - } - if has_instance_poses { - // TODO: invalidate things forward in time. - for time in time_column.times() { - per_entity - .pose_transforms - .insert(time, CacheEntry::Uncached); - } - } - if has_pinhole_or_view_coordinates { - for time in time_column.times() { - per_entity - .pinhole_projections - .insert(time, CacheEntry::Uncached); - } - } + // All of these require complex latest-at queries that would require a lot more context, + // are fairly expensive, and may depend on other components that may come in at the same time. + // (we could inject that here, but it's not entirely straight forward). + // So instead, we note down that the caches is invalidated for the given entity & times. + per_timeline.queued_updates.push(QueuedTransformUpdates { + entity_path: entity_path.clone(), + times: time_column.times().collect(), + aspects, + }); + // TODO: invalidate existing cache entries caches forward in time. } } } @@ -472,3 +468,18 @@ fn query_and_resolve_instance_poses_at_entity( transforms } + +fn query_and_resolve_pinhole_projection_at_entity( + entity_path: &EntityPath, + entity_db: &EntityDb, + query: &LatestAtQuery, +) -> Option { + entity_db + .latest_at_component::(entity_path, query) + .map(|(_index, image_from_camera)| ResolvedPinholeProjection { + image_from_camera, + view_coordinates: entity_db + .latest_at_component::(entity_path, query) + .map_or(components::ViewCoordinates::RDF, |(_index, res)| res), + }) +} From 5d7c31f1d4bbf72e171ee8eead7f9b80f4da1843 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 13 Jan 2025 16:44:51 +0100 Subject: [PATCH 10/31] improve "aspect" determiniation --- .../re_view_spatial/src/transform_cache.rs | 84 ++++++++----------- 1 file changed, 33 insertions(+), 51 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 849dcd7abdf9..b8804cea20d7 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -20,8 +20,6 @@ pub struct TransformCacheStoreSubscriber { transform_components: IntSet, pose_components: IntSet, pinhole_components: IntSet, - /// All components of interest, a combination of the above. - all_interesting_components: IntSet, per_timeline: HashMap, } @@ -31,32 +29,21 @@ impl Default for TransformCacheStoreSubscriber { fn default() -> Self { use re_types::Archetype as _; - let transform_components: IntSet = - re_types::archetypes::Transform3D::all_components() + Self { + transform_components: re_types::archetypes::Transform3D::all_components() .iter() .map(|descr| descr.component_name) - .collect(); - let pose_components = re_types::archetypes::InstancePoses3D::all_components() - .iter() - .map(|descr| descr.component_name) - .collect(); - let pinhole_components = [ - components::PinholeProjection::name(), - components::ViewCoordinates::name(), - ] - .into_iter() - .collect(); - - let all_interesting_components = transform_components - .union(&pose_components) - .union(&pinhole_components) - .collect(); - - Self { - transform_components, - pose_components, - pinhole_components, - all_interesting_components, + .collect(), + pose_components: re_types::archetypes::InstancePoses3D::all_components() + .iter() + .map(|descr| descr.component_name) + .collect(), + pinhole_components: [ + components::PinholeProjection::name(), + components::ViewCoordinates::name(), + ] + .into_iter() + .collect(), per_timeline: Default::default(), } @@ -67,9 +54,9 @@ bitflags::bitflags! { /// Flags for the different kinds of independent transforms that the transform cache handles. #[derive(Debug, Clone, Copy)] pub struct TransformAspect: u8 { - const TREE = 1 << 0; - const POSE = 1 << 1; - const PINHOLE = 1 << 2; + const Tree = 1 << 0; + const Pose = 1 << 1; + const PinholeOrViewCoordinates = 1 << 2; } } @@ -198,7 +185,7 @@ impl TransformCacheStoreSubscriber { for time in queued_update.times { let query = LatestAtQuery::new(*timeline, time); - if queued_update.aspects.contains(TransformAspect::TREE) { + if queued_update.aspects.contains(TransformAspect::Tree) { if let Some(transform) = query_and_resolve_tree_transform_at_entity( entity_path, entity_db, @@ -207,7 +194,7 @@ impl TransformCacheStoreSubscriber { entity_entry.tree_transforms.insert(time, transform); } } - if queued_update.aspects.contains(TransformAspect::POSE) { + if queued_update.aspects.contains(TransformAspect::Pose) { let transforms = query_and_resolve_instance_poses_at_entity( entity_path, entity_db, @@ -217,7 +204,10 @@ impl TransformCacheStoreSubscriber { entity_entry.pose_transforms.insert(time, transforms); } } - if queued_update.aspects.contains(TransformAspect::PINHOLE) { + if queued_update + .aspects + .contains(TransformAspect::PinholeOrViewCoordinates) + { if let Some(resolved_pinhole_projection) = query_and_resolve_pinhole_projection_at_entity( entity_path, @@ -254,26 +244,18 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { continue; } - // TODO: do a quicker check for nothing happening. - - let has_tree_transforms = event - .chunk - .component_names() - .any(|component_name| self.transform_components.contains(&component_name)); - let has_instance_poses = event - .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() - }); - let mut aspects = TransformAspect::empty(); - aspects.set(TransformAspect::TREE, has_tree_transforms); - aspects.set(TransformAspect::POSE, has_instance_poses); - aspects.set(TransformAspect::PINHOLE, has_pinhole_or_view_coordinates); + for component_name in event.chunk.component_names() { + if self.transform_components.contains(&component_name) { + aspects.set(TransformAspect::Tree, true); + } + if self.pose_components.contains(&component_name) { + aspects.set(TransformAspect::Pose, true); + } + if self.pinhole_components.contains(&component_name) { + aspects.set(TransformAspect::PinholeOrViewCoordinates, true); + } + } if aspects.is_empty() { continue; } From 10e03693716d1eb85fc0fb90a29a63fa8277fa8c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 13 Jan 2025 17:00:40 +0100 Subject: [PATCH 11/31] implement cache invalidation --- .../re_view_spatial/src/transform_cache.rs | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index b8804cea20d7..e07a4a79d43e 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -276,12 +276,40 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { // are fairly expensive, and may depend on other components that may come in at the same time. // (we could inject that here, but it's not entirely straight forward). // So instead, we note down that the caches is invalidated for the given entity & times. + + // Any time _after_ the first event in this chunk is no longer valid now. + // (e.g. if a rotation is added prior to translations later on, + // then the resulting transforms at those translations changes as well for latest-at queries) + let mut invalidated_times = Vec::new(); + let Some(min_time) = time_column.times().min() else { + continue; + }; + if let Some(entity_entry) = per_timeline.per_entity.get_mut(&entity_path.hash()) { + if aspects.contains(TransformAspect::Tree) { + let invalidated_tree_transforms = + entity_entry.tree_transforms.split_off(&min_time); + invalidated_times.extend(invalidated_tree_transforms.into_keys()); + } + if aspects.contains(TransformAspect::Pose) { + let invalidated_pose_transforms = + entity_entry.pose_transforms.split_off(&min_time); + invalidated_times.extend(invalidated_pose_transforms.into_keys()); + } + if aspects.contains(TransformAspect::PinholeOrViewCoordinates) { + let invalidated_pinhole_projections = + entity_entry.pinhole_projections.split_off(&min_time); + invalidated_times.extend(invalidated_pinhole_projections.into_keys()); + } + } + per_timeline.queued_updates.push(QueuedTransformUpdates { entity_path: entity_path.clone(), - times: time_column.times().collect(), + times: time_column + .times() + .chain(invalidated_times.into_iter()) + .collect(), aspects, }); - // TODO: invalidate existing cache entries caches forward in time. } } } From 6c924870f8bcab5e1394873991db5aa4e86f765b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 13 Jan 2025 17:04:12 +0100 Subject: [PATCH 12/31] renamed transform context to transform tree context --- crates/viewer/re_view_spatial/src/contexts/mod.rs | 6 +++--- ...{transform_context.rs => transform_tree_context.rs} | 10 +++++----- .../viewer/re_view_spatial/src/visualizers/cameras.rs | 7 ++++--- .../src/visualizers/transform3d_arrows.rs | 4 ++-- .../src/visualizers/utilities/entity_iterator.rs | 4 ++-- 5 files changed, 16 insertions(+), 15 deletions(-) rename crates/viewer/re_view_spatial/src/contexts/{transform_context.rs => transform_tree_context.rs} (99%) diff --git a/crates/viewer/re_view_spatial/src/contexts/mod.rs b/crates/viewer/re_view_spatial/src/contexts/mod.rs index 7bab048b990c..2bb6a7ac3877 100644 --- a/crates/viewer/re_view_spatial/src/contexts/mod.rs +++ b/crates/viewer/re_view_spatial/src/contexts/mod.rs @@ -1,10 +1,10 @@ mod depth_offsets; -mod transform_context; +mod transform_tree_context; pub use depth_offsets::EntityDepthOffsets; use re_types::ViewClassIdentifier; use re_view::AnnotationSceneContext; -pub use transform_context::{TransformContext, TransformInfo, TwoDInThreeDTransformInfo}; +pub use transform_tree_context::{TransformInfo, TransformTreeContext, TwoDInThreeDTransformInfo}; // ----------------------------------------------------------------------------- @@ -24,7 +24,7 @@ pub struct SpatialSceneEntityContext<'a> { pub fn register_spatial_contexts( system_registry: &mut re_viewer_context::ViewSystemRegistrator<'_>, ) -> Result<(), ViewClassRegistryError> { - system_registry.register_context_system::()?; + system_registry.register_context_system::()?; system_registry.register_context_system::()?; system_registry.register_context_system::()?; Ok(()) diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs similarity index 99% rename from crates/viewer/re_view_spatial/src/contexts/transform_context.rs rename to crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs index e692a8730651..ddfd2f444fe4 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs @@ -127,7 +127,7 @@ impl TransformInfo { /// (Note that right now the query IS always the same accross all views for a given frame since it's just latest-at controlled by the timeline, /// but once we support range queries it may be not or only partially the case) #[derive(Clone)] -pub struct TransformContext { +pub struct TransformTreeContext { /// All transforms provided are relative to this reference path. space_origin: EntityPath, @@ -135,13 +135,13 @@ pub struct TransformContext { transform_per_entity: IntMap, } -impl IdentifiedViewSystem for TransformContext { +impl IdentifiedViewSystem for TransformTreeContext { fn identifier() -> re_viewer_context::ViewSystemIdentifier { "TransformContext".into() } } -impl Default for TransformContext { +impl Default for TransformTreeContext { fn default() -> Self { Self { space_origin: EntityPath::root(), @@ -150,7 +150,7 @@ impl Default for TransformContext { } } -impl ViewContextSystem for TransformContext { +impl ViewContextSystem for TransformTreeContext { fn compatible_component_sets(&self) -> Vec { vec![ Transform3D::all_components() @@ -249,7 +249,7 @@ impl ViewContextSystem for TransformContext { } } -impl TransformContext { +impl TransformTreeContext { /// Gather transforms for everything _above_ the root. fn gather_parent_transforms<'a>( &mut self, diff --git a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs index 03f9d108d8b7..64174545f520 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs @@ -14,7 +14,8 @@ use re_viewer_context::{ use super::{filter_visualizable_3d_entities, SpatialViewVisualizerData}; use crate::{ - contexts::TransformContext, query_pinhole, space_camera_3d::SpaceCamera3D, ui::SpatialViewState, + contexts::TransformTreeContext, query_pinhole, space_camera_3d::SpaceCamera3D, + ui::SpatialViewState, }; const CAMERA_COLOR: re_renderer::Color32 = re_renderer::Color32::from_rgb(150, 150, 150); @@ -46,7 +47,7 @@ impl CamerasVisualizer { fn visit_instance( &mut self, line_builder: &mut re_renderer::LineDrawableBuilder<'_>, - transforms: &TransformContext, + transforms: &TransformTreeContext, data_result: &DataResult, pinhole: &Pinhole, pinhole_view_coordinates: ViewCoordinates, @@ -214,7 +215,7 @@ impl VisualizerSystem for CamerasVisualizer { query: &ViewQuery<'_>, context_systems: &ViewContextCollection, ) -> Result, ViewSystemExecutionError> { - let transforms = context_systems.get::()?; + let transforms = context_systems.get::()?; // Counting all cameras ahead of time is a bit wasteful, but we also don't expect a huge amount, // so let re_renderer's allocator internally decide what buffer sizes to pick & grow them as we go. diff --git a/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs b/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs index 48387f241bb9..1ecb0dc73177 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/transform3d_arrows.rs @@ -13,7 +13,7 @@ use re_viewer_context::{ VisualizableEntities, VisualizableFilterContext, VisualizerQueryInfo, VisualizerSystem, }; -use crate::{contexts::TransformContext, ui::SpatialViewState, view_kind::SpatialViewKind}; +use crate::{contexts::TransformTreeContext, ui::SpatialViewState, view_kind::SpatialViewKind}; use super::{filter_visualizable_3d_entities, CamerasVisualizer, SpatialViewVisualizerData}; @@ -81,7 +81,7 @@ impl VisualizerSystem for Transform3DArrowsVisualizer { query: &ViewQuery<'_>, context_systems: &ViewContextCollection, ) -> Result, ViewSystemExecutionError> { - let transforms = context_systems.get::()?; + let transforms = context_systems.get::()?; let latest_at_query = re_chunk_store::LatestAtQuery::new(query.timeline, query.latest_at); diff --git a/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs b/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs index 74b2a328c37f..2ca627bf1430 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/utilities/entity_iterator.rs @@ -6,7 +6,7 @@ use re_viewer_context::{ ViewSystemExecutionError, }; -use crate::contexts::{EntityDepthOffsets, SpatialSceneEntityContext, TransformContext}; +use crate::contexts::{EntityDepthOffsets, SpatialSceneEntityContext, TransformTreeContext}; // --- @@ -84,7 +84,7 @@ where &HybridResults<'_>, ) -> Result<(), ViewSystemExecutionError>, { - let transforms = view_ctx.get::()?; + let transforms = view_ctx.get::()?; let depth_offsets = view_ctx.get::()?; let annotations = view_ctx.get::()?; From b719f5ff0aa38ff04733dbf573d61c545cd33f54 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 14 Jan 2025 00:02:57 +0100 Subject: [PATCH 13/31] test wip (why is it failing?) --- .../src/contexts/transform_tree_context.rs | 1 - .../re_view_spatial/src/transform_cache.rs | 115 +++++++++++++++++- 2 files changed, 112 insertions(+), 4 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs index ddfd2f444fe4..0fc19ae166c6 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs @@ -176,7 +176,6 @@ impl ViewContextSystem for TransformTreeContext { query: &re_viewer_context::ViewQuery<'_>, ) { re_tracing::profile_function!(); - debug_assert_transform_field_order(ctx.viewer_ctx.reflection); // Make sure transform cache is up to date. diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index e07a4a79d43e..6aa972cfe845 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -62,6 +62,7 @@ bitflags::bitflags! { /// Points in time that have changed for a given entity, /// i.e. the cache is invalid for these times. +#[derive(Debug)] struct QueuedTransformUpdates { entity_path: EntityPath, times: Vec, @@ -107,7 +108,7 @@ impl PerTimelinePerEntityTransforms { pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> Option<&glam::Affine3A> { debug_assert!(query.timeline() == self.timeline); self.tree_transforms - .range(..query.at()) + .range(..query.at().inc()) .next_back() .map(|(_time, transform)| transform) } @@ -116,7 +117,7 @@ impl PerTimelinePerEntityTransforms { pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> Option<&Vec> { debug_assert!(query.timeline() == self.timeline); self.pose_transforms - .range(..query.at()) + .range(..query.at().inc()) .next_back() .map(|(_time, transform)| transform) } @@ -125,7 +126,7 @@ impl PerTimelinePerEntityTransforms { pub fn latest_at_pinhole(&self, query: &LatestAtQuery) -> Option<&ResolvedPinholeProjection> { debug_assert!(query.timeline() == self.timeline); self.pinhole_projections - .range(..query.at()) + .range(..query.at().inc()) .next_back() .map(|(_time, transform)| transform) } @@ -493,3 +494,111 @@ fn query_and_resolve_pinhole_projection_at_entity( .map_or(components::ViewCoordinates::RDF, |(_index, res)| res), }) } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use re_chunk_store::{external::re_chunk::ChunkBuilder, ChunkId, RowId}; + use re_types::archetypes; + + use super::*; + + #[test] + fn test_tree_transforms() { + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + TransformCacheStoreSubscriber::access(&entity_db.store_id(), |_| { + // Make sure the subscriber is registered. + }); + + // Log a few tree transforms at different times. + let timeline = Timeline::new_sequence("t"); + entity_db + .add_chunk(&Arc::new( + ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Transform3D::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)), + ) + .with_component_batch( + RowId::new(), + [(timeline, 3)], + // Don't clear out existing translation. + &[components::Scale3D(glam::Vec3::new(1.0, 2.0, 3.0).into())], + ) + .with_archetype( + RowId::new(), + [(timeline, 4)], + // Clears out previous translation & scale. + //&archetypes::Transform3D::from_rotation(glam::Quat::from_rotation_x(1.0)), + &archetypes::Transform3D::from_translation(glam::Vec3::new( + 123.0, 2.0, 3.0, + )), + ) + .build() + .unwrap(), + )) + .unwrap(); + + // Check that the transform cache has the expected transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); + assert!(transforms_per_timeline + .entity_transforms(EntityPath::from("not_my_entity").hash()) + .is_none()); + + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("my_entity").hash()) + .unwrap(); + + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 0)), + None + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 1)), + Some(&glam::Affine3A::from_translation(glam::Vec3::new( + 1.0, 2.0, 3.0 + ))) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 2)), + Some(&glam::Affine3A::from_translation(glam::Vec3::new( + 1.0, 2.0, 3.0 + ))) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 3)), + Some(&glam::Affine3A::from_scale_rotation_translation( + glam::Vec3::new(1.0, 2.0, 3.0), + glam::Quat::IDENTITY, + glam::Vec3::new(1.0, 2.0, 3.0), + )) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 4)), + // Some(&glam::Affine3A::from_quat(glam::Quat::from_rotation_x(1.0))) + Some(&glam::Affine3A::from_translation(glam::Vec3::new( + 123.0, 2.0, 3.0 + ))) + ); + }); + } + + #[test] + fn test_pose_transforms() { + // TODO: + } + + #[test] + fn test_pinhole_projections() { + // TODO: + } + + #[test] + fn test_invalidation() { + // TODO: + } +} From fbba77cfb4fd2f3daa7e153c515c1a8a444277bb Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 14 Jan 2025 10:17:24 +0100 Subject: [PATCH 14/31] Fix not all events being processed --- crates/viewer/re_view_spatial/src/transform_cache.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 6aa972cfe845..8e024c686786 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -236,10 +236,6 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { re_tracing::profile_function!(); for event in events { - 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; From 2e2545feaa12357b58bf8529ea96d4a46e511088 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 14 Jan 2025 18:45:33 +0100 Subject: [PATCH 15/31] add more tests, fix clears, improve interface --- .../src/contexts/transform_tree_context.rs | 53 +-- .../re_view_spatial/src/transform_cache.rs | 417 ++++++++++++++---- 2 files changed, 345 insertions(+), 125 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs index 0fc19ae166c6..ebf02596994f 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs @@ -286,7 +286,7 @@ impl TransformTreeContext { ); let new_transform = transform_info_for_upward_propagation( reference_from_ancestor, - transforms_at_entity, + &transforms_at_entity, ); reference_from_ancestor = new_transform.reference_from_entity; @@ -347,7 +347,7 @@ impl TransformTreeContext { child_path, reference_from_parent, twod_in_threed_info.clone(), - transforms_at_entity, + &transforms_at_entity, ); self.gather_descendants_transforms( @@ -398,7 +398,7 @@ fn lookup_image_plane_distance( /// Compute transform info for when we walk up the tree from the reference. fn transform_info_for_upward_propagation( reference_from_ancestor: glam::Affine3A, - transforms_at_entity: TransformsAtEntity, + transforms_at_entity: &TransformsAtEntity<'_>, ) -> TransformInfo { let mut reference_from_entity = reference_from_ancestor; @@ -416,7 +416,7 @@ fn transform_info_for_upward_propagation( // Collect & compute poses. let (mut reference_from_instances, has_instance_transforms) = - if let Ok(mut entity_from_instances) = SmallVec1::<[glam::Affine3A; 1]>::try_from_vec( + if let Ok(mut entity_from_instances) = SmallVec1::<[glam::Affine3A; 1]>::try_from_slice( transforms_at_entity.entity_from_instance_poses, ) { for entity_from_instance in &mut entity_from_instances { @@ -429,17 +429,15 @@ fn transform_info_for_upward_propagation( }; // Apply tree transform if any. - if let Some(parent_from_entity_tree_transform) = - transforms_at_entity.parent_from_entity_tree_transform - { - reference_from_entity *= parent_from_entity_tree_transform.inverse(); - if has_instance_transforms { - for reference_from_instance in &mut reference_from_instances { - *reference_from_instance = reference_from_entity * (*reference_from_instance); - } - } else { - *reference_from_instances.first_mut() = reference_from_entity; + reference_from_entity *= transforms_at_entity + .parent_from_entity_tree_transform + .inverse(); + if has_instance_transforms { + for reference_from_instance in &mut reference_from_instances { + *reference_from_instance = reference_from_entity * (*reference_from_instance); } + } else { + *reference_from_instances.first_mut() = reference_from_entity; } TransformInfo { @@ -457,21 +455,18 @@ fn transform_info_for_downward_propagation( current_path: &EntityPath, reference_from_parent: glam::Affine3A, mut twod_in_threed_info: Option, - transforms_at_entity: TransformsAtEntity, + transforms_at_entity: &TransformsAtEntity<'_>, ) -> TransformInfo { let mut reference_from_entity = reference_from_parent; // Apply tree transform. - if let Some(parent_from_entity_tree_transform) = - transforms_at_entity.parent_from_entity_tree_transform - { - reference_from_entity *= parent_from_entity_tree_transform; - } + + reference_from_entity *= transforms_at_entity.parent_from_entity_tree_transform; // Collect & compute poses. let (mut reference_from_instances, has_instance_transforms) = if let Ok(mut entity_from_instances) = - SmallVec1::try_from_vec(transforms_at_entity.entity_from_instance_poses) + SmallVec1::try_from_slice(transforms_at_entity.entity_from_instance_poses) { for entity_from_instance in &mut entity_from_instances { *entity_from_instance = reference_from_entity * (*entity_from_instance); @@ -603,19 +598,19 @@ fn pinhole_with_image_plane_to_transform( /// Resolved transforms at an entity. #[derive(Default)] -struct TransformsAtEntity { - parent_from_entity_tree_transform: Option, - entity_from_instance_poses: Vec, +struct TransformsAtEntity<'a> { + parent_from_entity_tree_transform: glam::Affine3A, + entity_from_instance_poses: &'a [glam::Affine3A], instance_from_pinhole_image_plane: Option, } -fn transforms_at( +fn transforms_at<'a>( entity_path: &EntityPath, query: &LatestAtQuery, pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, encountered_pinhole: &mut Option, - transforms_per_timeline: &CachedTransformsPerTimeline, -) -> TransformsAtEntity { + transforms_per_timeline: &'a CachedTransformsPerTimeline, +) -> TransformsAtEntity<'a> { // This is called very frequently, don't put a profile scope here. let Some(entity_transforms) = transforms_per_timeline.entity_transforms(entity_path.hash()) @@ -637,8 +632,8 @@ fn transforms_at( }); let transforms_at_entity = TransformsAtEntity { - parent_from_entity_tree_transform: parent_from_entity_tree_transform.copied(), - entity_from_instance_poses: entity_from_instance_poses.map_or(Vec::new(), |v| v.clone()), + parent_from_entity_tree_transform, + entity_from_instance_poses, instance_from_pinhole_image_plane, }; diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 8e024c686786..0438cc7165e8 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -84,15 +84,21 @@ pub struct PerTimelinePerEntityTransforms { pose_transforms: BTreeMap>, // Note that pinhole projections are fairly rare - it's worth considering storing them separately so we don't have this around for every entity. // The flipside of that is of course that we'd have to do more lookups (unless we come up with a way to linearly iterate them) - pinhole_projections: BTreeMap, + pinhole_projections: BTreeMap>, } -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq)] pub struct ResolvedPinholeProjection { pub image_from_camera: components::PinholeProjection, pub view_coordinates: components::ViewCoordinates, } +impl ResolvedPinholeProjection { + /// View coordinates used when there's no view coordinates explicitely logged. + pub const DEFAULT_VIEW_COORDINATES: components::ViewCoordinates = + components::ViewCoordinates::RDF; +} + impl CachedTransformsPerTimeline { #[inline] pub fn entity_transforms( @@ -105,21 +111,23 @@ impl CachedTransformsPerTimeline { impl PerTimelinePerEntityTransforms { #[inline] - pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> Option<&glam::Affine3A> { + pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> glam::Affine3A { debug_assert!(query.timeline() == self.timeline); self.tree_transforms .range(..query.at().inc()) .next_back() - .map(|(_time, transform)| transform) + .map(|(_time, transform)| *transform) + .unwrap_or(glam::Affine3A::IDENTITY) } #[inline] - pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> Option<&Vec> { + pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> &[glam::Affine3A] { debug_assert!(query.timeline() == self.timeline); self.pose_transforms .range(..query.at().inc()) .next_back() - .map(|(_time, transform)| transform) + .map(|(_time, transform)| transform.as_slice()) + .unwrap_or(&[]) } #[inline] @@ -128,7 +136,7 @@ impl PerTimelinePerEntityTransforms { self.pinhole_projections .range(..query.at().inc()) .next_back() - .map(|(_time, transform)| transform) + .and_then(|(_time, transform)| transform.as_ref()) } } @@ -193,6 +201,11 @@ impl TransformCacheStoreSubscriber { &query, ) { entity_entry.tree_transforms.insert(time, transform); + } else { + // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! + entity_entry + .tree_transforms + .insert(time, glam::Affine3A::IDENTITY); } } if queued_update.aspects.contains(TransformAspect::Pose) { @@ -201,25 +214,22 @@ impl TransformCacheStoreSubscriber { entity_db, &query, ); - if !transforms.is_empty() { - entity_entry.pose_transforms.insert(time, transforms); - } + // *do* also insert empty ones, otherwise it's not possible to clear previous state. + entity_entry.pose_transforms.insert(time, transforms); } if queued_update .aspects .contains(TransformAspect::PinholeOrViewCoordinates) { - if let Some(resolved_pinhole_projection) = + // `None` values need to be inserted as well to clear out previous state. + entity_entry.pinhole_projections.insert( + time, query_and_resolve_pinhole_projection_at_entity( entity_path, entity_db, &query, - ) - { - entity_entry - .pinhole_projections - .insert(time, resolved_pinhole_projection); - } + ), + ); } } } @@ -411,36 +421,36 @@ fn query_and_resolve_instance_poses_at_entity( ) } - let mut iter_translation = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_rotation_quat = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_rotation_axis_angle = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_scale = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); - let mut iter_mat3x3 = clamped_or_nothing( - result - .component_batch::() - .unwrap_or_default(), - max_count, - ); + let batch_translation = result + .component_batch::() + .unwrap_or_default(); + let batch_rotation_quat = result + .component_batch::() + .unwrap_or_default(); + let batch_rotation_axis_angle = result + .component_batch::() + .unwrap_or_default(); + let batch_scale = result + .component_batch::() + .unwrap_or_default(); + let batch_mat3x3 = result + .component_batch::() + .unwrap_or_default(); + + if batch_translation.is_empty() + && batch_rotation_quat.is_empty() + && batch_rotation_axis_angle.is_empty() + && batch_scale.is_empty() + && batch_mat3x3.is_empty() + { + return Vec::new(); + } + + let mut iter_translation = clamped_or_nothing(batch_translation, max_count); + let mut iter_rotation_quat = clamped_or_nothing(batch_rotation_quat, max_count); + let mut iter_rotation_axis_angle = clamped_or_nothing(batch_rotation_axis_angle, max_count); + let mut iter_scale = clamped_or_nothing(batch_scale, max_count); + let mut iter_mat3x3 = clamped_or_nothing(batch_mat3x3, max_count); let mut transforms = Vec::with_capacity(max_count); for _ in 0..max_count { @@ -487,7 +497,10 @@ fn query_and_resolve_pinhole_projection_at_entity( image_from_camera, view_coordinates: entity_db .latest_at_component::(entity_path, query) - .map_or(components::ViewCoordinates::RDF, |(_index, res)| res), + .map_or( + ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, + |(_index, res)| res, + ), }) } @@ -496,105 +509,317 @@ mod tests { use std::sync::Arc; use re_chunk_store::{external::re_chunk::ChunkBuilder, ChunkId, RowId}; - use re_types::archetypes; + use re_types::{archetypes, Loggable, SerializedComponentBatch}; use super::*; - #[test] - fn test_tree_transforms() { - let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + fn ensure_subscriber_registered(entity_db: &EntityDb) { TransformCacheStoreSubscriber::access(&entity_db.store_id(), |_| { // Make sure the subscriber is registered. }); + } + + #[test] + fn test_transforms_per_timeline_access() { + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); // Log a few tree transforms at different times. let timeline = Timeline::new_sequence("t"); - entity_db - .add_chunk(&Arc::new( - ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) - .with_archetype( - RowId::new(), - [(timeline, 1)], - &archetypes::Transform3D::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)), - ) - .with_component_batch( - RowId::new(), - [(timeline, 3)], - // Don't clear out existing translation. - &[components::Scale3D(glam::Vec3::new(1.0, 2.0, 3.0).into())], - ) - .with_archetype( - RowId::new(), - [(timeline, 4)], - // Clears out previous translation & scale. - //&archetypes::Transform3D::from_rotation(glam::Quat::from_rotation_x(1.0)), - &archetypes::Transform3D::from_translation(glam::Vec3::new( - 123.0, 2.0, 3.0, - )), - ) - .build() - .unwrap(), - )) + let chunk0 = ChunkBuilder::new(ChunkId::new(), EntityPath::from("with_transform")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Transform3D::from_translation([1.0, 2.0, 3.0]), + ) + .build() .unwrap(); + let chunk1 = ChunkBuilder::new(ChunkId::new(), EntityPath::from("without_transform")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + // Anything that doesn't have components the transform cache is interested in. + &archetypes::Points3D::new([[1.0, 2.0, 3.0]]), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk0)).unwrap(); + entity_db.add_chunk(&Arc::new(chunk1)).unwrap(); - // Check that the transform cache has the expected transforms. TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { cache.apply_all_updates(&entity_db); let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); assert!(transforms_per_timeline - .entity_transforms(EntityPath::from("not_my_entity").hash()) + .entity_transforms(EntityPath::from("without_transform").hash()) + .is_none()); + assert!(transforms_per_timeline + .entity_transforms(EntityPath::from("rando").hash()) .is_none()); + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("with_transform").hash()) + .unwrap(); + assert_eq!(transforms.timeline, timeline); + assert_eq!(transforms.tree_transforms.len(), 1); + assert_eq!(transforms.pose_transforms.len(), 0); + assert_eq!(transforms.pinhole_projections.len(), 0); + }); + } + + #[test] + fn test_tree_transforms() { + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); + // Log a few tree transforms at different times. + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Transform3D::from_translation([1.0, 2.0, 3.0]), + ) + .with_archetype( + RowId::new(), + [(timeline, 3)], + &archetypes::Transform3D::update_fields().with_scale([1.0, 2.0, 3.0]), + ) + .with_archetype( + RowId::new(), + [(timeline, 4)], + &archetypes::Transform3D::from_rotation(glam::Quat::from_rotation_x(1.0)), + ) + .with_archetype( + RowId::new(), + [(timeline, 5)], + &archetypes::Transform3D::clear_fields(), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Check that the transform cache has the expected transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); let transforms = transforms_per_timeline .entity_transforms(EntityPath::from("my_entity").hash()) .unwrap(); assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 0)), - None + glam::Affine3A::IDENTITY ); assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 1)), - Some(&glam::Affine3A::from_translation(glam::Vec3::new( - 1.0, 2.0, 3.0 - ))) + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)) ); assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 2)), - Some(&glam::Affine3A::from_translation(glam::Vec3::new( - 1.0, 2.0, 3.0 - ))) + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)) ); assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 3)), - Some(&glam::Affine3A::from_scale_rotation_translation( + glam::Affine3A::from_scale_rotation_translation( glam::Vec3::new(1.0, 2.0, 3.0), glam::Quat::IDENTITY, glam::Vec3::new(1.0, 2.0, 3.0), - )) + ) ); assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 4)), - // Some(&glam::Affine3A::from_quat(glam::Quat::from_rotation_x(1.0))) - Some(&glam::Affine3A::from_translation(glam::Vec3::new( - 123.0, 2.0, 3.0 - ))) + glam::Affine3A::from_quat(glam::Quat::from_rotation_x(1.0)) ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 5)), + glam::Affine3A::IDENTITY + ) }); } #[test] fn test_pose_transforms() { - // TODO: + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); + + // Log a few tree transforms at different times. + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::InstancePoses3D::new().with_translations([ + [1.0, 2.0, 3.0], + [4.0, 5.0, 6.0], + [7.0, 8.0, 9.0], + ]), + ) + .with_archetype( + RowId::new(), + [(timeline, 3)], + // Less instances, and a splatted scale. + &archetypes::InstancePoses3D::new() + .with_translations([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]]) + .with_scales([[2.0, 3.0, 4.0]]), + ) + .with_serialized_batches( + RowId::new(), + [(timeline, 4)], + [ + SerializedComponentBatch::new( + arrow::array::new_empty_array(&components::Translation3D::arrow_datatype()), + archetypes::InstancePoses3D::descriptor_translations(), + ), + SerializedComponentBatch::new( + arrow::array::new_empty_array(&components::Scale3D::arrow_datatype()), + archetypes::InstancePoses3D::descriptor_scales(), + ), + ], + ) + // TODO(#7245): Use this instead of the above + // .with_archetype( + // RowId::new(), + // [(timeline, 4)], + // &archetypes::InstancePoses3D::clear_fields(), + // ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Check that the transform cache has the expected transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("my_entity").hash()) + .unwrap(); + + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 0)), + &[] + ); + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 1)), + &[ + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)), + glam::Affine3A::from_translation(glam::Vec3::new(4.0, 5.0, 6.0)), + glam::Affine3A::from_translation(glam::Vec3::new(7.0, 8.0, 9.0)), + ] + ); + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 2)), + &[ + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)), + glam::Affine3A::from_translation(glam::Vec3::new(4.0, 5.0, 6.0)), + glam::Affine3A::from_translation(glam::Vec3::new(7.0, 8.0, 9.0)), + ] + ); + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 3)), + &[ + glam::Affine3A::from_scale_rotation_translation( + glam::Vec3::new(2.0, 3.0, 4.0), + glam::Quat::IDENTITY, + glam::Vec3::new(1.0, 2.0, 3.0), + ), + glam::Affine3A::from_scale_rotation_translation( + glam::Vec3::new(2.0, 3.0, 4.0), + glam::Quat::IDENTITY, + glam::Vec3::new(4.0, 5.0, 6.0), + ), + ] + ); + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 4)), + &[] + ); + }); } #[test] fn test_pinhole_projections() { - // TODO: + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); + + let image_from_camera = + components::PinholeProjection::from_focal_length_and_principal_point( + [1.0, 2.0], + [1.0, 2.0], + ); + + // Log a few tree transforms at different times. + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Pinhole::new(image_from_camera), + ) + .with_archetype( + RowId::new(), + [(timeline, 3)], + &archetypes::ViewCoordinates::BLU, + ) + // Clear out the pinhole projection (this should yield nothing then for the remaining view coordinates.) + .with_serialized_batch( + RowId::new(), + [(timeline, 4)], + SerializedComponentBatch::new( + arrow::array::new_empty_array(&components::PinholeProjection::arrow_datatype()), + archetypes::Pinhole::descriptor_image_from_camera(), + ), + ) + // TODO(#7245): Use this instead + // .with_archetype( + // RowId::new(), + // [(timeline, 4)], + // &archetypes::Pinhole::clear_fields(), + // ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Check that the transform cache has the expected transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("my_entity").hash()) + .unwrap(); + + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 0)), + None + ); + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 1)), + Some(&ResolvedPinholeProjection { + image_from_camera, + view_coordinates: ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, + }) + ); + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 2)), + Some(&ResolvedPinholeProjection { + image_from_camera, + view_coordinates: ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, + }) + ); + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 3)), + Some(&ResolvedPinholeProjection { + image_from_camera, + view_coordinates: components::ViewCoordinates::BLU, + }) + ); + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 4)), + None // View coordinates alone doesn't give us a pinhole projection from the transform cache. + ); + }); } #[test] - fn test_invalidation() { + fn test_out_of_order_updates() { // TODO: } } From 71937de21bc4b944bb0e488d9a4a01bcd0566329 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 14 Jan 2025 18:53:20 +0100 Subject: [PATCH 16/31] add test for out of order updates on transform cache --- .../re_view_spatial/src/transform_cache.rs | 95 ++++++++++++++++++- 1 file changed, 94 insertions(+), 1 deletion(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 0438cc7165e8..9f5e7591d02b 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -632,6 +632,10 @@ mod tests { assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 5)), glam::Affine3A::IDENTITY + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 123)), + glam::Affine3A::IDENTITY ) }); } @@ -732,6 +736,10 @@ mod tests { transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 4)), &[] ); + assert_eq!( + transforms.latest_at_instance_poses(&LatestAtQuery::new(timeline, 123)), + &[] + ); }); } @@ -815,11 +823,96 @@ mod tests { transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 4)), None // View coordinates alone doesn't give us a pinhole projection from the transform cache. ); + assert_eq!( + transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 123)), + None + ); }); } #[test] fn test_out_of_order_updates() { - // TODO: + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); + + // Log a few tree transforms at different times. + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Transform3D::from_translation([1.0, 2.0, 3.0]), + ) + .with_archetype( + RowId::new(), + [(timeline, 3)], + // Note that this doesn't clear anything that could be inserted at time 2. + &archetypes::Transform3D::update_fields().with_translation([2.0, 3.0, 4.0]), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Check that the transform cache has the expected transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("my_entity").hash()) + .unwrap(); + + // Check that the transform cache has the expected transforms. + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 1)), + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 3)), + glam::Affine3A::from_translation(glam::Vec3::new(2.0, 3.0, 4.0)) + ); + }); + + // Add a transform between the two that invalidates the one at time stamp 3. + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity")) + .with_archetype( + RowId::new(), + [(timeline, 2)], + &archetypes::Transform3D::update_fields().with_scale([-1.0, -2.0, -3.0]), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Check that the transform cache has the expected changed transforms. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + let transforms_per_timeline = cache.transforms_per_timeline(timeline).unwrap(); + let transforms = transforms_per_timeline + .entity_transforms(EntityPath::from("my_entity").hash()) + .unwrap(); + + // Check that the transform cache has the expected transforms. + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 1)), + glam::Affine3A::from_translation(glam::Vec3::new(1.0, 2.0, 3.0)) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 2)), + glam::Affine3A::from_scale_rotation_translation( + glam::Vec3::new(-1.0, -2.0, -3.0), + glam::Quat::IDENTITY, + glam::Vec3::new(1.0, 2.0, 3.0), + ) + ); + assert_eq!( + transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 3)), + glam::Affine3A::from_scale_rotation_translation( + glam::Vec3::new(-1.0, -2.0, -3.0), + glam::Quat::IDENTITY, + glam::Vec3::new(2.0, 3.0, 4.0), + ) + ); + }); } } From a254f8087526029b1db15013b875c5c36a0a5715 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 14 Jan 2025 20:30:10 +0100 Subject: [PATCH 17/31] typos --- .../src/contexts/transform_tree_context.rs | 10 +++++----- crates/viewer/re_view_spatial/src/transform_cache.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs index ebf02596994f..b6ef18c89be7 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs @@ -110,21 +110,21 @@ impl TransformInfo { /// Provides transforms from an entity to a chosen reference space for all elements in the scene /// for the currently selected time & timeline. /// -/// The resulting transforms are are dependent on: +/// The resulting transforms are dependent on: /// * tree, pose, pinhole and view-coordinates transforms components as logged to the data store /// * TODO(#6743): blueprint overrides aren't respected yet /// * the view' spatial origin /// * the query time /// * TODO(#723): ranges aren't taken into account yet /// * TODO(andreas): the queried entities. Right now we determine transforms for ALL entities in the scene. -/// since 3D views tend to display almost everything that's mostly fine, but it's very very wasteful when they don't. +/// since 3D views tend to display almost everything that's mostly fine, but it's very wasteful when they don't. /// /// The renderer then uses this reference space as its world space, /// making world and reference space equivalent for a given view. /// /// TODO(#7025): Right now we also do full tree traversal in here to resolve transforms to the root. /// However, for views that share the same query, we can easily make all entities relative to the respective origin in a linear pass over all matrices. -/// (Note that right now the query IS always the same accross all views for a given frame since it's just latest-at controlled by the timeline, +/// (Note that right now the query IS always the same across all views for a given frame since it's just latest-at controlled by the timeline, /// but once we support range queries it may be not or only partially the case) #[derive(Clone)] pub struct TransformTreeContext { @@ -240,7 +240,7 @@ impl ViewContextSystem for TransformTreeContext { &time_query, transforms_per_timeline, ); - }); // Note that this can return None if no event has happend for this timeline yet. + }); // Note that this can return None if no event has happened for this timeline yet. } fn as_any(&self) -> &dyn std::any::Any { @@ -428,7 +428,7 @@ fn transform_info_for_upward_propagation( (SmallVec1::new(reference_from_entity), false) }; - // Apply tree transform if any. + // Apply tree transform. reference_from_entity *= transforms_at_entity .parent_from_entity_tree_transform .inverse(); diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 9f5e7591d02b..ec4a16419bf2 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -94,7 +94,7 @@ pub struct ResolvedPinholeProjection { } impl ResolvedPinholeProjection { - /// View coordinates used when there's no view coordinates explicitely logged. + /// View coordinates used when there's no view coordinates explicitly logged. pub const DEFAULT_VIEW_COORDINATES: components::ViewCoordinates = components::ViewCoordinates::RDF; } From a83035e59d6a65fae6856bae1272474524aa69fc Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 10:30:49 +0100 Subject: [PATCH 18/31] address some doc & style related comments --- .../src/contexts/transform_tree_context.rs | 12 ++---- .../re_view_spatial/src/transform_cache.rs | 40 +++++++++++-------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs index b6ef18c89be7..6d474bd6b525 100644 --- a/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs +++ b/crates/viewer/re_view_spatial/src/contexts/transform_tree_context.rs @@ -206,7 +206,6 @@ impl ViewContextSystem for TransformTreeContext { let Some(transforms_per_timeline) = cache.transforms_per_timeline(query.timeline) else { // No transforms on this timeline at all. In other words, everything is identity! - // TODO(andreas): why are query results a tree to begin with? We want a flat list not just here! query_result.tree.visit(&mut |node: &DataResultNode| { self.transform_per_entity.insert( node.data_result.entity_path.hash(), @@ -226,7 +225,7 @@ impl ViewContextSystem for TransformTreeContext { data_result_tree, current_tree, &time_query, - // Ignore potential pinhole camera at the root of the view, since it regarded as being "above" this root. + // Ignore potential pinhole camera at the root of the view, since it is regarded as being "above" this root. TransformInfo::default(), transforms_per_timeline, ); @@ -267,8 +266,7 @@ impl TransformTreeContext { 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 + "Path {parent_path} is not part of the global entity tree whereas its child is" ); return; }; @@ -379,8 +377,6 @@ fn lookup_image_plane_distance( entity_path: &EntityPath, query: &LatestAtQuery, ) -> f32 { - re_tracing::profile_function!(); - data_result_tree .lookup_result_by_path(entity_path) .cloned() @@ -550,7 +546,7 @@ But they are instead ordered like this:\n{actual_order:?}" #[cfg(not(debug_assertions))] fn debug_assert_transform_field_order(_: &re_types::reflection::Reflection) {} -fn pinhole_with_image_plane_to_transform( +fn transform_from_pinhole_with_image_plane( entity_path: &EntityPath, resolved_pinhole_projection: &ResolvedPinholeProjection, pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32, @@ -624,7 +620,7 @@ fn transforms_at<'a>( entity_transforms .latest_at_pinhole(query) .map(|resolved_pinhole_projection| { - pinhole_with_image_plane_to_transform( + transform_from_pinhole_with_image_plane( entity_path, resolved_pinhole_projection, pinhole_image_plane_distance, diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index ec4a16419bf2..3dc8384988cc 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -11,14 +11,20 @@ use re_chunk_store::{ use re_entity_db::EntityDb; use re_log_types::{EntityPath, EntityPathHash, StoreId, TimeInt, Timeline}; use re_types::{ + archetypes::{self}, components::{self}, Archetype as _, Component, ComponentName, }; /// Store subscriber that resolves all transform components at a given entity to an affine transform. pub struct TransformCacheStoreSubscriber { + /// All components of [`archetypes::Transform3D`] transform_components: IntSet, + + /// All components of [`archetypes::InstancePoses3D`] pose_components: IntSet, + + /// All components related to pinholes (i.e. [`components::PinholeProjection`] and [`components::ViewCoordinates`]). pinhole_components: IntSet, per_timeline: HashMap, @@ -30,11 +36,11 @@ impl Default for TransformCacheStoreSubscriber { use re_types::Archetype as _; Self { - transform_components: re_types::archetypes::Transform3D::all_components() + transform_components: archetypes::Transform3D::all_components() .iter() .map(|descr| descr.component_name) .collect(), - pose_components: re_types::archetypes::InstancePoses3D::all_components() + pose_components: archetypes::InstancePoses3D::all_components() .iter() .map(|descr| descr.component_name) .collect(), @@ -328,7 +334,7 @@ fn query_and_resolve_tree_transform_at_entity( query: &LatestAtQuery, ) -> Option { // TODO(andreas): Filter out the components we're actually interested in? - let components = re_types::archetypes::Transform3D::all_components(); + let components = archetypes::Transform3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); let result = entity_db.latest_at(query, entity_path, component_names); if result.components.is_empty() { @@ -389,18 +395,18 @@ fn query_and_resolve_instance_poses_at_entity( query: &LatestAtQuery, ) -> Vec { // TODO(andreas): Filter out the components we're actually interested in? - let components = re_types::archetypes::InstancePoses3D::all_components(); + let components = archetypes::InstancePoses3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); let result = entity_db.latest_at(query, entity_path, component_names); - let max_count = result + let max_num_instances = result .components .iter() .map(|(name, row)| row.num_instances(name)) .max() .unwrap_or(0) as usize; - if max_count == 0 { + if max_num_instances == 0 { return Vec::new(); } @@ -445,16 +451,16 @@ fn query_and_resolve_instance_poses_at_entity( { return Vec::new(); } - - let mut iter_translation = clamped_or_nothing(batch_translation, max_count); - let mut iter_rotation_quat = clamped_or_nothing(batch_rotation_quat, max_count); - let mut iter_rotation_axis_angle = clamped_or_nothing(batch_rotation_axis_angle, max_count); - let mut iter_scale = clamped_or_nothing(batch_scale, max_count); - let mut iter_mat3x3 = clamped_or_nothing(batch_mat3x3, max_count); - - let mut transforms = Vec::with_capacity(max_count); - for _ in 0..max_count { - // Order see `debug_assert_transform_field_order` + let mut iter_translation = clamped_or_nothing(batch_translation, max_num_instances); + let mut iter_rotation_quat = clamped_or_nothing(batch_rotation_quat, max_num_instances); + let mut iter_rotation_axis_angle = + clamped_or_nothing(batch_rotation_axis_angle, max_num_instances); + let mut iter_scale = clamped_or_nothing(batch_scale, max_num_instances); + let mut iter_mat3x3 = clamped_or_nothing(batch_mat3x3, max_num_instances); + + let mut transforms = Vec::with_capacity(max_num_instances); + for _ in 0..max_num_instances { + // We apply these in a specific order - see `debug_assert_transform_field_order` let mut transform = glam::Affine3A::IDENTITY; if let Some(translation) = iter_translation.next() { transform = glam::Affine3A::from(translation); @@ -636,7 +642,7 @@ mod tests { assert_eq!( transforms.latest_at_tree_transform(&LatestAtQuery::new(timeline, 123)), glam::Affine3A::IDENTITY - ) + ); }); } From 625c5844b05878d28c00f8233c1220ef6c67cf51 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 10:33:29 +0100 Subject: [PATCH 19/31] it ain't no queue! --- .../re_view_spatial/src/transform_cache.rs | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 3dc8384988cc..d81745dde93f 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -60,8 +60,13 @@ bitflags::bitflags! { /// Flags for the different kinds of independent transforms that the transform cache handles. #[derive(Debug, Clone, Copy)] pub struct TransformAspect: u8 { + /// The entity has a tree transform, i.e. any non-style component of [`archetypes::Transform3D`]. const Tree = 1 << 0; + + /// The entity has instance poses, i.e. any non-style component of [`archetypes::InstancePoses3D`]. const Pose = 1 << 1; + + /// The entity has a pinhole projection or view coordinates, i.e. either [`components::PinholeProjection`] or [`components::ViewCoordinates`]. const PinholeOrViewCoordinates = 1 << 2; } } @@ -69,7 +74,7 @@ bitflags::bitflags! { /// Points in time that have changed for a given entity, /// i.e. the cache is invalid for these times. #[derive(Debug)] -struct QueuedTransformUpdates { +struct InvalidatedTransforms { entity_path: EntityPath, times: Vec, aspects: TransformAspect, @@ -78,7 +83,7 @@ struct QueuedTransformUpdates { pub struct CachedTransformsPerTimeline { /// Updates that should be applied to the cache. /// I.e. times & entities at which the cache is invalid right now. - queued_updates: Vec, + invalidated_transforms: Vec, per_entity: IntMap, } @@ -185,7 +190,7 @@ impl TransformCacheStoreSubscriber { re_tracing::profile_function!(); for (timeline, per_timeline) in &mut self.per_timeline { - for queued_update in per_timeline.queued_updates.drain(..) { + for queued_update in per_timeline.invalidated_transforms.drain(..) { let entity_path = &queued_update.entity_path; let entity_entry = per_timeline .per_entity @@ -280,7 +285,7 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { // But that's fairly rare, so a few false positive entries here are fine. let per_timeline = self.per_timeline.entry(*timeline).or_insert_with(|| { CachedTransformsPerTimeline { - queued_updates: Default::default(), + invalidated_transforms: Default::default(), per_entity: Default::default(), } }); @@ -315,14 +320,16 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { } } - per_timeline.queued_updates.push(QueuedTransformUpdates { - entity_path: entity_path.clone(), - times: time_column - .times() - .chain(invalidated_times.into_iter()) - .collect(), - aspects, - }); + per_timeline + .invalidated_transforms + .push(InvalidatedTransforms { + entity_path: entity_path.clone(), + times: time_column + .times() + .chain(invalidated_times.into_iter()) + .collect(), + aspects, + }); } } } From cb30d710f69909fb3780019c1abe38b9cfcc2f74 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 10:52:30 +0100 Subject: [PATCH 20/31] more docs, Box pose & pinhole to save some memory --- .../re_view_spatial/src/transform_cache.rs | 93 +++++++++++++------ 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index d81745dde93f..b9374958f981 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -17,6 +17,17 @@ use re_types::{ }; /// Store subscriber that resolves all transform components at a given entity to an affine transform. +/// +/// It only handles resulting transforms individually to each entity, not how these transforms propagate in the tree. +/// For transform tree propagation see [`crate::contexts::TransformTreeContext`]. +/// +/// There are different kinds of transforms handled here: +/// * [`archetypes::Transform3D`] +/// Tree transforms that should propagate in the tree (via [`crate::contexts::TransformTreeContext`]). +/// * [`archetypes::InstancePoses3D`] +/// Instance poses that should be applied to the tree transforms (via [`crate::contexts::TransformTreeContext`]) but not propagate. +/// * [`components::PinholeProjection`] and [`components::ViewCoordinates`] +/// Pinhole projections & associated view coordinates used for visualizing cameras in 3D and embedding 2D in 3D pub struct TransformCacheStoreSubscriber { /// All components of [`archetypes::Transform3D`] transform_components: IntSet, @@ -88,19 +99,35 @@ pub struct CachedTransformsPerTimeline { per_entity: IntMap, } +type PoseTransformMap = BTreeMap>; + +/// Maps from time to pinhole projection. +/// +/// Unlike with tree & pose transforms, there's identity value that we can insert upon clears. +/// (clears here meaning that the user first logs a pinhole and then later either logs a clear or an empty pinhole array) +/// Therefore, we instead store those events as `None` values to ensure that everything after a clear +/// is properly marked as having no pinhole projection. +type PinholeProjectionMap = BTreeMap>; + pub struct PerTimelinePerEntityTransforms { timeline: Timeline, tree_transforms: BTreeMap, - pose_transforms: BTreeMap>, - // Note that pinhole projections are fairly rare - it's worth considering storing them separately so we don't have this around for every entity. - // The flipside of that is of course that we'd have to do more lookups (unless we come up with a way to linearly iterate them) - pinhole_projections: BTreeMap>, + + // Pose transforms and pinhole projections are typically more rare, which is why we store them as optional boxes. + pose_transforms: Option>, + pinhole_projections: Option>, } #[derive(Clone, Debug, PartialEq)] pub struct ResolvedPinholeProjection { pub image_from_camera: components::PinholeProjection, + + /// View coordinates at this pinhole camera. + /// + /// This is needed to orient 2D in 3D and 3D in 2D the right way around + /// (answering questions like which axis is distance to viewer increasing). + /// If no view coordinates were logged, this is set to [`Self::DEFAULT_VIEW_COORDINATES`]. pub view_coordinates: components::ViewCoordinates, } @@ -135,9 +162,9 @@ impl PerTimelinePerEntityTransforms { pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> &[glam::Affine3A] { debug_assert!(query.timeline() == self.timeline); self.pose_transforms - .range(..query.at().inc()) - .next_back() - .map(|(_time, transform)| transform.as_slice()) + .as_ref() + .and_then(|pose_transforms| pose_transforms.range(..query.at().inc()).next_back()) + .map(|(_time, pose_transforms)| pose_transforms.as_slice()) .unwrap_or(&[]) } @@ -145,9 +172,11 @@ impl PerTimelinePerEntityTransforms { pub fn latest_at_pinhole(&self, query: &LatestAtQuery) -> Option<&ResolvedPinholeProjection> { debug_assert!(query.timeline() == self.timeline); self.pinhole_projections - .range(..query.at().inc()) - .next_back() - .and_then(|(_time, transform)| transform.as_ref()) + .as_ref() + .and_then(|pinhole_projections| { + pinhole_projections.range(..query.at().inc()).next_back() + }) + .and_then(|(_time, projection)| projection.as_ref()) } } @@ -226,21 +255,28 @@ impl TransformCacheStoreSubscriber { &query, ); // *do* also insert empty ones, otherwise it's not possible to clear previous state. - entity_entry.pose_transforms.insert(time, transforms); + entity_entry + .pose_transforms + .get_or_insert_with(Box::default) + .insert(time, transforms); } if queued_update .aspects .contains(TransformAspect::PinholeOrViewCoordinates) { // `None` values need to be inserted as well to clear out previous state. - entity_entry.pinhole_projections.insert( - time, - query_and_resolve_pinhole_projection_at_entity( - entity_path, - entity_db, - &query, - ), - ); + // See also doc string on `PinholeProjectionMap`. + entity_entry + .pinhole_projections + .get_or_insert_with(Box::default) + .insert( + time, + query_and_resolve_pinhole_projection_at_entity( + entity_path, + entity_db, + &query, + ), + ); } } } @@ -309,14 +345,17 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { invalidated_times.extend(invalidated_tree_transforms.into_keys()); } if aspects.contains(TransformAspect::Pose) { - let invalidated_pose_transforms = - entity_entry.pose_transforms.split_off(&min_time); - invalidated_times.extend(invalidated_pose_transforms.into_keys()); + if let Some(pose_transforms) = &mut entity_entry.pose_transforms { + let invalidated_pose_transforms = pose_transforms.split_off(&min_time); + invalidated_times.extend(invalidated_pose_transforms.into_keys()); + } } if aspects.contains(TransformAspect::PinholeOrViewCoordinates) { - let invalidated_pinhole_projections = - entity_entry.pinhole_projections.split_off(&min_time); - invalidated_times.extend(invalidated_pinhole_projections.into_keys()); + if let Some(pinhole_projections) = &mut entity_entry.pinhole_projections { + let invalidated_pinhole_projections = + pinhole_projections.split_off(&min_time); + invalidated_times.extend(invalidated_pinhole_projections.into_keys()); + } } } @@ -573,8 +612,8 @@ mod tests { .unwrap(); assert_eq!(transforms.timeline, timeline); assert_eq!(transforms.tree_transforms.len(), 1); - assert_eq!(transforms.pose_transforms.len(), 0); - assert_eq!(transforms.pinhole_projections.len(), 0); + assert_eq!(transforms.pose_transforms, None); + assert_eq!(transforms.pinhole_projections, None); }); } From 6950a7010637fd83d67cb86e886a5d9e0c1c080b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:16:38 +0100 Subject: [PATCH 21/31] Move default view coordinates constant to Pinhole --- .../store/re_types/src/archetypes/pinhole_ext.rs | 10 +++++++++- .../viewer/re_view_spatial/src/transform_cache.rs | 15 +++------------ crates/viewer/re_view_spatial/src/ui_2d.rs | 3 +-- .../re_view_spatial/src/visualizers/cameras.rs | 10 ++++++++-- .../src/visualizers/depth_images.rs | 3 +-- .../viewer/re_view_spatial/src/visualizers/mod.rs | 6 +----- 6 files changed, 23 insertions(+), 24 deletions(-) diff --git a/crates/store/re_types/src/archetypes/pinhole_ext.rs b/crates/store/re_types/src/archetypes/pinhole_ext.rs index 787e2b932837..dba2076ef46b 100644 --- a/crates/store/re_types/src/archetypes/pinhole_ext.rs +++ b/crates/store/re_types/src/archetypes/pinhole_ext.rs @@ -1,8 +1,16 @@ -use crate::datatypes::Vec2D; +use crate::{components::ViewCoordinates, datatypes::Vec2D}; use super::Pinhole; impl Pinhole { + /// Camera orientation used when there's no camera orientation explicitly logged. + /// + /// - x pointing right + /// - y pointing down + /// - z pointing into the image plane + /// (this is convenient for reading out a depth image which has typically positive z values) + pub const DEFAULT_CAMERA_XYZ: ViewCoordinates = ViewCoordinates::RDF; + /// Creates a pinhole from the camera focal length and resolution, both specified in pixels. /// /// The focal length is the diagonal of the projection matrix. diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index b9374958f981..e0b5ebf6745d 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -131,12 +131,6 @@ pub struct ResolvedPinholeProjection { pub view_coordinates: components::ViewCoordinates, } -impl ResolvedPinholeProjection { - /// View coordinates used when there's no view coordinates explicitly logged. - pub const DEFAULT_VIEW_COORDINATES: components::ViewCoordinates = - components::ViewCoordinates::RDF; -} - impl CachedTransformsPerTimeline { #[inline] pub fn entity_transforms( @@ -549,10 +543,7 @@ fn query_and_resolve_pinhole_projection_at_entity( image_from_camera, view_coordinates: entity_db .latest_at_component::(entity_path, query) - .map_or( - ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, - |(_index, res)| res, - ), + .map_or(archetypes::Pinhole::DEFAULT_CAMERA_XYZ, |(_index, res)| res), }) } @@ -854,14 +845,14 @@ mod tests { transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 1)), Some(&ResolvedPinholeProjection { image_from_camera, - view_coordinates: ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, + view_coordinates: archetypes::Pinhole::DEFAULT_CAMERA_XYZ, }) ); assert_eq!( transforms.latest_at_pinhole(&LatestAtQuery::new(timeline, 2)), Some(&ResolvedPinholeProjection { image_from_camera, - view_coordinates: ResolvedPinholeProjection::DEFAULT_VIEW_COORDINATES, + view_coordinates: archetypes::Pinhole::DEFAULT_CAMERA_XYZ, }) ); assert_eq!( diff --git a/crates/viewer/re_view_spatial/src/ui_2d.rs b/crates/viewer/re_view_spatial/src/ui_2d.rs index 3104e685c88b..a77dc8c6df91 100644 --- a/crates/viewer/re_view_spatial/src/ui_2d.rs +++ b/crates/viewer/re_view_spatial/src/ui_2d.rs @@ -10,7 +10,6 @@ use re_types::{ archetypes::{Background, NearClipPlane, VisualBounds2D}, components as blueprint_components, }, - components::ViewCoordinates, }; use re_ui::{ContextExt as _, ModifiersMarkdown, MouseButtonMarkdown}; use re_view::controls::{DRAG_PAN2D_BUTTON, ZOOM_SCROLL_MODIFIER}; @@ -353,7 +352,7 @@ fn setup_target_config( ) .into(), resolution: Some([resolution.x, resolution.y].into()), - camera_xyz: Some(ViewCoordinates::RDF), + camera_xyz: Some(Pinhole::DEFAULT_CAMERA_XYZ), image_plane_distance: None, }; } diff --git a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs index 64174545f520..834d90f90daf 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/cameras.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/cameras.rs @@ -237,7 +237,7 @@ impl VisualizerSystem for CamerasVisualizer { transforms, data_result, &pinhole, - pinhole.camera_xyz.unwrap_or(ViewCoordinates::RDF), // TODO(#2641): This should come from archetype + pinhole.camera_xyz.unwrap_or(Pinhole::DEFAULT_CAMERA_XYZ), entity_highlight, ); } @@ -280,4 +280,10 @@ impl TypedComponentFallbackProvider for CamerasVisualizer { } } -re_viewer_context::impl_component_fallback_provider!(CamerasVisualizer => [ImagePlaneDistance]); +impl TypedComponentFallbackProvider for CamerasVisualizer { + fn fallback_for(&self, _ctx: &QueryContext<'_>) -> ViewCoordinates { + Pinhole::DEFAULT_CAMERA_XYZ + } +} + +re_viewer_context::impl_component_fallback_provider!(CamerasVisualizer => [ImagePlaneDistance, ViewCoordinates]); diff --git a/crates/viewer/re_view_spatial/src/visualizers/depth_images.rs b/crates/viewer/re_view_spatial/src/visualizers/depth_images.rs index 652eafbdb38f..3b946caefd14 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/depth_images.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/depth_images.rs @@ -7,7 +7,6 @@ use re_types::{ archetypes::DepthImage, components::{ self, Colormap, DepthMeter, DrawOrder, FillRatio, ImageBuffer, ImageFormat, ValueRange, - ViewCoordinates, }, image::ImageKind, Component as _, @@ -190,7 +189,7 @@ impl DepthImageVisualizer { * glam::Affine3A::from_mat3( intrinsics .camera_xyz - .unwrap_or(ViewCoordinates::RDF) // TODO(#2641): This should come from archetype + .unwrap_or(re_types::archetypes::Pinhole::DEFAULT_CAMERA_XYZ) .from_rdf(), ); diff --git a/crates/viewer/re_view_spatial/src/visualizers/mod.rs b/crates/viewer/re_view_spatial/src/visualizers/mod.rs index b1b5ea34d38f..0bf2cb4b8297 100644 --- a/crates/viewer/re_view_spatial/src/visualizers/mod.rs +++ b/crates/viewer/re_view_spatial/src/visualizers/mod.rs @@ -267,11 +267,7 @@ pub fn load_keypoint_connections( /// /// TODO(#1387): Image coordinate space should be configurable. pub fn image_view_coordinates() -> re_types::components::ViewCoordinates { - // Typical image spaces have - // - x pointing right - // - y pointing down - // - z pointing into the image plane (this is convenient for reading out a depth image which has typically positive z values) - re_types::components::ViewCoordinates::RDF + re_types::archetypes::Pinhole::DEFAULT_CAMERA_XYZ } fn filter_visualizable_2d_entities( From fb4efad72032c4adc3a51a93dab01b7843f0807c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:21:10 +0100 Subject: [PATCH 22/31] more comment & structure improvements --- .../re_view_spatial/src/transform_cache.rs | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index e0b5ebf6745d..6a65e3e0646b 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -144,7 +144,7 @@ impl CachedTransformsPerTimeline { impl PerTimelinePerEntityTransforms { #[inline] pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> glam::Affine3A { - debug_assert!(query.timeline() == self.timeline); + debug_assert_eq!(query.timeline(), self.timeline); self.tree_transforms .range(..query.at().inc()) .next_back() @@ -154,7 +154,7 @@ impl PerTimelinePerEntityTransforms { #[inline] pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> &[glam::Affine3A] { - debug_assert!(query.timeline() == self.timeline); + debug_assert_eq!(query.timeline(), self.timeline); self.pose_transforms .as_ref() .and_then(|pose_transforms| pose_transforms.range(..query.at().inc()).next_back()) @@ -164,7 +164,7 @@ impl PerTimelinePerEntityTransforms { #[inline] pub fn latest_at_pinhole(&self, query: &LatestAtQuery) -> Option<&ResolvedPinholeProjection> { - debug_assert!(query.timeline() == self.timeline); + debug_assert_eq!(query.timeline(), self.timeline); self.pinhole_projections .as_ref() .and_then(|pinhole_projections| { @@ -208,13 +208,14 @@ impl TransformCacheStoreSubscriber { /// Makes sure the transform cache is up to date with the latest data. /// - /// This needs to be called once per frame. + /// This needs to be called once per frame prior to any transform propagation. + /// (which is done by [`crate::contexts::TransformTreeContext`]) pub fn apply_all_updates(&mut self, entity_db: &EntityDb) { re_tracing::profile_function!(); for (timeline, per_timeline) in &mut self.per_timeline { - for queued_update in per_timeline.invalidated_transforms.drain(..) { - let entity_path = &queued_update.entity_path; + for invalidated_transform in per_timeline.invalidated_transforms.drain(..) { + let entity_path = &invalidated_transform.entity_path; let entity_entry = per_timeline .per_entity .entry(entity_path.hash()) @@ -225,24 +226,28 @@ impl TransformCacheStoreSubscriber { pinhole_projections: Default::default(), }); - for time in queued_update.times { + for time in invalidated_transform.times { let query = LatestAtQuery::new(*timeline, time); - if queued_update.aspects.contains(TransformAspect::Tree) { - if let Some(transform) = query_and_resolve_tree_transform_at_entity( + if invalidated_transform + .aspects + .contains(TransformAspect::Tree) + { + let transform = query_and_resolve_tree_transform_at_entity( entity_path, entity_db, &query, - ) { - entity_entry.tree_transforms.insert(time, transform); - } else { - // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! - entity_entry - .tree_transforms - .insert(time, glam::Affine3A::IDENTITY); - } + ) + // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! + .unwrap_or(glam::Affine3A::IDENTITY); + entity_entry + .tree_transforms + .insert(time, glam::Affine3A::IDENTITY); } - if queued_update.aspects.contains(TransformAspect::Pose) { + if invalidated_transform + .aspects + .contains(TransformAspect::Pose) + { let transforms = query_and_resolve_instance_poses_at_entity( entity_path, entity_db, @@ -254,23 +259,21 @@ impl TransformCacheStoreSubscriber { .get_or_insert_with(Box::default) .insert(time, transforms); } - if queued_update + if invalidated_transform .aspects .contains(TransformAspect::PinholeOrViewCoordinates) { + let pinhole_projection = query_and_resolve_pinhole_projection_at_entity( + entity_path, + entity_db, + &query, + ); // `None` values need to be inserted as well to clear out previous state. // See also doc string on `PinholeProjectionMap`. entity_entry .pinhole_projections .get_or_insert_with(Box::default) - .insert( - time, - query_and_resolve_pinhole_projection_at_entity( - entity_path, - entity_db, - &query, - ), - ); + .insert(time, pinhole_projection); } } } From e330fe9008281f97177588f508c1f07e31e8cb24 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:21:16 +0100 Subject: [PATCH 23/31] fix wrong subscriber name --- crates/viewer/re_view_spatial/src/transform_cache.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 6a65e3e0646b..fb10e97581c9 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -238,17 +238,15 @@ impl TransformCacheStoreSubscriber { entity_db, &query, ) - // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! .unwrap_or(glam::Affine3A::IDENTITY); - entity_entry - .tree_transforms - .insert(time, glam::Affine3A::IDENTITY); + // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! + entity_entry.tree_transforms.insert(time, transform); } if invalidated_transform .aspects .contains(TransformAspect::Pose) { - let transforms = query_and_resolve_instance_poses_at_entity( + let poses = query_and_resolve_instance_poses_at_entity( entity_path, entity_db, &query, @@ -257,7 +255,7 @@ impl TransformCacheStoreSubscriber { entity_entry .pose_transforms .get_or_insert_with(Box::default) - .insert(time, transforms); + .insert(time, poses); } if invalidated_transform .aspects @@ -283,7 +281,7 @@ impl TransformCacheStoreSubscriber { impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { fn name() -> String { - "rerun.TransformResolverStoreSubscriber".to_owned() + "rerun.TransformCacheStoreSubscriber".to_owned() } fn on_events<'a>(&mut self, events: impl Iterator) { From f40d4433bfa55f4638b3fa8e875eb5d5c5a033b6 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:26:18 +0100 Subject: [PATCH 24/31] more comment & style --- .../re_view_spatial/src/transform_cache.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index fb10e97581c9..3a0c2550284e 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -91,6 +91,7 @@ struct InvalidatedTransforms { aspects: TransformAspect, } +#[derive(Default)] pub struct CachedTransformsPerTimeline { /// Updates that should be applied to the cache. /// I.e. times & entities at which the cache is invalid right now. @@ -293,16 +294,19 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { continue; } + // The components we are interested in may only show up on some of the timelines + // within this chunk, so strictly speaking the affected "aspects" we compute here are conservative. + // But that's fairly rare, so a few false positive entries here are fine. let mut aspects = TransformAspect::empty(); for component_name in event.chunk.component_names() { if self.transform_components.contains(&component_name) { - aspects.set(TransformAspect::Tree, true); + aspects |= TransformAspect::Tree; } if self.pose_components.contains(&component_name) { - aspects.set(TransformAspect::Pose, true); + aspects |= TransformAspect::Pose; } if self.pinhole_components.contains(&component_name) { - aspects.set(TransformAspect::PinholeOrViewCoordinates, true); + aspects |= TransformAspect::PinholeOrViewCoordinates; } } if aspects.is_empty() { @@ -312,14 +316,7 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { let entity_path = event.chunk.entity_path(); for (timeline, time_column) in event.diff.chunk.timelines() { - // The components we are interested in may only show up on some of the timelines. - // But that's fairly rare, so a few false positive entries here are fine. - let per_timeline = self.per_timeline.entry(*timeline).or_insert_with(|| { - CachedTransformsPerTimeline { - invalidated_transforms: Default::default(), - per_entity: Default::default(), - } - }); + let per_timeline = self.per_timeline.entry(*timeline).or_default(); // All of these require complex latest-at queries that would require a lot more context, // are fairly expensive, and may depend on other components that may come in at the same time. From c517a85c2111f2ac5487c6de25b63367e8d22621 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:47:33 +0100 Subject: [PATCH 25/31] consistently handle invalid transforms --- .../re_view_spatial/src/transform_cache.rs | 88 +++++++++++-------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 3a0c2550284e..b56fa91d27dc 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use ahash::HashMap; +use glam::Affine3A; use itertools::Either; use nohash_hasher::{IntMap, IntSet}; @@ -100,7 +101,7 @@ pub struct CachedTransformsPerTimeline { per_entity: IntMap, } -type PoseTransformMap = BTreeMap>; +type PoseTransformMap = BTreeMap>; /// Maps from time to pinhole projection. /// @@ -113,7 +114,7 @@ type PinholeProjectionMap = BTreeMap> pub struct PerTimelinePerEntityTransforms { timeline: Timeline, - tree_transforms: BTreeMap, + tree_transforms: BTreeMap, // Pose transforms and pinhole projections are typically more rare, which is why we store them as optional boxes. pose_transforms: Option>, @@ -144,17 +145,17 @@ impl CachedTransformsPerTimeline { impl PerTimelinePerEntityTransforms { #[inline] - pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> glam::Affine3A { + pub fn latest_at_tree_transform(&self, query: &LatestAtQuery) -> Affine3A { debug_assert_eq!(query.timeline(), self.timeline); self.tree_transforms .range(..query.at().inc()) .next_back() .map(|(_time, transform)| *transform) - .unwrap_or(glam::Affine3A::IDENTITY) + .unwrap_or(Affine3A::IDENTITY) } #[inline] - pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> &[glam::Affine3A] { + pub fn latest_at_instance_poses(&self, query: &LatestAtQuery) -> &[Affine3A] { debug_assert_eq!(query.timeline(), self.timeline); self.pose_transforms .as_ref() @@ -239,7 +240,7 @@ impl TransformCacheStoreSubscriber { entity_db, &query, ) - .unwrap_or(glam::Affine3A::IDENTITY); + .unwrap_or(Affine3A::IDENTITY); // If there's *no* transform, we have to put identity in, otherwise we'd miss clears! entity_entry.tree_transforms.insert(time, transform); } @@ -323,7 +324,7 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { // (we could inject that here, but it's not entirely straight forward). // So instead, we note down that the caches is invalidated for the given entity & times. - // Any time _after_ the first event in this chunk is no longer valid now. + // This invalidates any time _after_ the first event in this chunk. // (e.g. if a rotation is added prior to translations later on, // then the resulting transforms at those translations changes as well for latest-at queries) let mut invalidated_times = Vec::new(); @@ -366,11 +367,16 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { } } +/// Queries all components that are part of pose transforms, returning the transform from child to parent. +/// +/// If any of the components yields an invalid transform, returns a `glam::Affine3A::ZERO`. +/// (this effectively disconnects a subtree from the transform hierarchy!) +// TODO(andreas): There's no way to discover invalid transforms right now. fn query_and_resolve_tree_transform_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, query: &LatestAtQuery, -) -> Option { +) -> Option { // TODO(andreas): Filter out the components we're actually interested in? let components = archetypes::Transform3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); @@ -379,59 +385,69 @@ fn query_and_resolve_tree_transform_at_entity( return None; } - let mut transform = glam::Affine3A::IDENTITY; + let mut transform = Affine3A::IDENTITY; - // Order see `debug_assert_transform_field_order` + // The order of the components here is important, and checked by `debug_assert_transform_field_order` if let Some(translation) = result.component_instance::(0) { - transform = glam::Affine3A::from(translation); + transform = Affine3A::from(translation); } if let Some(axis_angle) = result.component_instance::(0) { - if let Ok(axis_angle) = glam::Affine3A::try_from(axis_angle) { + if let Ok(axis_angle) = Affine3A::try_from(axis_angle) { transform *= axis_angle; } else { - // Invalid transform. - return None; + return Some(Affine3A::ZERO); } } if let Some(quaternion) = result.component_instance::(0) { - if let Ok(quaternion) = glam::Affine3A::try_from(quaternion) { + if let Ok(quaternion) = Affine3A::try_from(quaternion) { transform *= quaternion; } else { - // Invalid transform. - return None; + return Some(Affine3A::ZERO); } } if let Some(scale) = result.component_instance::(0) { if scale.x() == 0.0 && scale.y() == 0.0 && scale.z() == 0.0 { - // Invalid scale. - return None; + return Some(Affine3A::ZERO); } - transform *= glam::Affine3A::from(scale); + transform *= Affine3A::from(scale); } if let Some(mat3x3) = result.component_instance::(0) { - let affine_transform = glam::Affine3A::from(mat3x3); + let affine_transform = Affine3A::from(mat3x3); if affine_transform.matrix3.determinant() == 0.0 { - // Invalid transform. - return None; + return Some(Affine3A::ZERO); } transform *= affine_transform; } - if result.component_instance::(0) == Some(components::TransformRelation::ChildFromParent) - // TODO(andreas): Should we warn? - && transform.matrix3.determinant() != 0.0 + if result.component_instance::(0) + == Some(components::TransformRelation::ChildFromParent) { - transform = transform.inverse(); + let determinant = transform.matrix3.determinant(); + if determinant != 0.0 && determinant.is_finite() { + transform = transform.inverse(); + } else { + // All "regular invalid" transforms should have been caught. + // So ending up here means something else went wrong? + re_log::warn_once!( + "Failed to express child-from-parent transform at {} since it wasn't invertible", + entity_path, + ); + } } Some(transform) } +/// Queries all components that are part of pose transforms, returning the transform from child to parent. +/// +/// If any of the components yields an invalid transform, returns a `glam::Affine3A::ZERO` for that instance. +/// (this effectively ignores the instance for most visualizations!) +// TODO(andreas): There's no way to discover invalid transforms right now. fn query_and_resolve_instance_poses_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, query: &LatestAtQuery, -) -> Vec { +) -> Vec { // TODO(andreas): Filter out the components we're actually interested in? let components = archetypes::InstancePoses3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); @@ -499,29 +515,29 @@ fn query_and_resolve_instance_poses_at_entity( let mut transforms = Vec::with_capacity(max_num_instances); for _ in 0..max_num_instances { // We apply these in a specific order - see `debug_assert_transform_field_order` - let mut transform = glam::Affine3A::IDENTITY; + let mut transform = Affine3A::IDENTITY; if let Some(translation) = iter_translation.next() { - transform = glam::Affine3A::from(translation); + transform = Affine3A::from(translation); } if let Some(rotation_quat) = iter_rotation_quat.next() { - if let Ok(rotation_quat) = glam::Affine3A::try_from(rotation_quat) { + if let Ok(rotation_quat) = Affine3A::try_from(rotation_quat) { transform *= rotation_quat; } else { - transform = glam::Affine3A::ZERO; + transform = Affine3A::ZERO; } } if let Some(rotation_axis_angle) = iter_rotation_axis_angle.next() { - if let Ok(axis_angle) = glam::Affine3A::try_from(rotation_axis_angle) { + if let Ok(axis_angle) = Affine3A::try_from(rotation_axis_angle) { transform *= axis_angle; } else { - transform = glam::Affine3A::ZERO; + transform = Affine3A::ZERO; } } if let Some(scale) = iter_scale.next() { - transform *= glam::Affine3A::from(scale); + transform *= Affine3A::from(scale); } if let Some(mat3x3) = iter_mat3x3.next() { - transform *= glam::Affine3A::from(mat3x3); + transform *= Affine3A::from(mat3x3); } transforms.push(transform); From f4155af1c3be8375903d4bd52f936e944e23630d Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 11:54:55 +0100 Subject: [PATCH 26/31] collect instead of loop --- .../re_view_spatial/src/transform_cache.rs | 59 +++++++++---------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index b56fa91d27dc..1474438e7382 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -512,38 +512,37 @@ fn query_and_resolve_instance_poses_at_entity( let mut iter_scale = clamped_or_nothing(batch_scale, max_num_instances); let mut iter_mat3x3 = clamped_or_nothing(batch_mat3x3, max_num_instances); - let mut transforms = Vec::with_capacity(max_num_instances); - for _ in 0..max_num_instances { - // We apply these in a specific order - see `debug_assert_transform_field_order` - let mut transform = Affine3A::IDENTITY; - if let Some(translation) = iter_translation.next() { - transform = Affine3A::from(translation); - } - if let Some(rotation_quat) = iter_rotation_quat.next() { - if let Ok(rotation_quat) = Affine3A::try_from(rotation_quat) { - transform *= rotation_quat; - } else { - transform = Affine3A::ZERO; + (0..max_num_instances) + .into_iter() + .map(|_| { + // We apply these in a specific order - see `debug_assert_transform_field_order` + let mut transform = Affine3A::IDENTITY; + if let Some(translation) = iter_translation.next() { + transform = Affine3A::from(translation); } - } - if let Some(rotation_axis_angle) = iter_rotation_axis_angle.next() { - if let Ok(axis_angle) = Affine3A::try_from(rotation_axis_angle) { - transform *= axis_angle; - } else { - transform = Affine3A::ZERO; + if let Some(rotation_quat) = iter_rotation_quat.next() { + if let Ok(rotation_quat) = Affine3A::try_from(rotation_quat) { + transform *= rotation_quat; + } else { + transform = Affine3A::ZERO; + } } - } - if let Some(scale) = iter_scale.next() { - transform *= Affine3A::from(scale); - } - if let Some(mat3x3) = iter_mat3x3.next() { - transform *= Affine3A::from(mat3x3); - } - - transforms.push(transform); - } - - transforms + if let Some(rotation_axis_angle) = iter_rotation_axis_angle.next() { + if let Ok(axis_angle) = Affine3A::try_from(rotation_axis_angle) { + transform *= axis_angle; + } else { + transform = Affine3A::ZERO; + } + } + if let Some(scale) = iter_scale.next() { + transform *= Affine3A::from(scale); + } + if let Some(mat3x3) = iter_mat3x3.next() { + transform *= Affine3A::from(mat3x3); + } + transform + }) + .collect() } fn query_and_resolve_pinhole_projection_at_entity( From 5d8fdaf03d1569551aae91136300e5cad93fcc1d Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 13:29:12 +0100 Subject: [PATCH 27/31] add garbage collection to transform cache --- .../re_view_spatial/src/transform_cache.rs | 218 +++++++++++++----- 1 file changed, 162 insertions(+), 56 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 1474438e7382..5223d5ca901c 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; -use ahash::HashMap; +use ahash::{HashMap, HashSet}; use glam::Affine3A; use itertools::Either; use nohash_hasher::{IntMap, IntSet}; @@ -279,6 +279,120 @@ impl TransformCacheStoreSubscriber { } } } + + fn add_chunk(&mut self, event: &re_chunk_store::ChunkStoreEvent, aspects: TransformAspect) { + let entity_path = event.chunk.entity_path(); + + for (timeline, time_column) in event.diff.chunk.timelines() { + let per_timeline = self.per_timeline.entry(*timeline).or_default(); + + // All of these require complex latest-at queries that would require a lot more context, + // are fairly expensive, and may depend on other components that may come in at the same time. + // (we could inject that here, but it's not entirely straight forward). + // So instead, we note down that the caches is invalidated for the given entity & times. + + // This invalidates any time _after_ the first event in this chunk. + // (e.g. if a rotation is added prior to translations later on, + // then the resulting transforms at those translations changes as well for latest-at queries) + let mut invalidated_times = Vec::new(); + let Some(min_time) = time_column.times().min() else { + continue; + }; + if let Some(entity_entry) = per_timeline.per_entity.get_mut(&entity_path.hash()) { + if aspects.contains(TransformAspect::Tree) { + let invalidated_tree_transforms = + entity_entry.tree_transforms.split_off(&min_time); + invalidated_times.extend(invalidated_tree_transforms.into_keys()); + } + if aspects.contains(TransformAspect::Pose) { + if let Some(pose_transforms) = &mut entity_entry.pose_transforms { + let invalidated_pose_transforms = pose_transforms.split_off(&min_time); + invalidated_times.extend(invalidated_pose_transforms.into_keys()); + } + } + if aspects.contains(TransformAspect::PinholeOrViewCoordinates) { + if let Some(pinhole_projections) = &mut entity_entry.pinhole_projections { + let invalidated_pinhole_projections = + pinhole_projections.split_off(&min_time); + invalidated_times.extend(invalidated_pinhole_projections.into_keys()); + } + } + } + + per_timeline + .invalidated_transforms + .push(InvalidatedTransforms { + entity_path: entity_path.clone(), + times: time_column + .times() + .chain(invalidated_times.into_iter()) + .collect(), + aspects, + }); + } + } + + fn remove_chunk(&mut self, event: &re_chunk_store::ChunkStoreEvent, aspects: TransformAspect) { + let entity_path = event.chunk.entity_path(); + + for (timeline, time_column) in event.diff.chunk.timelines() { + let Some(per_timeline) = self.per_timeline.get_mut(timeline) else { + continue; + }; + + // Remove incoming data. + for invalidated_transform in per_timeline + .invalidated_transforms + .iter_mut() + .filter(|invalidated_transform| &invalidated_transform.entity_path == entity_path) + { + let times = time_column.times().collect::>(); + invalidated_transform + .times + .retain(|time| !times.contains(time)); + } + per_timeline + .invalidated_transforms + .retain(|invalidated_transform| !invalidated_transform.times.is_empty()); + + // Remove existing data. + if let Some(per_entity) = per_timeline.per_entity.get_mut(&entity_path.hash()) { + for time in time_column.times() { + if aspects.contains(TransformAspect::Tree) { + per_entity.tree_transforms.remove(&time); + } + if aspects.contains(TransformAspect::Pose) { + if let Some(pose_transforms) = &mut per_entity.pose_transforms { + pose_transforms.remove(&time); + } + } + if aspects.contains(TransformAspect::PinholeOrViewCoordinates) { + if let Some(pinhole_projections) = &mut per_entity.pinhole_projections { + pinhole_projections.remove(&time); + } + } + } + + if per_entity.tree_transforms.is_empty() + && per_entity + .pose_transforms + .as_ref() + .map_or(true, |pose_transforms| pose_transforms.is_empty()) + && per_entity + .pinhole_projections + .as_ref() + .map_or(true, |pinhole_projections| pinhole_projections.is_empty()) + { + per_timeline.per_entity.remove(&entity_path.hash()); + } + } + + if per_timeline.per_entity.is_empty() && per_timeline.invalidated_transforms.is_empty() + { + self.per_timeline.remove(timeline); + } + } + } } impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { @@ -290,11 +404,6 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { re_tracing::profile_function!(); for event in events { - if event.kind == re_chunk_store::ChunkStoreDiffKind::Deletion { - // Not participating in GC for now. - continue; - } - // The components we are interested in may only show up on some of the timelines // within this chunk, so strictly speaking the affected "aspects" we compute here are conservative. // But that's fairly rare, so a few false positive entries here are fine. @@ -314,54 +423,10 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { continue; } - let entity_path = event.chunk.entity_path(); - - for (timeline, time_column) in event.diff.chunk.timelines() { - let per_timeline = self.per_timeline.entry(*timeline).or_default(); - - // All of these require complex latest-at queries that would require a lot more context, - // are fairly expensive, and may depend on other components that may come in at the same time. - // (we could inject that here, but it's not entirely straight forward). - // So instead, we note down that the caches is invalidated for the given entity & times. - - // This invalidates any time _after_ the first event in this chunk. - // (e.g. if a rotation is added prior to translations later on, - // then the resulting transforms at those translations changes as well for latest-at queries) - let mut invalidated_times = Vec::new(); - let Some(min_time) = time_column.times().min() else { - continue; - }; - if let Some(entity_entry) = per_timeline.per_entity.get_mut(&entity_path.hash()) { - if aspects.contains(TransformAspect::Tree) { - let invalidated_tree_transforms = - entity_entry.tree_transforms.split_off(&min_time); - invalidated_times.extend(invalidated_tree_transforms.into_keys()); - } - if aspects.contains(TransformAspect::Pose) { - if let Some(pose_transforms) = &mut entity_entry.pose_transforms { - let invalidated_pose_transforms = pose_transforms.split_off(&min_time); - invalidated_times.extend(invalidated_pose_transforms.into_keys()); - } - } - if aspects.contains(TransformAspect::PinholeOrViewCoordinates) { - if let Some(pinhole_projections) = &mut entity_entry.pinhole_projections { - let invalidated_pinhole_projections = - pinhole_projections.split_off(&min_time); - invalidated_times.extend(invalidated_pinhole_projections.into_keys()); - } - } - } - - per_timeline - .invalidated_transforms - .push(InvalidatedTransforms { - entity_path: entity_path.clone(), - times: time_column - .times() - .chain(invalidated_times.into_iter()) - .collect(), - aspects, - }); + if event.kind == re_chunk_store::ChunkStoreDiffKind::Deletion { + self.remove_chunk(event, aspects); + } else { + self.add_chunk(event, aspects); } } } @@ -513,7 +578,6 @@ fn query_and_resolve_instance_poses_at_entity( let mut iter_mat3x3 = clamped_or_nothing(batch_mat3x3, max_num_instances); (0..max_num_instances) - .into_iter() .map(|_| { // We apply these in a specific order - see `debug_assert_transform_field_order` let mut transform = Affine3A::IDENTITY; @@ -564,7 +628,9 @@ fn query_and_resolve_pinhole_projection_at_entity( mod tests { use std::sync::Arc; - use re_chunk_store::{external::re_chunk::ChunkBuilder, ChunkId, RowId}; + use re_chunk_store::{ + external::re_chunk::ChunkBuilder, ChunkId, GarbageCollectionOptions, RowId, + }; use re_types::{archetypes, Loggable, SerializedComponentBatch}; use super::*; @@ -971,4 +1037,44 @@ mod tests { ); }); } + + #[test] + fn test_gc() { + let mut entity_db = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording)); + ensure_subscriber_registered(&entity_db); + + let timeline = Timeline::new_sequence("t"); + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity0")) + .with_archetype( + RowId::new(), + [(timeline, 1)], + &archetypes::Transform3D::from_translation([1.0, 2.0, 3.0]), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Apply some updates to the transform before GC pass. + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + cache.apply_all_updates(&entity_db); + }); + + let chunk = ChunkBuilder::new(ChunkId::new(), EntityPath::from("my_entity1")) + .with_archetype( + RowId::new(), + [(timeline, 2)], + &archetypes::Transform3D::from_translation([4.0, 5.0, 6.0]), + ) + .build() + .unwrap(); + entity_db.add_chunk(&Arc::new(chunk)).unwrap(); + + // Don't apply updates for this chunk. + + entity_db.gc(&GarbageCollectionOptions::gc_everything()); + + TransformCacheStoreSubscriber::access_mut(&entity_db.store_id(), |cache| { + assert!(cache.transforms_per_timeline(timeline).is_none()); + }); + } } From bbf8290eb33c38fde7f57ef6200e3ae0e4d481af Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 14:09:01 +0100 Subject: [PATCH 28/31] reference ticket for reporting invalid transforms --- crates/viewer/re_view_spatial/src/transform_cache.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 5223d5ca901c..8799d9c812ce 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -436,13 +436,13 @@ impl PerStoreChunkSubscriber for TransformCacheStoreSubscriber { /// /// If any of the components yields an invalid transform, returns a `glam::Affine3A::ZERO`. /// (this effectively disconnects a subtree from the transform hierarchy!) -// TODO(andreas): There's no way to discover invalid transforms right now. +// TODO(#3849): There's no way to discover invalid transforms right now (they can be intentional but often aren't). fn query_and_resolve_tree_transform_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, query: &LatestAtQuery, ) -> Option { - // TODO(andreas): Filter out the components we're actually interested in? + // TODO(andreas): Filter out styling components. let components = archetypes::Transform3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); let result = entity_db.latest_at(query, entity_path, component_names); @@ -507,13 +507,13 @@ fn query_and_resolve_tree_transform_at_entity( /// /// If any of the components yields an invalid transform, returns a `glam::Affine3A::ZERO` for that instance. /// (this effectively ignores the instance for most visualizations!) -// TODO(andreas): There's no way to discover invalid transforms right now. +// TODO(#3849): There's no way to discover invalid transforms right now (they can be intentional but often aren't). fn query_and_resolve_instance_poses_at_entity( entity_path: &EntityPath, entity_db: &EntityDb, query: &LatestAtQuery, ) -> Vec { - // TODO(andreas): Filter out the components we're actually interested in? + // TODO(andreas): Filter out styling components. let components = archetypes::InstancePoses3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); let result = entity_db.latest_at(query, entity_path, component_names); From 0d499f85681aa28642ef5d9f4870a3d38b8b6897 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 14:23:37 +0100 Subject: [PATCH 29/31] warn for non-mono transform lists --- crates/store/re_chunk/src/helpers.rs | 4 +-- crates/store/re_query/src/latest_at.rs | 4 +-- .../re_view_spatial/src/transform_cache.rs | 29 ++++++++++++++----- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/crates/store/re_chunk/src/helpers.rs b/crates/store/re_chunk/src/helpers.rs index 710eba1b6eab..f2a284628f8d 100644 --- a/crates/store/re_chunk/src/helpers.rs +++ b/crates/store/re_chunk/src/helpers.rs @@ -349,7 +349,7 @@ impl UnitChunkShared { /// Returns the raw data for the specified component, assuming a mono-batch. /// - /// Returns an error if the underlying batch is not of unit length. + /// Returns none if the underlying batch is not of unit length. #[inline] pub fn component_mono_raw( &self, @@ -369,7 +369,7 @@ impl UnitChunkShared { /// Returns the deserialized data for the specified component, assuming a mono-batch. /// - /// Returns an error if the data cannot be deserialized, or if the underlying batch is not of unit length. + /// Returns none if the data cannot be deserialized, or if the underlying batch is not of unit length. #[inline] pub fn component_mono(&self) -> Option> { let res = self.component_mono_raw(&C::name())?; diff --git a/crates/store/re_query/src/latest_at.rs b/crates/store/re_query/src/latest_at.rs index eb0afd78425d..922cd3a648b1 100644 --- a/crates/store/re_query/src/latest_at.rs +++ b/crates/store/re_query/src/latest_at.rs @@ -508,7 +508,7 @@ impl LatestAtResults { /// Returns the deserialized data for the specified component, assuming a mono-batch. /// - /// Returns an error if the data cannot be deserialized, or if the underlying batch is not of unit length. + /// Logs an error if the data cannot be deserialized, or if the underlying batch is not of unit length. #[inline] pub fn component_mono(&self) -> Option { self.component_mono_with_log_level(re_log::Level::Error) @@ -516,7 +516,7 @@ impl LatestAtResults { /// Returns the deserialized data for the specified component, assuming a mono-batch. /// - /// Returns an error if the data cannot be deserialized, or if the underlying batch is not of unit length. + /// Returns none if the data cannot be deserialized, or if the underlying batch is not of unit length. #[inline] pub fn component_mono_quiet(&self) -> Option { self.components diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 8799d9c812ce..9af7169979d8 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -445,38 +445,51 @@ fn query_and_resolve_tree_transform_at_entity( // TODO(andreas): Filter out styling components. let components = archetypes::Transform3D::all_components(); let component_names = components.iter().map(|descr| descr.component_name); - let result = entity_db.latest_at(query, entity_path, component_names); - if result.components.is_empty() { + let results = entity_db.latest_at(query, entity_path, component_names); + if results.components.is_empty() { return None; } let mut transform = Affine3A::IDENTITY; + // It's an error if there's more than one component. Warn in that case. + let mono_log_level = re_log::Level::Warn; + // The order of the components here is important, and checked by `debug_assert_transform_field_order` - if let Some(translation) = result.component_instance::(0) { + if let Some(translation) = + results.component_mono_with_log_level::(mono_log_level) + { transform = Affine3A::from(translation); } - if let Some(axis_angle) = result.component_instance::(0) { + if let Some(axis_angle) = + results.component_mono_with_log_level::(mono_log_level) + { if let Ok(axis_angle) = Affine3A::try_from(axis_angle) { transform *= axis_angle; } else { return Some(Affine3A::ZERO); } } - if let Some(quaternion) = result.component_instance::(0) { + if let Some(quaternion) = + results.component_mono_with_log_level::(mono_log_level) + { if let Ok(quaternion) = Affine3A::try_from(quaternion) { transform *= quaternion; } else { return Some(Affine3A::ZERO); } } - if let Some(scale) = result.component_instance::(0) { + if let Some(scale) = + results.component_mono_with_log_level::(mono_log_level) + { if scale.x() == 0.0 && scale.y() == 0.0 && scale.z() == 0.0 { return Some(Affine3A::ZERO); } transform *= Affine3A::from(scale); } - if let Some(mat3x3) = result.component_instance::(0) { + if let Some(mat3x3) = + results.component_mono_with_log_level::(mono_log_level) + { let affine_transform = Affine3A::from(mat3x3); if affine_transform.matrix3.determinant() == 0.0 { return Some(Affine3A::ZERO); @@ -484,7 +497,7 @@ fn query_and_resolve_tree_transform_at_entity( transform *= affine_transform; } - if result.component_instance::(0) + if results.component_mono_with_log_level::(mono_log_level) == Some(components::TransformRelation::ChildFromParent) { let determinant = transform.matrix3.determinant(); From 4aed79412fecdbb1c74aa820e1eef9a856597d5c Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 14:30:05 +0100 Subject: [PATCH 30/31] doc string fix --- crates/viewer/re_view_spatial/src/transform_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_view_spatial/src/transform_cache.rs b/crates/viewer/re_view_spatial/src/transform_cache.rs index 9af7169979d8..87922ecb7eef 100644 --- a/crates/viewer/re_view_spatial/src/transform_cache.rs +++ b/crates/viewer/re_view_spatial/src/transform_cache.rs @@ -129,7 +129,7 @@ pub struct ResolvedPinholeProjection { /// /// This is needed to orient 2D in 3D and 3D in 2D the right way around /// (answering questions like which axis is distance to viewer increasing). - /// If no view coordinates were logged, this is set to [`Self::DEFAULT_VIEW_COORDINATES`]. + /// If no view coordinates were logged, this is set to [`archetypes::Pinhole::DEFAULT_CAMERA_XYZ`]. pub view_coordinates: components::ViewCoordinates, } From 3e21fdd00e89df152b680541d64f7d6f2ede9bab Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 15 Jan 2025 14:41:36 +0100 Subject: [PATCH 31/31] undo incorrect comment change --- crates/store/re_chunk/src/helpers.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/store/re_chunk/src/helpers.rs b/crates/store/re_chunk/src/helpers.rs index f2a284628f8d..710eba1b6eab 100644 --- a/crates/store/re_chunk/src/helpers.rs +++ b/crates/store/re_chunk/src/helpers.rs @@ -349,7 +349,7 @@ impl UnitChunkShared { /// Returns the raw data for the specified component, assuming a mono-batch. /// - /// Returns none if the underlying batch is not of unit length. + /// Returns an error if the underlying batch is not of unit length. #[inline] pub fn component_mono_raw( &self, @@ -369,7 +369,7 @@ impl UnitChunkShared { /// Returns the deserialized data for the specified component, assuming a mono-batch. /// - /// Returns none if the data cannot be deserialized, or if the underlying batch is not of unit length. + /// Returns an error if the data cannot be deserialized, or if the underlying batch is not of unit length. #[inline] pub fn component_mono(&self) -> Option> { let res = self.component_mono_raw(&C::name())?;