From 91da2b99ad537c42db9eee961f6d6f6df6f5164e Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 16 Feb 2024 09:18:50 +0100 Subject: [PATCH] Less restrictive visualizability constraints of 2d entitites, improved space view generation heuristics (#5188) ### What * Fixes #4926 * it does not address child-of-root heuristics for time series, but they weren't as much of a concern * Fixes heuristic issues observed in https://github.com/rerun-io/cpp-example-opencv-eigen/pull/20 Two changes in here: * we no longer determine the dimensionality of a topological subspace, instead we only determine whether it can't display 2d/3d * 2d: we now state we can _always_ display 2d objects * 3d: we can display it unless the parent of the active subspace is a pinhole in which case we only make the entity _at_ the origin as visualizable * make `ViewCoordinates` add "heuristic hints" to spatial topology * this is all that's needed to get rid of child-of-root heuristics in 3d without regressing existing examples OpenCV example that didn't work before: ![image](https://github.com/rerun-io/rerun/assets/1220815/571ad089-98cd-4295-ba5e-3b81c39c93c2) New release check: ![image](https://github.com/rerun-io/rerun/assets/1220815/d4d577e1-7fc1-41e5-90b4-6764c9db2f0d) ### 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 newly built examples: [app.rerun.io](https://app.rerun.io/pr/5188/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5188/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/5188/index.html?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/5188) - [Docs preview](https://rerun.io/preview/cc3735c02cd092df5843f7fe97bfa998db2e459d/docs) - [Examples preview](https://rerun.io/preview/cc3735c02cd092df5843f7fe97bfa998db2e459d/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- .../re_space_view_spatial/src/heuristics.rs | 70 +--- .../src/space_view_2d.rs | 86 ++--- .../src/space_view_3d.rs | 136 ++++--- .../src/spatial_topology.rs | 348 +++++++++--------- ...d_heuristics.py => check_heuristics_2d.py} | 0 .../check_heuristics_mixed_2d_and_3d.py | 67 ++++ 6 files changed, 336 insertions(+), 371 deletions(-) rename tests/python/release_checklist/{check_2d_heuristics.py => check_heuristics_2d.py} (100%) create mode 100644 tests/python/release_checklist/check_heuristics_mixed_2d_and_3d.py diff --git a/crates/re_space_view_spatial/src/heuristics.rs b/crates/re_space_view_spatial/src/heuristics.rs index 2537cda31957..5becb064a3df 100644 --- a/crates/re_space_view_spatial/src/heuristics.rs +++ b/crates/re_space_view_spatial/src/heuristics.rs @@ -1,12 +1,11 @@ use std::collections::BTreeSet; -use ahash::HashMap; use egui::NumExt as _; use nohash_hasher::IntSet; use re_data_ui::image_meaning_for_entity; use re_entity_db::EditableAutoValue; -use re_log_types::{EntityPath, EntityPathPart}; +use re_log_types::EntityPath; use re_types::{ components::{DepthMeter, TensorData}, tensor_data::TensorDataMeaning, @@ -18,7 +17,6 @@ use re_viewer_context::{ use crate::{ query_pinhole, - spatial_topology::{SpatialTopology, SubSpace, SubSpaceDimensionality}, view_kind::SpatialSpaceViewKind, visualizers::{ CamerasVisualizer, ImageVisualizer, SpatialViewVisualizerData, Transform3DArrowsVisualizer, @@ -254,69 +252,3 @@ pub fn default_visualized_entities_for_visualizer_kind( .cloned() .collect() } - -/// Splits the root space into subspaces under certain circumstances. -/// -/// TODO(#4926): This seems to be unnecessarily complicated. -/// #4926 describes the rationale and how we might be able to remove this. -pub fn root_space_split_heuristic( - topo: &SpatialTopology, - relevant_entities: &IntSet, - dimensionality: SubSpaceDimensionality, -) -> HashMap { - re_tracing::profile_function!(); - - // We want to split the root space if… - // - // … there's a root space in the first place. - let Some(root_space) = topo.subspace_for_subspace_origin(EntityPath::root().hash()) else { - return Default::default(); - }; - // … and that root space doesn't have a different dimensionality. - if root_space.dimensionality != dimensionality - && root_space.dimensionality != SubSpaceDimensionality::Unknown - { - return Default::default(); - } - // … the root space is not empty. - if root_space.entities.is_empty() { - return Default::default(); - } - // … nothing relevant logged directly at the root. - if relevant_entities.contains(&EntityPath::root()) { - return Default::default(); - } - - let mut interesting_children_of_root_spaces = HashMap::::default(); - for entity in &root_space.entities { - let Some(root_child) = entity.iter().next() else { - continue; - }; - interesting_children_of_root_spaces - .entry(root_child.clone()) - .or_insert_with(|| { - let root_child_path: EntityPath = [root_child.clone()].as_slice().into(); - SubSpace { - origin: root_child_path.clone(), - dimensionality, - entities: Default::default(), // Filled as we go. - child_spaces: root_space - .child_spaces - .iter() - .filter_map(|(child, connection)| { - if child.is_descendant_of(&root_child_path) { - Some((child.clone(), *connection)) - } else { - None - } - }) - .collect(), - parent_space: Some(EntityPath::root().hash()), - } - }) - .entities - .insert(entity.clone()); - } - - interesting_children_of_root_spaces -} diff --git a/crates/re_space_view_spatial/src/space_view_2d.rs b/crates/re_space_view_spatial/src/space_view_2d.rs index cbab4e35f8a0..f1ac75e203d5 100644 --- a/crates/re_space_view_spatial/src/space_view_2d.rs +++ b/crates/re_space_view_spatial/src/space_view_2d.rs @@ -19,7 +19,7 @@ use crate::{ default_visualized_entities_for_visualizer_kind, update_object_property_heuristics, }, max_image_dimension_subscriber::{ImageDimensions, MaxImageDimensions}, - spatial_topology::{SpatialTopology, SubSpaceDimensionality}, + spatial_topology::{SpatialTopology, SubSpaceConnectionFlags}, ui::SpatialSpaceViewState, view_kind::SpatialSpaceViewKind, visualizers::register_2d_spatial_visualizers, @@ -90,51 +90,29 @@ impl SpaceViewClass for SpatialSpaceView2D { let context = SpatialTopology::access(entity_db.store_id(), |topo| { let primary_space = topo.subspace_for_entity(space_origin); - match primary_space.dimensionality { - SubSpaceDimensionality::Unknown => VisualizableFilterContext2D { - entities_in_main_2d_space: primary_space.entities.clone(), + if !primary_space.supports_2d_content() { + // If this is strict 3D space, only display the origin entity itself. + // Everything else we have to assume requires some form of transformation. + return VisualizableFilterContext2D { + entities_in_main_2d_space: std::iter::once(space_origin.clone()).collect(), reprojectable_3d_entities: Default::default(), - }, - - SubSpaceDimensionality::TwoD => { - // All entities in the 2d space are visualizable + the parent space if it is connected via a pinhole. - // For the moment we don't allow going down pinholes again. - let reprojected_3d_entities = primary_space - .parent_space - .and_then(|parent_space_origin| { - let is_connected_pinhole = topo - .subspace_for_subspace_origin(parent_space_origin) - .and_then(|parent_space| { - parent_space - .child_spaces - .get(&primary_space.origin) - .map(|connection| connection.is_connected_pinhole()) - }) - .unwrap_or(false); - - if is_connected_pinhole { - topo.subspace_for_subspace_origin(parent_space_origin) - .map(|parent_space| parent_space.entities.clone()) - } else { - None - } - }) - .unwrap_or_default(); - - VisualizableFilterContext2D { - entities_in_main_2d_space: primary_space.entities.clone(), - reprojectable_3d_entities: reprojected_3d_entities, - } - } - - SubSpaceDimensionality::ThreeD => { - // If this is 3D space, only display the origin entity itself. - // Everything else we have to assume requires some form of transformation. - VisualizableFilterContext2D { - entities_in_main_2d_space: std::iter::once(space_origin.clone()).collect(), - reprojectable_3d_entities: Default::default(), - } - } + }; + } + + // All space are visualizable + the parent space if it is connected via a pinhole. + // For the moment we don't allow going down pinholes again. + let reprojectable_3d_entities = + if primary_space.connection_to_parent.is_connected_pinhole() { + topo.subspace_for_subspace_origin(primary_space.parent_space) + .map(|parent_space| parent_space.entities.clone()) + .unwrap_or_default() + } else { + Default::default() + }; + + VisualizableFilterContext2D { + entities_in_main_2d_space: primary_space.entities.clone(), + reprojectable_3d_entities, } }); @@ -182,7 +160,7 @@ impl SpaceViewClass for SpatialSpaceView2D { recommended_space_views: topo .iter_subspaces() .flat_map(|subspace| { - if subspace.dimensionality == SubSpaceDimensionality::ThreeD + if !subspace.supports_2d_content() || subspace.entities.is_empty() || indicated_entities.is_disjoint(&subspace.entities) { @@ -197,14 +175,16 @@ impl SpaceViewClass for SpatialSpaceView2D { .cloned() .collect(); - // For explicit 2D spaces (typically indicated by a pinhole or similar) start at the origin, otherwise start at the common ancestor. + // For explicit 2D spaces with a pinhole at the origin, otherwise start at the common ancestor. // This generally avoids the `/` root entity unless it's required as a common ancestor. - let recommended_root = - if subspace.dimensionality == SubSpaceDimensionality::TwoD { - subspace.origin.clone() - } else { - EntityPath::common_ancestor_of(relevant_entities.iter()) - }; + let recommended_root = if subspace + .connection_to_parent + .contains(SubSpaceConnectionFlags::Pinhole) + { + subspace.origin.clone() + } else { + EntityPath::common_ancestor_of(relevant_entities.iter()) + }; let mut recommended_space_views = Vec::::new(); diff --git a/crates/re_space_view_spatial/src/space_view_3d.rs b/crates/re_space_view_spatial/src/space_view_3d.rs index 6e89a592adc9..61128fe3c79b 100644 --- a/crates/re_space_view_spatial/src/space_view_3d.rs +++ b/crates/re_space_view_spatial/src/space_view_3d.rs @@ -1,3 +1,4 @@ +use itertools::Itertools; use nohash_hasher::IntSet; use re_entity_db::EntityProperties; use re_log_types::{EntityPath, EntityPathFilter}; @@ -11,10 +12,9 @@ use re_viewer_context::{ use crate::{ contexts::{register_spatial_contexts, PrimitiveCounter}, heuristics::{ - default_visualized_entities_for_visualizer_kind, root_space_split_heuristic, - update_object_property_heuristics, + default_visualized_entities_for_visualizer_kind, update_object_property_heuristics, }, - spatial_topology::{SpatialTopology, SubSpaceDimensionality}, + spatial_topology::{HeuristicHints, SpatialTopology, SubSpaceConnectionFlags}, ui::SpatialSpaceViewState, view_kind::SpatialSpaceViewKind, visualizers::register_3d_spatial_visualizers, @@ -84,46 +84,40 @@ impl SpaceViewClass for SpatialSpaceView3D { let context = SpatialTopology::access(entity_db.store_id(), |topo| { let primary_space = topo.subspace_for_entity(space_origin); - match primary_space.dimensionality { - SubSpaceDimensionality::Unknown => VisualizableFilterContext3D { - entities_in_main_3d_space: primary_space.entities.clone(), + if !primary_space.supports_3d_content() { + // If this is strict 2D space, only display the origin entity itself. + // Everything else we have to assume requires some form of transformation. + return VisualizableFilterContext3D { + entities_in_main_3d_space: std::iter::once(space_origin.clone()).collect(), entities_under_pinholes: Default::default(), - }, - - SubSpaceDimensionality::ThreeD => { - // All entities in the 3d space are visualizable + everything under pinholes. - let mut entities_in_main_3d_space = primary_space.entities.clone(); - let mut entities_under_pinholes = IntSet::::default(); - - for (child_origin, connection) in &primary_space.child_spaces { - if connection.is_connected_pinhole() { - let Some(child_space) = - topo.subspace_for_subspace_origin(child_origin.hash()) - else { - // Should never happen, implies that a child space is not in the list of subspaces. - continue; - }; - - // Entities _at_ pinholes are a special case: we display both 3d and 2d visualizers for them. - entities_in_main_3d_space.insert(child_space.origin.clone()); - entities_under_pinholes.extend(child_space.entities.iter().cloned()); - } - } + }; + } - VisualizableFilterContext3D { - entities_in_main_3d_space, - entities_under_pinholes, - } + // All entities in the 3d space are visualizable + everything under pinholes. + let mut entities_in_main_3d_space = primary_space.entities.clone(); + let mut entities_under_pinholes = IntSet::::default(); + + for child_origin in &primary_space.child_spaces { + let Some(child_space) = topo.subspace_for_subspace_origin(child_origin.hash()) + else { + // Should never happen, implies that a child space is not in the list of subspaces. + continue; + }; + + if child_space + .connection_to_parent + .contains(SubSpaceConnectionFlags::Pinhole) + { + // Note that for this the connection to the parent is allowed to contain the disconnected flag. + // Entities _at_ pinholes are a special case: we display both 3d and 2d visualizers for them. + entities_in_main_3d_space.insert(child_space.origin.clone()); + entities_under_pinholes.extend(child_space.entities.iter().cloned()); } + } - SubSpaceDimensionality::TwoD => { - // If this is 2D space, only display the origin entity itself. - // Everything else we have to assume requires some form of transformation. - VisualizableFilterContext3D { - entities_in_main_3d_space: std::iter::once(space_origin.clone()).collect(), - entities_under_pinholes: Default::default(), - } - } + VisualizableFilterContext3D { + entities_in_main_3d_space, + entities_under_pinholes, } }); @@ -161,40 +155,40 @@ impl SpaceViewClass for SpatialSpaceView3D { // Spawn a space view at each subspace that has any potential 3D content. // Note that visualizability filtering is all about being in the right subspace, // so we don't need to call the visualizers' filter functions here. - SpatialTopology::access(ctx.entity_db.store_id(), |topo| { - let split_root_spaces = root_space_split_heuristic( - topo, - &indicated_entities, - SubSpaceDimensionality::ThreeD, - ); - - SpaceViewSpawnHeuristics { - recommended_space_views: topo - .iter_subspaces() - .flat_map(|subspace| { - if subspace.origin.is_root() && !split_root_spaces.is_empty() { - itertools::Either::Left(split_root_spaces.values()) - } else { - itertools::Either::Right(std::iter::once(subspace)) - } - }) - .filter_map(|subspace| { - if subspace.dimensionality == SubSpaceDimensionality::TwoD - || subspace.entities.is_empty() - || indicated_entities.is_disjoint(&subspace.entities) + SpatialTopology::access(ctx.entity_db.store_id(), |topo| SpaceViewSpawnHeuristics { + recommended_space_views: topo + .iter_subspaces() + .filter_map(|subspace| { + if !subspace.supports_3d_content() || subspace.entities.is_empty() { + None + } else { + // Creates space views at each view coordinates if there's any. + // (yes, we do so even if they're empty at the moment!) + let mut roots = subspace + .heuristic_hints + .iter() + .filter(|(_, hint)| hint.contains(HeuristicHints::ViewCoordinates3d)) + .map(|(root, _)| root.clone()) + .collect::>(); + + // If there's no view coordinates or there are still some entities not covered, + // create a view at the subspace origin. + if !roots.iter().contains(&subspace.origin) + && indicated_entities + .intersection(&subspace.entities) + .any(|e| roots.iter().all(|root| !e.starts_with(root))) { - None - } else { - Some(RecommendedSpaceView { - root: subspace.origin.clone(), - query_filter: EntityPathFilter::subtree_entity_filter( - &subspace.origin, - ), - }) + roots.push(subspace.origin.clone()); } - }) - .collect(), - } + + Some(roots.into_iter().map(|root| RecommendedSpaceView { + query_filter: EntityPathFilter::subtree_entity_filter(&root), + root, + })) + } + }) + .flatten() + .collect(), }) .unwrap_or_default() } diff --git a/crates/re_space_view_spatial/src/spatial_topology.rs b/crates/re_space_view_spatial/src/spatial_topology.rs index 82c38149518e..2e16990c5270 100644 --- a/crates/re_space_view_spatial/src/spatial_topology.rs +++ b/crates/re_space_view_spatial/src/spatial_topology.rs @@ -5,32 +5,10 @@ use nohash_hasher::{IntMap, IntSet}; use re_data_store::{StoreSubscriber, StoreSubscriberHandle}; use re_log_types::{EntityPath, EntityPathHash, StoreId}; use re_types::{ - components::{DisconnectedSpace, PinholeProjection}, + components::{DisconnectedSpace, PinholeProjection, ViewCoordinates}, Loggable, }; -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum SubSpaceDimensionality { - /// We don't know if this space is a 2D or 3D space. - /// - /// This is the most common case and happens whenever there's no projection that - /// establishes a clear distinction between 2D and 3D spaces. - /// - /// Note that this can both mean "there are both 2D and 3D relationships within the space" - /// as well as "there are not spatial relationships at all within the space". - Unknown, - - /// The space is definitively a 2D space. - /// - /// This conclusion is usually reached by the presence of a projection operation. - TwoD, - - /// The space is definitively a 3D space. - /// - /// This conclusion is usually reached by the presence of a projection operation. - ThreeD, -} - bitflags::bitflags! { #[derive(PartialEq, Eq, Debug, Copy, Clone)] pub struct SubSpaceConnectionFlags: u8 { @@ -48,6 +26,14 @@ impl SubSpaceConnectionFlags { } } +bitflags::bitflags! { + /// Marks entities that are of special interest for heuristics. + #[derive(PartialEq, Eq, Debug, Copy, Clone)] + pub struct HeuristicHints: u8 { + const ViewCoordinates3d = 0b0000001; + } +} + /// Spatial subspace within we typically expect a homogeneous dimensionality without any projections & disconnects. /// /// Subspaces are separated by projections or explicit disconnects. @@ -64,8 +50,6 @@ pub struct SubSpace { /// This is also used to uniquely identify the space. pub origin: EntityPath, - pub dimensionality: SubSpaceDimensionality, - /// All entities that were logged at any point in time and are part of this subspace. /// /// Contains the origin entity as well, unless the origin is the `EntityPath::root()` and nothing was logged to it directly. @@ -77,18 +61,29 @@ pub struct SubSpace { /// which would make this an expensive query. pub entities: IntSet, - /// Origin paths of child spaces and how they're connected. + /// Origin paths of child spaces. /// /// This implies that there is a either an explicit disconnect or /// a projection at the origin of the child space. - /// How it is connected is implied by `parent_space` in the child space. + /// How it is connected is implied by `connection_to_parent` in the child space. /// /// Any path in `child_spaces` is *not* contained in `entities`. /// This implies that the camera itself is not part of its 3D space even when it may still have a 3D transform. - pub child_spaces: IntMap, + pub child_spaces: IntSet, - /// Origin of the parent space if any. - pub parent_space: Option, + /// Origin of the parent space. + /// + /// The root space has `EntityPathHash::NONE` as parent. + pub parent_space: EntityPathHash, + + /// The connection to the parent space. + /// + /// Note that since flags are derived from the presence of components at the origin, + /// the root space still tracks this information. + pub connection_to_parent: SubSpaceConnectionFlags, + + /// Entities in this space that qualify for one or more heuristic hints. + pub heuristic_hints: IntMap, // // TODO(andreas): // We could (and should) add here additional transform hierarchy information within this space. @@ -96,35 +91,27 @@ pub struct SubSpace { } impl SubSpace { - /// Updates dimensionality based on a new connection to a child space. - fn add_or_update_child_connection( - &mut self, - child_path: &EntityPath, - new_connections_to_child: SubSpaceConnectionFlags, - ) { - if new_connections_to_child.contains(SubSpaceConnectionFlags::Pinhole) { - match self.dimensionality { - SubSpaceDimensionality::Unknown => { - self.dimensionality = SubSpaceDimensionality::ThreeD; - } - SubSpaceDimensionality::TwoD => { - // For the moment the only way to get a 2D space is by defining a pinhole, - // but in the future other projections may also cause a space to be defined as 2D space. - // TODO(#3849, #4301): We should be able to tag the source entity as having an invalid transform so we can display a permanent warning in the ui. - re_log::warn_once!("There was already a pinhole logged at {:?}. -The new pinhole at {:?} is nested under it, implying an invalid projection from a 2D space to a 2D space.", self.origin, child_path); - // We keep 2D. - } - SubSpaceDimensionality::ThreeD => { - // Already 3D. - } - } - } + /// Whether 3d content in this subspace can be displayed. + #[inline] + pub fn supports_3d_content(&self) -> bool { + // Note that we currently do *not* walk up the tree of spaces to check for pinholes. + // Pro: + // * on a disconnect everything should be possible again, so why would that not be the case at every cut? + // * being overly restrictive means we won't display 3D content when we could. + // * for the same reason we also don't want to preclude 3D content when encountering 2D view coordinates, albeit this may still inform heuristics + // Con: + // * if at any point (without a disconnect) we encountered a pinhole prior, everything below should be considered 2D + !self + .connection_to_parent + .contains(SubSpaceConnectionFlags::Pinhole) + } - self.child_spaces - .entry(child_path.clone()) - .or_insert(new_connections_to_child) // insert into child spaces in the first place - .insert(new_connections_to_child); // insert into connection flags + /// Whether 2d content in this subspace can be displayed. + #[inline] + #[allow(clippy::unused_self)] + pub fn supports_2d_content(&self) -> bool { + // There's currently no way to prevent a subspace from displaying 2d content. + true } } @@ -205,10 +192,11 @@ impl Default for SpatialTopology { EntityPath::root().hash(), SubSpace { origin: EntityPath::root(), - dimensionality: SubSpaceDimensionality::Unknown, entities: IntSet::default(), // Note that this doesn't contain the root entity. - child_spaces: IntMap::default(), - parent_space: None, + child_spaces: IntSet::default(), + parent_space: EntityPathHash::NONE, + connection_to_parent: SubSpaceConnectionFlags::empty(), + heuristic_hints: IntMap::default(), }, )) .collect(), @@ -284,11 +272,15 @@ impl SpatialTopology { re_tracing::profile_function!(); let mut new_subspace_connections = SubSpaceConnectionFlags::empty(); + let mut new_heuristic_hints = HeuristicHints::empty(); + for added_component in added_components { if added_component == &DisconnectedSpace::name() { new_subspace_connections.insert(SubSpaceConnectionFlags::Disconnected); } else if added_component == &PinholeProjection::name() { new_subspace_connections.insert(SubSpaceConnectionFlags::Pinhole); + } else if added_component == &ViewCoordinates::name() { + new_heuristic_hints.insert(HeuristicHints::ViewCoordinates3d); }; } @@ -308,6 +300,18 @@ impl SpatialTopology { } else { self.add_new_entity(entity_path, new_subspace_connections); }; + + if !new_heuristic_hints.is_empty() { + let subspace = self + .subspaces + .get_mut(&self.subspace_origin_hash_for_entity(entity_path)) + .expect("unknown subspace origin, `SpatialTopology` is in an invalid state"); + subspace + .heuristic_hints + .entry(entity_path.clone()) + .or_insert(HeuristicHints::empty()) + .insert(new_heuristic_hints); + } } fn update_space_with_new_connections( @@ -323,18 +327,7 @@ impl SpatialTopology { .subspaces .get_mut(&subspace_origin_hash) .expect("Subspace origin not part of origin->subspace map."); - - // (see also `split_subspace`)` - if new_connections.contains(SubSpaceConnectionFlags::Pinhole) { - subspace.dimensionality = SubSpaceDimensionality::TwoD; - } - - if let Some(parent_origin_hash) = subspace.parent_space { - self.subspaces - .get_mut(&parent_origin_hash) - .expect("Parent origin not part of origin->subspace map.") - .add_or_update_child_connection(entity_path, new_connections); - } + subspace.connection_to_parent.insert(new_connections); } else { // Split the existing subspace. self.split_subspace(subspace_origin_hash, entity_path, new_connections); @@ -349,19 +342,21 @@ impl SpatialTopology { ) { let subspace_origin_hash = self.subspace_origin_hash_for_entity(entity_path); - let target_space_origin_hash = if subspace_connections.is_empty() { - // Add entity to the existing space. - let subspace = self - .subspaces - .get_mut(&subspace_origin_hash) - .expect("Subspace origin not part of origin->subspace map."); - subspace.entities.insert(entity_path.clone()); - subspace.origin.hash() - } else { - // Create a new subspace with this entity as its origin & containing this entity. - self.split_subspace(subspace_origin_hash, entity_path, subspace_connections); - entity_path.hash() - }; + let target_space_origin_hash = + if subspace_connections.is_empty() || entity_path.hash() == subspace_origin_hash { + // Add entity to the existing space. + let subspace = self + .subspaces + .get_mut(&subspace_origin_hash) + .expect("Subspace origin not part of origin->subspace map."); + subspace.entities.insert(entity_path.clone()); + subspace.connection_to_parent.insert(subspace_connections); + subspace.origin.hash() + } else { + // Create a new subspace with this entity as its origin & containing this entity. + self.split_subspace(subspace_origin_hash, entity_path, subspace_connections); + entity_path.hash() + }; self.subspace_origin_per_logged_entity .insert(entity_path.hash(), target_space_origin_hash); @@ -371,7 +366,7 @@ impl SpatialTopology { &mut self, split_subspace_origin_hash: EntityPathHash, new_space_origin: &EntityPath, - connections: SubSpaceConnectionFlags, + connection_to_parent: SubSpaceConnectionFlags, ) { let split_subspace = self .subspaces @@ -379,20 +374,13 @@ impl SpatialTopology { .expect("Subspace origin not part of origin->subspace map."); debug_assert!(new_space_origin.is_descendant_of(&split_subspace.origin)); - // Determine the space dimensionality of the new space and update the current space's space type if necessary. - // (see also `update_space_with_new_connections`) - let new_space_dimensionality = if connections.contains(SubSpaceConnectionFlags::Pinhole) { - SubSpaceDimensionality::TwoD - } else { - SubSpaceDimensionality::Unknown - }; - let mut new_space = SubSpace { origin: new_space_origin.clone(), - dimensionality: new_space_dimensionality, entities: std::iter::once(new_space_origin.clone()).collect(), child_spaces: Default::default(), - parent_space: Some(split_subspace_origin_hash), + parent_space: split_subspace_origin_hash, + connection_to_parent, + heuristic_hints: Default::default(), }; // Transfer entities from self to the new space if they're children of the new space. @@ -408,31 +396,26 @@ impl SpatialTopology { }); // Transfer any child spaces from self to the new space if they're children of the new space. - split_subspace - .child_spaces - .retain(|child_origin, connections| { - debug_assert!(child_origin != new_space_origin); - - if child_origin.is_descendant_of(new_space_origin) { - new_space - .child_spaces - .insert(child_origin.clone(), *connections); - false - } else { - true - } - }); - - // Note that the new connection information may change the known dimensionality of the space that we're splitting. - split_subspace.add_or_update_child_connection(new_space_origin, connections); + split_subspace.child_spaces.retain(|child_origin| { + debug_assert!(child_origin != new_space_origin); + + if child_origin.is_descendant_of(new_space_origin) { + new_space.child_spaces.insert(child_origin.clone()); + false + } else { + true + } + }); + + split_subspace.child_spaces.insert(new_space_origin.clone()); // Patch parents of the child spaces that were moved to the new space. - for child_origin in new_space.child_spaces.keys() { + for child_origin in &new_space.child_spaces { let child_space = self .subspaces .get_mut(&child_origin.hash()) .expect("Child origin not part of origin->subspace map."); - child_space.parent_space = Some(new_space.origin.hash()); + child_space.parent_space = new_space.origin.hash(); } self.subspaces.insert(new_space.origin.hash(), new_space); @@ -443,11 +426,11 @@ impl SpatialTopology { mod tests { use re_log_types::EntityPath; use re_types::{ - components::{DisconnectedSpace, PinholeProjection}, + components::{DisconnectedSpace, PinholeProjection, ViewCoordinates}, ComponentName, Loggable as _, }; - use crate::spatial_topology::{SubSpaceConnectionFlags, SubSpaceDimensionality}; + use crate::spatial_topology::{HeuristicHints, SubSpaceConnectionFlags}; use super::SpatialTopology; @@ -485,6 +468,25 @@ mod tests { topo.subspace_for_entity(&"robo/leg".into()).origin, EntityPath::root() ); + + // Add splitting entities to the root space - this should not cause any splits. + for (name, flags) in [ + (PinholeProjection::name(), SubSpaceConnectionFlags::Pinhole), + ( + DisconnectedSpace::name(), + SubSpaceConnectionFlags::Pinhole | SubSpaceConnectionFlags::Disconnected, + ), + ( + ViewCoordinates::name(), + SubSpaceConnectionFlags::Pinhole | SubSpaceConnectionFlags::Disconnected, + ), + ] { + add_diff(&mut topo, "", &[name]); + let subspace = topo.subspace_for_entity(&"robo".into()); + assert_eq!(subspace.connection_to_parent, flags); + assert!(subspace.child_spaces.is_empty()); + assert!(subspace.parent_space.is_none()); + } } #[test] @@ -523,15 +525,13 @@ mod tests { let left_camera = topo.subspace_for_entity(&"robo/eyes/left/cam".into()); assert_eq!(left_camera.origin, "robo/eyes/left/cam".into()); - assert_eq!(left_camera.parent_space, Some(root.origin.hash())); - assert_eq!(left_camera.dimensionality, SubSpaceDimensionality::TwoD); - - assert_eq!(root.dimensionality, SubSpaceDimensionality::ThreeD); + assert_eq!(left_camera.parent_space, root.origin.hash()); assert_eq!( - root.child_spaces, - std::iter::once((left_camera.origin.clone(), SubSpaceConnectionFlags::Pinhole)) - .collect() + left_camera.connection_to_parent, + SubSpaceConnectionFlags::Pinhole ); + + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); assert!(root.parent_space.is_none()); } @@ -554,21 +554,21 @@ mod tests { let right_camera = topo.subspace_for_entity(&"robo/eyes/right/cam".into()); assert_eq!(right_camera.origin, "robo/eyes/right/cam".into()); - assert_eq!(right_camera.parent_space, Some(root.origin.hash())); - assert_eq!(right_camera.dimensionality, SubSpaceDimensionality::TwoD); + assert_eq!(right_camera.parent_space, root.origin.hash()); + assert_eq!( + right_camera.connection_to_parent, + SubSpaceConnectionFlags::Pinhole + ); + assert_eq!(left_camera.origin, "robo/eyes/left/cam".into()); + assert_eq!(left_camera.parent_space, root.origin.hash()); assert_eq!( - root.child_spaces, - [ - (left_camera.origin.clone(), SubSpaceConnectionFlags::Pinhole), - ( - right_camera.origin.clone(), - SubSpaceConnectionFlags::Pinhole - ) - ] - .into_iter() - .collect() + left_camera.connection_to_parent, + SubSpaceConnectionFlags::Pinhole ); + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); + assert!(root.parent_space.is_none()); + // If we make up entities that weren't logged we get the closest space assert_eq!( topo.subspace_for_entity(&"robo/eyes/right/cam/unheard".into()) @@ -593,24 +593,30 @@ mod tests { let right_camera = topo.subspace_for_entity(&"robo/eyes/right/cam".into()); assert_eq!(left_camera.origin, "robo/eyes/left/cam".into()); - assert_eq!(left_camera.parent_space, Some(root.origin.hash())); - assert_eq!(left_camera.dimensionality, SubSpaceDimensionality::TwoD); - assert_eq!(root.dimensionality, SubSpaceDimensionality::ThreeD); + assert_eq!(left_camera.parent_space, root.origin.hash()); + assert_eq!( + left_camera.connection_to_parent, + SubSpaceConnectionFlags::Disconnected | SubSpaceConnectionFlags::Pinhole + ); + assert_eq!(right_camera.parent_space, root.origin.hash()); + assert_eq!( + right_camera.connection_to_parent, + SubSpaceConnectionFlags::Pinhole + ); + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); + } + + // Add view coordinates to robo. + add_diff(&mut topo, "robo", &[ViewCoordinates::name()]); + { + let root = topo.subspace_for_entity(&EntityPath::root()); + assert!(root.parent_space.is_none()); + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); assert_eq!( - root.child_spaces, - [ - ( - left_camera.origin.clone(), - SubSpaceConnectionFlags::Disconnected | SubSpaceConnectionFlags::Pinhole - ), - ( - right_camera.origin.clone(), - SubSpaceConnectionFlags::Pinhole - ) - ] - .into_iter() - .collect() + root.heuristic_hints, + std::iter::once((EntityPath::from("robo"), HeuristicHints::ViewCoordinates3d)) + .collect() ); } } @@ -636,20 +642,11 @@ mod tests { let cam0 = topo.subspace_for_entity(&"cam0".into()); let cam1 = topo.subspace_for_entity(&"cam0/cam1".into()); - assert_eq!(cam0.dimensionality, SubSpaceDimensionality::TwoD); - assert_eq!(cam1.dimensionality, SubSpaceDimensionality::TwoD); - assert_eq!(cam0.parent_space, Some(EntityPath::root().hash())); - assert_eq!(cam1.parent_space, Some(cam0.origin.hash())); - - assert_eq!( - root.child_spaces, - std::iter::once((cam0.origin.clone(), SubSpaceConnectionFlags::Pinhole)).collect() - ); - - assert_eq!( - cam0.child_spaces, - std::iter::once((cam1.origin.clone(), SubSpaceConnectionFlags::Pinhole)).collect() - ); + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); + assert_eq!(cam0.connection_to_parent, SubSpaceConnectionFlags::Pinhole); + assert_eq!(cam1.connection_to_parent, SubSpaceConnectionFlags::Pinhole); + assert_eq!(cam0.parent_space, EntityPath::root().hash()); + assert_eq!(cam1.parent_space, cam0.origin.hash()); assert!(cam1.child_spaces.is_empty()); } } @@ -670,19 +667,14 @@ mod tests { check_paths_in_space(&topo, &["camera", "camera/image"], "camera"); let cam = topo.subspace_for_entity(&"camera".into()); - assert_eq!(cam.dimensionality, SubSpaceDimensionality::TwoD); - assert_eq!(cam.parent_space, Some(EntityPath::root().hash())); - - let root = topo.subspace_for_entity(&"stuff".into()); - assert_eq!(root.dimensionality, SubSpaceDimensionality::ThreeD); assert_eq!( - root.child_spaces, - std::iter::once(( - cam.origin.clone(), - SubSpaceConnectionFlags::Disconnected | SubSpaceConnectionFlags::Pinhole - )) - .collect() + cam.connection_to_parent, + SubSpaceConnectionFlags::Disconnected | SubSpaceConnectionFlags::Pinhole ); + assert_eq!(cam.parent_space, EntityPath::root().hash()); + + let root = topo.subspace_for_entity(&"stuff".into()); + assert_eq!(root.connection_to_parent, SubSpaceConnectionFlags::empty()); } fn add_diff(topo: &mut SpatialTopology, path: &str, components: &[ComponentName]) { diff --git a/tests/python/release_checklist/check_2d_heuristics.py b/tests/python/release_checklist/check_heuristics_2d.py similarity index 100% rename from tests/python/release_checklist/check_2d_heuristics.py rename to tests/python/release_checklist/check_heuristics_2d.py diff --git a/tests/python/release_checklist/check_heuristics_mixed_2d_and_3d.py b/tests/python/release_checklist/check_heuristics_mixed_2d_and_3d.py new file mode 100644 index 000000000000..55679e6b1bef --- /dev/null +++ b/tests/python/release_checklist/check_heuristics_mixed_2d_and_3d.py @@ -0,0 +1,67 @@ +from __future__ import annotations + +import os +from argparse import Namespace +from uuid import uuid4 + +import numpy as np +import rerun as rr + +README = """ +# 3D & 2D Heuristics + +This checks whether the heuristics do the right thing with mixed 2D and 3D data. + +Reset the blueprint to make sure you are viewing new heuristics and not a cached blueprint. + +### Action +You should see 4 space-views: + - 2D: `image1` with an all red image + - 2D: `image2` with an all green image + - 2D: `3d/camera` with an all blue image + - 3D: `3d` with: + - a 3D box + - a pinhole camera, showing the blue image + - no red or green image +""" + + +def log_image(path: str, height: int, width: int, color: tuple[int, int, int]) -> None: + image = np.zeros((height, width, 3), dtype=np.uint8) + image[:, :, :] = color + rr.log(path, rr.Image(image)) + + +def log_readme() -> None: + rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True) + + +def log_images() -> None: + log_image("image1", 20, 30, (255, 0, 0)) + log_image("image2", 20, 30, (0, 255, 0)) + + +def log_3d_scene() -> None: + rr.log("3d", rr.ViewCoordinates.RIGHT_HAND_Y_DOWN) + rr.log("3d/box", rr.Boxes3D(half_sizes=[1.0, 1.0, 1.0])) + rr.log("3d/camera", rr.Pinhole(focal_length=30, width=30, height=20)) + log_image("3d/camera", 20, 30, (0, 0, 255)) + + +def run(args: Namespace) -> None: + # TODO(cmc): I have no idea why this works without specifying a `recording_id`, but + # I'm not gonna rely on it anyway. + rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4()) + + log_readme() + log_images() + log_3d_scene() + + +if __name__ == "__main__": + import argparse + + parser = argparse.ArgumentParser(description="Interactive release checklist") + rr.script_add_args(parser) + args = parser.parse_args() + run(args)