From 99ddfb16d9babb818dc1374bd86bbd00b6954b2b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 3 Feb 2023 11:24:37 +0100 Subject: [PATCH 1/4] Allow adding entities from arbitrary space paths Fix bug in space info reachability check (which drives the entity picker) --- .../src/component_types/transform.rs | 10 +++ crates/re_viewer/src/misc/space_info.rs | 74 +++++++++++-------- crates/re_viewer/src/misc/transform_cache.rs | 26 +++---- crates/re_viewer/src/ui/space_view.rs | 11 --- .../src/ui/space_view_entity_picker.rs | 24 +++--- crates/re_viewer/src/ui/viewport.rs | 25 ++++--- 6 files changed, 89 insertions(+), 81 deletions(-) diff --git a/crates/re_log_types/src/component_types/transform.rs b/crates/re_log_types/src/component_types/transform.rs index 2e37e95c0dec..e7b1020d319c 100644 --- a/crates/re_log_types/src/component_types/transform.rs +++ b/crates/re_log_types/src/component_types/transform.rs @@ -51,6 +51,16 @@ pub struct Rigid3 { #[cfg(feature = "glam")] impl Rigid3 { + pub const IDENTITY: Rigid3 = Rigid3 { + rotation: Quaternion { + x: 1.0, + y: 0.0, + z: 0.0, + w: 0.0, + }, + translation: Vec3D([0.0, 0.0, 0.0]), + }; + #[inline] pub fn new_parent_from_child(parent_from_child: macaw::IsoTransform) -> Self { Self { diff --git a/crates/re_viewer/src/misc/space_info.rs b/crates/re_viewer/src/misc/space_info.rs index 78336bf634a1..9b4c4aefaf6f 100644 --- a/crates/re_viewer/src/misc/space_info.rs +++ b/crates/re_viewer/src/misc/space_info.rs @@ -98,7 +98,7 @@ impl SpaceInfo { /// Information about all spaces. /// /// This is gathered by analyzing the transform hierarchy of the entities: -/// For every child of the root there is a space info. +/// For every child of the root there is a space info, as well as the root itself. /// Each of these we walk down recursively, every time a transform is encountered, we create another space info. /// /// Expected to be recreated every frame (or whenever new data is available). @@ -163,20 +163,40 @@ impl SpaceInfoCollection { let mut spaces_info = Self::default(); - for tree in entity_db.tree.children.values() { - // Each root entity is its own space (or should be) - - if query_transform(entity_db, &tree.path, &query).is_some() { - re_log::warn_once!( - "Root entity '{}' has a _transform - this is not allowed!", - tree.path - ); - } + // The root itself. + // To make our heuristics work we pretend direct child of the root has a transform, + // breaking the pattern applied for everything else where we create a SpaceInfo once we hit a transform. + // + // TODO(andreas): Our dependency on SpaceInfo in this way is quite telling - we should be able to create SpaceVies without having a corresponding space view. + // Currently too many things depend on every SpaceView being backed up by a concrete SpaceInfo on its space path. + if query_transform(entity_db, &EntityPath::root(), &query).is_some() { + re_log::warn_once!("The root entity has a _transform! This will have no effect. Did you mean to apply the transform elsewhere?"); + } + let mut root_space_info = SpaceInfo::new(EntityPath::root()); + root_space_info + .descendants_without_transform + .insert(EntityPath::root()); + for tree in entity_db.tree.children.values() { let mut space_info = SpaceInfo::new(tree.path.clone()); + let transform = query_transform(entity_db, &EntityPath::root(), &query) + .unwrap_or(Transform::Rigid3(re_log_types::Rigid3::IDENTITY)); + space_info.parent = Some((EntityPath::root(), transform.clone())); + space_info + .descendants_without_transform + .insert(tree.path.clone()); + + root_space_info + .child_spaces + .insert(tree.path.clone(), transform); + + // From there on, every time there is a transform we create a space info. add_children(entity_db, &mut spaces_info, &mut space_info, tree, &query); spaces_info.spaces.insert(tree.path.clone(), space_info); } + spaces_info + .spaces + .insert(EntityPath::root(), root_space_info); for (entity_path, space_info) in &mut spaces_info.spaces { space_info.coordinates = query_view_coordinates(entity_db, entity_path, &query); @@ -209,8 +229,6 @@ impl SpaceInfoCollection { /// /// For how, you need to check [`crate::misc::TransformCache`]! /// Note that in any individual frame, entities may or may not be reachable. - /// - /// If `from` and `to_reference` are not under the same root branch, they are regarded as [`UnreachableTransform::Unconnected`] pub fn is_reachable_by_transform( &self, from: &EntityPath, @@ -218,31 +236,29 @@ impl SpaceInfoCollection { ) -> Result<(), UnreachableTransform> { crate::profile_function!(); - // By convention we regard the global hierarchy as a forest - don't allow breaking out of the current tree. - if from.iter().next() != to_reference.iter().next() { - return Err(UnreachableTransform::Unconnected); - } - // Get closest space infos for the given entity paths. let Some(mut from_space) = self.get_first_parent_with_info(from) else { re_log::warn_once!("{} not part of space infos", from); - return Err(UnreachableTransform::Unconnected); + return Err(UnreachableTransform::UnknownSpaceInfo); }; let Some(mut to_reference_space) = self.get_first_parent_with_info(to_reference) else { re_log::warn_once!("{} not part of space infos", to_reference); - return Err(UnreachableTransform::Unconnected); + return Err(UnreachableTransform::UnknownSpaceInfo); }; - // If this is not true, the path we're querying, `from`, is outside of the tree the reference node. - // Note that this means that all transforms on the way are inversed! - let from_is_child_of_reference = from.is_descendant_of(to_reference); - // Reachability is (mostly) commutative! - // This means we can simply walk from the lower node to the parent until we're on the same node + // This means we can simply walk from both nodes up until we find a common ancestor! // If we haven't encountered any obstacles, we're fine! let mut encountered_pinhole = false; while from_space.path != to_reference_space.path { - let parent = if from_is_child_of_reference { + // Decide if we should walk up "from" or "to_reference" + // If "from" is a descendant of "to_reference", we walk up "from" + // Otherwise we walk up on "to_reference". + // + // If neither is a descendant of the other it doesn't matter which one we walk up, since we eventually going to hit common ancestor! + let walk_up_from = from_space.path.is_descendant_of(&to_reference_space.path); + + let parent = if walk_up_from { &from_space.parent } else { &to_reference_space.parent @@ -258,7 +274,7 @@ impl SpaceInfoCollection { Err(UnreachableTransform::NestedPinholeCameras) } else { encountered_pinhole = true; - if pinhole.resolution.is_none() && !from_is_child_of_reference { + if pinhole.resolution.is_none() && !walk_up_from { Err(UnreachableTransform::InversePinholeCameraWithoutResolution) } else { Ok(()) @@ -269,10 +285,10 @@ impl SpaceInfoCollection { let Some(parent_space) = self.get(parent_path) else { re_log::warn_once!("{} not part of space infos", parent_path); - return Err(UnreachableTransform::Unconnected); + return Err(UnreachableTransform::UnknownSpaceInfo); }; - if from_is_child_of_reference { + if walk_up_from { from_space = parent_space; } else { to_reference_space = parent_space; @@ -283,7 +299,7 @@ impl SpaceInfoCollection { from, to_reference ); - return Err(UnreachableTransform::Unconnected); + return Err(UnreachableTransform::UnknownSpaceInfo); } } diff --git a/crates/re_viewer/src/misc/transform_cache.rs b/crates/re_viewer/src/misc/transform_cache.rs index 654d6b716da5..68be7d783a7d 100644 --- a/crates/re_viewer/src/misc/transform_cache.rs +++ b/crates/re_viewer/src/misc/transform_cache.rs @@ -24,16 +24,15 @@ pub struct TransformCache { unreachable_descendants: Vec<(EntityPath, UnreachableTransform)>, /// The first parent of reference_path that is no longer reachable. - first_unreachable_parent: (EntityPath, UnreachableTransform), + first_unreachable_parent: Option<(EntityPath, UnreachableTransform)>, } #[derive(Clone, Copy)] pub enum UnreachableTransform { - /// Not part of the hierarchy at all. - /// TODO(andreas): This is only needed for the "breaking out of the root" case that we want to scrap. - /// After that it's impossible to be unreachable since there can always be an identity matrix. - /// We already allow walking over unlogged paths (because why not) - Unconnected, + /// [`crate::misc::SpaceInfoCollection`] is outdated and can't find a corresponding space info for the given path. + /// + /// If at all, this should only happen for a single frame until space infos are rebuilt. + UnknownSpaceInfo, /// More than one pinhole camera between this and the reference space. NestedPinholeCameras, /// Exiting out of a space with a pinhole camera that doesn't have a resolution is not supported. @@ -45,8 +44,8 @@ pub enum UnreachableTransform { impl std::fmt::Display for UnreachableTransform { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(match self { - Self::Unconnected => - "No entity path connection from this space view.", + Self::UnknownSpaceInfo => + "Can't determine transform because internal data structures are not in a valid state. Please file an issue on https://github.com/rerun-io/rerun/", Self::NestedPinholeCameras => "Can't display entities under nested pinhole cameras.", Self::UnknownTransform => @@ -75,7 +74,7 @@ impl TransformCache { reference_path: root_path.clone(), reference_from_entity_per_entity: Default::default(), unreachable_descendants: Default::default(), - first_unreachable_parent: (EntityPath::root(), UnreachableTransform::Unconnected), + first_unreachable_parent: None, }; // Find the entity path tree for the root. @@ -113,13 +112,6 @@ impl TransformCache { let mut encountered_pinhole = false; let mut reference_from_ancestor = glam::Mat4::IDENTITY; while let Some(parent_tree) = parent_tree_stack.pop() { - // By convention we regard the global hierarchy as a forest - don't allow breaking out of the current tree. - if parent_tree.path.is_root() { - transforms.first_unreachable_parent = - (parent_tree.path.clone(), UnreachableTransform::Unconnected); - break; - } - // 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! match inverse_transform_at( @@ -130,7 +122,7 @@ impl TransformCache { ) { Err(unreachable_reason) => { transforms.first_unreachable_parent = - (parent_tree.path.clone(), unreachable_reason); + Some((parent_tree.path.clone(), unreachable_reason)); break; } Ok(None) => {} diff --git a/crates/re_viewer/src/ui/space_view.rs b/crates/re_viewer/src/ui/space_view.rs index d022c34d6882..f3cd6f9f99fb 100644 --- a/crates/re_viewer/src/ui/space_view.rs +++ b/crates/re_viewer/src/ui/space_view.rs @@ -41,11 +41,6 @@ pub struct SpaceView { pub id: SpaceViewId, pub name: String, - /// Everything under this root *can* be shown in the space view. - /// - /// TODO(andreas): We decided to remove this concept. - pub root_path: EntityPath, - /// The "anchor point" of this space view. /// It refers to a [`SpaceInfo`] which forms our reference point for all scene->world transforms in this space view. /// I.e. the position of this entity path in space forms the origin of the coordinate system in this space view. @@ -71,11 +66,6 @@ impl SpaceView { space_info: &SpaceInfo, queries_entities: &[EntityPath], ) -> Self { - let root_path = space_info.path.iter().next().map_or_else( - || space_info.path.clone(), - |c| EntityPath::from(vec![c.clone()]), - ); - let name = if queries_entities.len() == 1 { // a single entity in this space-view - name the space after it queries_entities[0].to_string() @@ -90,7 +80,6 @@ impl SpaceView { Self { name, id: SpaceViewId::random(), - root_path, space_path: space_info.path.clone(), data_blueprint: data_blueprint_tree, view_state: ViewState::default(), diff --git a/crates/re_viewer/src/ui/space_view_entity_picker.rs b/crates/re_viewer/src/ui/space_view_entity_picker.rs index 63f9c2ec1952..7793db512a37 100644 --- a/crates/re_viewer/src/ui/space_view_entity_picker.rs +++ b/crates/re_viewer/src/ui/space_view_entity_picker.rs @@ -75,20 +75,18 @@ impl SpaceViewEntityPicker { fn add_entities_ui(ctx: &mut ViewerContext<'_>, ui: &mut egui::Ui, space_view: &mut SpaceView) { let spaces_info = SpaceInfoCollection::new(&ctx.log_db.entity_db); - // TODO(andreas): remove use space_view.root_path, just show everything - if let Some(tree) = ctx.log_db.entity_db.tree.subtree(&space_view.root_path) { - let entities_add_info = create_entity_add_info(ctx, tree, space_view, &spaces_info); + let tree = &ctx.log_db.entity_db.tree; + let entities_add_info = create_entity_add_info(ctx, tree, space_view, &spaces_info); - add_entities_tree_ui( - ctx, - ui, - &spaces_info, - &tree.path.to_string(), - tree, - space_view, - &entities_add_info, - ); - } + add_entities_tree_ui( + ctx, + ui, + &spaces_info, + &tree.path.to_string(), + tree, + space_view, + &entities_add_info, + ); } fn add_entities_tree_ui( diff --git a/crates/re_viewer/src/ui/viewport.rs b/crates/re_viewer/src/ui/viewport.rs index 0757614fc68a..7dbc56808bbf 100644 --- a/crates/re_viewer/src/ui/viewport.rs +++ b/crates/re_viewer/src/ui/viewport.rs @@ -521,24 +521,27 @@ impl Viewport { let mut space_views = Vec::new(); for space_view_candidate in Self::all_possible_space_views(ctx, spaces_info) { + // Skip root space for now, messes things up. + if space_view_candidate.space_path.is_root() { + continue; + } + let Some(space_info) = spaces_info.get(&space_view_candidate.space_path) else { // Should never happen. continue; }; - // If it doesn't contain anything but the transform itself, skip, - if space_info.descendants_without_transform.is_empty() { - continue; - } - if space_view_candidate.category == ViewCategory::Spatial { - // Skip if connection to parent is via rigid (too trivial for a new space view!) - if let Some(parent_transform) = space_info.parent_transform() { - match parent_transform { - re_log_types::Transform::Rigid3(_) => { - continue; + // For every item that isn't a direct descendant of the root, skip if connection to parent is via rigid (too trivial for a new space view!) + if space_info.path.parent() != Some(EntityPath::root()) { + if let Some(parent_transform) = space_info.parent_transform() { + match parent_transform { + re_log_types::Transform::Rigid3(_) => { + continue; + } + re_log_types::Transform::Pinhole(_) + | re_log_types::Transform::Unknown => {} } - re_log_types::Transform::Pinhole(_) | re_log_types::Transform::Unknown => {} } } From 3061773accc77fe6c08fa6607d98591be0cb308b Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 3 Feb 2023 11:49:34 +0100 Subject: [PATCH 2/4] improve group naming for now unrestricted object addings --- crates/re_viewer/src/ui/data_blueprint.rs | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/crates/re_viewer/src/ui/data_blueprint.rs b/crates/re_viewer/src/ui/data_blueprint.rs index 3cb411c10171..44305198d82b 100644 --- a/crates/re_viewer/src/ui/data_blueprint.rs +++ b/crates/re_viewer/src/ui/data_blueprint.rs @@ -221,7 +221,7 @@ impl DataBlueprintTree { } else { // Otherwise, create a new group which only contains this entity and add the group to the hierarchy. let new_group = self.groups.insert(DataBlueprintGroup { - display_name: path_to_group_name(path, base_path), + display_name: path_to_group_name(path), ..Default::default() }); self.add_group_to_hierarchy_recursively(new_group, path, base_path); @@ -271,8 +271,7 @@ impl DataBlueprintTree { // Short circuit to the root group at base_path. // If the entity is outside of the base path we would walk up all the way to the root - // That's ok but we want to stop one element short (since a space view can only show elements under a shared path) - if &parent_path == base_path || parent_path.iter().count() == 1 { + if &parent_path == base_path { parent_path = EntityPath::root(); } @@ -289,7 +288,7 @@ impl DataBlueprintTree { std::collections::hash_map::Entry::Vacant(vacant_mapping) => { let parent_group = self.groups.insert(DataBlueprintGroup { - display_name: path_to_group_name(&parent_path, base_path), + display_name: path_to_group_name(&parent_path), children: smallvec![new_group], ..Default::default() }); @@ -398,18 +397,6 @@ impl DataBlueprintTree { } } -fn path_to_group_name(path: &EntityPath, base_path: &EntityPath) -> String { - // Root should never be pasesd in here, but handle it gracefully regardless. - let name = path.iter().last().map_or("/".to_owned(), |c| c.to_string()); - - // How many steps until a common ancestor? - let mut num_steps_until_common_ancestor = 0; - let mut common_ancestor = base_path.clone(); - while !path.is_descendant_of(&common_ancestor) { - // Can never go beyond root because everything is a descendent of root, therefore this is safe. - common_ancestor = common_ancestor.parent().unwrap(); - num_steps_until_common_ancestor += 1; - } - - format!("{}{}", "../".repeat(num_steps_until_common_ancestor), name) +fn path_to_group_name(path: &EntityPath) -> String { + path.iter().last().map_or("/".to_owned(), |c| c.to_string()) } From 273f77e1b4573f591eb1094ef2df20db3767562a Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 3 Feb 2023 14:23:30 +0100 Subject: [PATCH 3/4] doc fix --- crates/re_viewer/src/misc/transform_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewer/src/misc/transform_cache.rs b/crates/re_viewer/src/misc/transform_cache.rs index 68be7d783a7d..00c00649c7aa 100644 --- a/crates/re_viewer/src/misc/transform_cache.rs +++ b/crates/re_viewer/src/misc/transform_cache.rs @@ -29,7 +29,7 @@ pub struct TransformCache { #[derive(Clone, Copy)] pub enum UnreachableTransform { - /// [`crate::misc::SpaceInfoCollection`] is outdated and can't find a corresponding space info for the given path. + /// [`super::space_info::SpaceInfoCollection`] is outdated and can't find a corresponding space info for the given path. /// /// If at all, this should only happen for a single frame until space infos are rebuilt. UnknownSpaceInfo, From a15805a83f6fc435d409e9b051db535a40106011 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 3 Feb 2023 19:25:03 +0100 Subject: [PATCH 4/4] comment & error fixes --- crates/re_viewer/src/misc/space_info.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/re_viewer/src/misc/space_info.rs b/crates/re_viewer/src/misc/space_info.rs index 9b4c4aefaf6f..16d86597ff56 100644 --- a/crates/re_viewer/src/misc/space_info.rs +++ b/crates/re_viewer/src/misc/space_info.rs @@ -167,10 +167,10 @@ impl SpaceInfoCollection { // To make our heuristics work we pretend direct child of the root has a transform, // breaking the pattern applied for everything else where we create a SpaceInfo once we hit a transform. // - // TODO(andreas): Our dependency on SpaceInfo in this way is quite telling - we should be able to create SpaceVies without having a corresponding space view. + // TODO(andreas): Our dependency on SpaceInfo in this way is quite telling - we should be able to create a SpaceView without having a corresponding SpaceInfo // Currently too many things depend on every SpaceView being backed up by a concrete SpaceInfo on its space path. if query_transform(entity_db, &EntityPath::root(), &query).is_some() { - re_log::warn_once!("The root entity has a _transform! This will have no effect. Did you mean to apply the transform elsewhere?"); + re_log::warn_once!("The root entity has a 'transform' component! This will have no effect. Did you mean to apply the transform elsewhere?"); } let mut root_space_info = SpaceInfo::new(EntityPath::root()); root_space_info @@ -190,7 +190,6 @@ impl SpaceInfoCollection { .child_spaces .insert(tree.path.clone(), transform); - // From there on, every time there is a transform we create a space info. add_children(entity_db, &mut spaces_info, &mut space_info, tree, &query); spaces_info.spaces.insert(tree.path.clone(), space_info); }