Fix auto properties no longer being applied for legacy properties (`E…

…ntityProperties`) (#6190) ### What * Fixes #6175 This regressed in #6164. Decided not to roll back things (which would have been hard) but instead reimplement how these properties are passed through and make their role more obvious. ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6190?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6190?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6190) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --------- Co-authored-by: Clement Rey <[email protected]>
rerun-io · May 2, 2024 · 0c14528 · 0c14528
1 parent f5cc5d7
commit 0c14528
Show file tree

Hide file tree

Showing 10 changed files with 114 additions and 102 deletions.
diff --git a/crates/re_space_view/src/data_query.rs b/crates/re_space_view/src/data_query.rs
@@ -1,13 +1,18 @@
-use re_entity_db::{external::re_data_store::LatestAtQuery, EntityDb, EntityProperties};
+use re_entity_db::{
+    external::re_data_store::LatestAtQuery, EntityDb, EntityProperties, EntityPropertyMap,
+};
 use re_log_types::Timeline;
 use re_viewer_context::{
     DataQueryResult, PerVisualizer, QueryRange, SpaceViewClassRegistry, StoreContext,
     VisualizableEntities,
 };
 
-pub struct EntityOverrideContext {
+pub struct EntityOverrideContext<'a> {
     pub legacy_space_view_properties: EntityProperties,
 
+    /// Provides auto properties for each entity.
+    pub legacy_auto_properties: &'a EntityPropertyMap,
+
     /// Query range that data results should fall back to if they don't specify their own.
     pub default_query_range: QueryRange,
 }
@@ -23,6 +28,7 @@ pub trait PropertyResolver {
         blueprint_query: &LatestAtQuery,
         active_timeline: &Timeline,
         space_view_class_registry: &SpaceViewClassRegistry,
+        legacy_auto_properties: &EntityPropertyMap,
         query_result: &mut DataQueryResult,
     );
 }
@@ -34,8 +40,6 @@ pub trait PropertyResolver {
 pub trait DataQuery {
     /// Execute a full query, returning a `DataResultTree` containing all results.
     ///
-    /// `auto_properties` is a map containing any heuristic-derived auto properties for the given `SpaceView`.
-    ///
     /// This is used when building up the contents for a `SpaceView`.
     fn execute_query(
         &self,

diff --git a/crates/re_space_view/src/space_view.rs b/crates/re_space_view/src/space_view.rs
@@ -330,7 +330,7 @@ impl SpaceViewBlueprint {
         &self,
         ctx: &ViewerContext<'_>,
         view_state: &mut dyn SpaceViewState,
-        view_props: &mut EntityPropertyMap,
+        auto_properties: &mut EntityPropertyMap,
     ) {
         let query_result = ctx.lookup_query_result(self.id).clone();
 
@@ -353,7 +353,7 @@ impl SpaceViewBlueprint {
             ctx,
             view_state,
             &per_system_entities,
-            view_props,
+            auto_properties,
         );
     }
 
@@ -490,6 +490,7 @@ mod tests {
         let timeline = Timeline::new("time", re_log_types::TimeType::Time);
         let mut recording = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording));
         let mut blueprint = EntityDb::new(StoreId::random(re_log_types::StoreKind::Blueprint));
+        let legacy_auto_properties = EntityPropertyMap::default();
 
         let points = Points3D::new(vec![[1.0, 2.0, 3.0]]);
 
@@ -559,6 +560,7 @@ mod tests {
                 &blueprint_query,
                 &timeline,
                 &space_view_class_registry,
+                &legacy_auto_properties,
                 &mut query_result,
             );
 
@@ -610,6 +612,7 @@ mod tests {
                 &blueprint_query,
                 &timeline,
                 &space_view_class_registry,
+                &legacy_auto_properties,
                 &mut query_result,
             );
 
@@ -667,6 +670,7 @@ mod tests {
                 &blueprint_query,
                 &timeline,
                 &space_view_class_registry,
+                &legacy_auto_properties,
                 &mut query_result,
             );
 
@@ -693,6 +697,7 @@ mod tests {
     fn test_component_overrides() {
         let space_view_class_registry = SpaceViewClassRegistry::default();
         let timeline = Timeline::new("time", re_log_types::TimeType::Time);
+        let legacy_auto_properties = EntityPropertyMap::default();
         let mut recording = EntityDb::new(StoreId::random(re_log_types::StoreKind::Recording));
         let mut visualizable_entities_per_visualizer =
             PerVisualizer::<VisualizableEntities>::default();
@@ -948,6 +953,7 @@ mod tests {
                 &blueprint_query,
                 &timeline,
                 &space_view_class_registry,
+                &legacy_auto_properties,
                 &mut query_result,
             );
 

diff --git a/crates/re_space_view/src/space_view_contents.rs b/crates/re_space_view/src/space_view_contents.rs
@@ -4,7 +4,7 @@ use smallvec::SmallVec;
 
 use re_entity_db::{
     external::{re_data_store::LatestAtQuery, re_query::PromiseResult},
-    EntityDb, EntityProperties, EntityPropertiesComponent, EntityTree,
+    EntityDb, EntityProperties, EntityPropertiesComponent, EntityPropertyMap, EntityTree,
 };
 use re_log_types::{
     path::RuleEffect, EntityPath, EntityPathFilter, EntityPathRule, EntityPathSubs, Timeline,
@@ -386,13 +386,14 @@ impl DataQueryPropertyResolver<'_> {
     ///  may include properties from the `SpaceView` or `DataQuery`.
     ///  - The individual overrides are found by walking an override subtree under the `data_query/<id>/individual_overrides`
     ///  - The recursive overrides are found by walking an override subtree under the `data_query/<id>/recursive_overrides`
-    fn build_override_context(
+    fn build_override_context<'a>(
         &self,
         blueprint: &EntityDb,
         blueprint_query: &LatestAtQuery,
         active_timeline: &Timeline,
         space_view_class_registry: &SpaceViewClassRegistry,
-    ) -> EntityOverrideContext {
+        legacy_auto_properties: &'a EntityPropertyMap,
+    ) -> EntityOverrideContext<'a> {
         re_tracing::profile_function!();
 
         let legacy_space_view_properties = self
@@ -410,6 +411,7 @@ impl DataQueryPropertyResolver<'_> {
         EntityOverrideContext {
             legacy_space_view_properties,
             default_query_range,
+            legacy_auto_properties,
         }
     }
 
@@ -424,7 +426,7 @@ impl DataQueryPropertyResolver<'_> {
         blueprint_query: &LatestAtQuery,
         active_timeline: &Timeline,
         query_result: &mut DataQueryResult,
-        override_context: &EntityOverrideContext,
+        override_context: &EntityOverrideContext<'_>,
         recursive_accumulated_legacy_properties: &EntityProperties,
         recursive_property_overrides: &IntMap<ComponentName, OverridePath>,
         handle: DataResultHandle,
@@ -461,11 +463,16 @@ impl DataQueryPropertyResolver<'_> {
                 } else {
                     recursive_accumulated_legacy_properties.clone()
                 };
+            let default_legacy_properties = override_context
+                .legacy_auto_properties
+                .get(&node.data_result.entity_path);
             let accumulated_legacy_properties =
                 if let Some(individual) = individual_legacy_properties.as_ref() {
-                    recursive_accumulated_legacy_properties.with_child(individual)
+                    recursive_accumulated_legacy_properties
+                        .with_child(individual)
+                        .with_child(&default_legacy_properties)
                 } else {
-                    recursive_accumulated_legacy_properties.clone()
+                    recursive_accumulated_legacy_properties.with_child(&default_legacy_properties)
                 };
 
             // Update visualizers from overrides.
@@ -611,6 +618,7 @@ impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> {
         blueprint_query: &LatestAtQuery,
         active_timeline: &Timeline,
         space_view_class_registry: &SpaceViewClassRegistry,
+        legacy_auto_properties: &EntityPropertyMap,
         query_result: &mut DataQueryResult,
     ) {
         re_tracing::profile_function!();
@@ -619,6 +627,7 @@ impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> {
             blueprint_query,
             active_timeline,
             space_view_class_registry,
+            legacy_auto_properties,
         );
 
         if let Some(root) = query_result.tree.root_handle() {

diff --git a/crates/re_space_view_spatial/src/heuristics.rs b/crates/re_space_view_spatial/src/heuristics.rs
@@ -21,29 +21,32 @@ use crate::{
     },
 };
 
-pub fn update_object_property_heuristics(
+pub fn generate_auto_legacy_properties(
     ctx: &ViewerContext<'_>,
     per_system_entities: &PerSystemEntities,
-    entity_properties: &mut re_entity_db::EntityPropertyMap,
     scene_bbox_accum: &macaw::BoundingBox,
     spatial_kind: SpatialSpaceViewKind,
-) {
+) -> re_entity_db::EntityPropertyMap {
     re_tracing::profile_function!();
 
+    let mut auto_properties = re_entity_db::EntityPropertyMap::default();
+
     // Do pinhole properties before, since they may be used in transform3d heuristics.
-    update_pinhole_property_heuristics(per_system_entities, entity_properties, scene_bbox_accum);
+    update_pinhole_property_heuristics(per_system_entities, &mut auto_properties, scene_bbox_accum);
     update_depth_cloud_property_heuristics(
         ctx,
         per_system_entities,
-        entity_properties,
+        &mut auto_properties,
         spatial_kind,
     );
     update_transform3d_lines_heuristics(
         ctx,
         per_system_entities,
-        entity_properties,
+        &mut auto_properties,
         scene_bbox_accum,
     );
+
+    auto_properties
 }
 
 pub fn auto_size_world_heuristic(
@@ -73,35 +76,34 @@ pub fn auto_size_world_heuristic(
 
 fn update_pinhole_property_heuristics(
     per_system_entities: &PerSystemEntities,
-    entity_properties: &mut re_entity_db::EntityPropertyMap,
+    auto_properties: &mut re_entity_db::EntityPropertyMap,
     scene_bbox_accum: &macaw::BoundingBox,
 ) {
     for ent_path in per_system_entities
         .get(&CamerasVisualizer::identifier())
         .unwrap_or(&BTreeSet::new())
     {
-        let mut properties = entity_properties.get(ent_path);
-        if properties.pinhole_image_plane_distance.is_auto() {
-            let scene_size = scene_bbox_accum.size().length();
-            let default_image_plane_distance = if scene_size.is_finite() && scene_size > 0.0 {
-                scene_size * 0.02 // Works pretty well for `examples/python/open_photogrammetry_format/open_photogrammetry_format.py --no-frames`
-            } else {
-                // This value somewhat arbitrary. In almost all cases where the scene has defined bounds
-                // the heuristic will change it or it will be user edited. In the case of non-defined bounds
-                // this value works better with the default camera setup.
-                0.3
-            };
-            properties.pinhole_image_plane_distance =
-                EditableAutoValue::Auto(default_image_plane_distance);
-            entity_properties.overwrite_properties(ent_path.clone(), properties);
-        }
+        let mut properties = auto_properties.get(ent_path);
+
+        let scene_size = scene_bbox_accum.size().length();
+        let default_image_plane_distance = if scene_size.is_finite() && scene_size > 0.0 {
+            scene_size * 0.02 // Works pretty well for `examples/python/open_photogrammetry_format/open_photogrammetry_format.py --no-frames`
+        } else {
+            // This value somewhat arbitrary. In almost all cases where the scene has defined bounds
+            // the heuristic will change it or it will be user edited. In the case of non-defined bounds
+            // this value works better with the default camera setup.
+            0.3
+        };
+        properties.pinhole_image_plane_distance =
+            EditableAutoValue::Auto(default_image_plane_distance);
+        auto_properties.overwrite_properties(ent_path.clone(), properties);
     }
 }
 
 fn update_depth_cloud_property_heuristics(
     ctx: &ViewerContext<'_>,
     per_system_entities: &PerSystemEntities,
-    entity_properties: &mut re_entity_db::EntityPropertyMap,
+    auto_properties: &mut re_entity_db::EntityPropertyMap,
     spatial_kind: SpatialSpaceViewKind,
 ) {
     // TODO(andreas): There should be a depth cloud system
@@ -126,38 +128,31 @@ fn update_depth_cloud_property_heuristics(
             .latest_at_component::<DepthMeter>(ent_path, &ctx.current_query())
             .map(|meter| meter.value.0);
 
-        let mut properties = entity_properties.get(ent_path);
-        if properties.backproject_depth.is_auto() {
-            properties.backproject_depth = EditableAutoValue::Auto(
-                meaning == TensorDataMeaning::Depth && spatial_kind == SpatialSpaceViewKind::ThreeD,
-            );
-        }
+        let mut properties = auto_properties.get(ent_path);
+        properties.backproject_depth = EditableAutoValue::Auto(
+            meaning == TensorDataMeaning::Depth && spatial_kind == SpatialSpaceViewKind::ThreeD,
+        );
 
         if meaning == TensorDataMeaning::Depth {
-            if properties.depth_from_world_scale.is_auto() {
-                let auto = meter.unwrap_or_else(|| {
-                    if tensor.dtype().is_integer() {
-                        1000.0
-                    } else {
-                        1.0
-                    }
-                });
-                properties.depth_from_world_scale = EditableAutoValue::Auto(auto);
-            }
-
-            if properties.backproject_radius_scale.is_auto() {
-                properties.backproject_radius_scale = EditableAutoValue::Auto(1.0);
-            }
+            let auto = meter.unwrap_or_else(|| {
+                if tensor.dtype().is_integer() {
+                    1000.0
+                } else {
+                    1.0
+                }
+            });
+            properties.depth_from_world_scale = EditableAutoValue::Auto(auto);
+            properties.backproject_radius_scale = EditableAutoValue::Auto(1.0);
 
-            entity_properties.overwrite_properties(ent_path.clone(), properties);
+            auto_properties.overwrite_properties(ent_path.clone(), properties);
         }
     }
 }
 
 fn update_transform3d_lines_heuristics(
     ctx: &ViewerContext<'_>,
     per_system_entities: &PerSystemEntities,
-    entity_properties: &mut re_entity_db::EntityPropertyMap,
+    auto_properties: &mut re_entity_db::EntityPropertyMap,
     scene_bbox_accum: &macaw::BoundingBox,
 ) {
     for ent_path in per_system_entities
@@ -186,37 +181,33 @@ fn update_transform3d_lines_heuristics(
             None
         }
 
-        let mut properties = entity_properties.get(ent_path);
-        if properties.transform_3d_visible.is_auto() {
-            // By default show the transform if it is a camera extrinsic,
-            // or if this entity only contains Transform3D components.
-            let only_has_transform_components = ctx
-                .recording_store()
-                .all_components(&ctx.current_query().timeline(), ent_path)
-                .map_or(false, |c| {
-                    c.iter()
-                        .all(|c| re_types::archetypes::Transform3D::all_components().contains(c))
-                });
-            properties.transform_3d_visible = EditableAutoValue::Auto(
-                only_has_transform_components || is_pinhole_extrinsics_of(ent_path, ctx).is_some(),
-            );
-        }
-
-        if properties.transform_3d_size.is_auto() {
-            if let Some(pinhole_path) = is_pinhole_extrinsics_of(ent_path, ctx) {
-                // If there's a pinhole, we orient ourselves on its image plane distance
-                let pinhole_path_props = entity_properties.get(pinhole_path);
-                properties.transform_3d_size = EditableAutoValue::Auto(
-                    *pinhole_path_props.pinhole_image_plane_distance * 0.25,
-                );
-            } else {
-                // Size should be proportional to the scene extent, here covered by its diagonal
-                let diagonal_length = (scene_bbox_accum.max - scene_bbox_accum.min).length();
-                properties.transform_3d_size = EditableAutoValue::Auto(diagonal_length * 0.05);
-            }
+        let mut properties = auto_properties.get(ent_path);
+
+        // By default show the transform if it is a camera extrinsic,
+        // or if this entity only contains Transform3D components.
+        let only_has_transform_components = ctx
+            .recording_store()
+            .all_components(&ctx.current_query().timeline(), ent_path)
+            .map_or(false, |c| {
+                c.iter()
+                    .all(|c| re_types::archetypes::Transform3D::all_components().contains(c))
+            });
+        properties.transform_3d_visible = EditableAutoValue::Auto(
+            only_has_transform_components || is_pinhole_extrinsics_of(ent_path, ctx).is_some(),
+        );
+
+        if let Some(pinhole_path) = is_pinhole_extrinsics_of(ent_path, ctx) {
+            // If there's a pinhole, we orient ourselves on its image plane distance
+            let pinhole_path_props = auto_properties.get(pinhole_path);
+            properties.transform_3d_size =
+                EditableAutoValue::Auto(*pinhole_path_props.pinhole_image_plane_distance * 0.25);
+        } else {
+            // Size should be proportional to the scene extent, here covered by its diagonal
+            let diagonal_length = (scene_bbox_accum.max - scene_bbox_accum.min).length();
+            properties.transform_3d_size = EditableAutoValue::Auto(diagonal_length * 0.05);
         }
 
-        entity_properties.overwrite_properties(ent_path.clone(), properties);
+        auto_properties.overwrite_properties(ent_path.clone(), properties);
     }
 }