Better origin heuristics for "Add entities to new space view" (#5423)

### What

Follow up to #5411. This PR provides a better heuristics for the the
default space origin with 2D and 3D space view. See the original PR for
the (updated) design decision section.

### 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/5423/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5423/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/5423/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/5423)
- [Docs
preview](https://rerun.io/preview/d50861028fe4f09576a16ed46196c04bf4e5dd04/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d50861028fe4f09576a16ed46196c04bf4e5dd04/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

crates/re_space_view_spatial/src/space_view_2d.rs

@@ -1,7 +1,7 @@
 use ahash::HashSet;
 use nohash_hasher::{IntMap, IntSet};
 
-use re_entity_db::{EntityProperties, EntityTree};
+use re_entity_db::{EntityDb, EntityProperties, EntityTree};
 use re_log_types::{EntityPath, EntityPathFilter};
 use re_types::{
     archetypes::{DepthImage, Image},
@@ -77,6 +77,20 @@ impl SpaceViewClass for SpatialSpaceView2D {
         re_viewer_context::SpaceViewClassLayoutPriority::High
     }
 
+    fn recommended_root_for_entities(
+        &self,
+        entities: &IntSet<EntityPath>,
+        entity_db: &EntityDb,
+    ) -> Option<EntityPath> {
+        let common_ancestor = EntityPath::common_ancestor_of(entities.iter());
+
+        // For a 2D space view, the origin of the subspace defined by the common ancestor is always
+        // better.
+        SpatialTopology::access(entity_db.store_id(), |topo| {
+            topo.subspace_for_entity(&common_ancestor).origin.clone()
+        })
+    }
+
     fn visualizable_filter_context(
         &self,
         space_origin: &EntityPath,

crates/re_space_view_spatial/src/space_view_3d.rs

@@ -1,6 +1,6 @@
 use itertools::Itertools;
 use nohash_hasher::IntSet;
-use re_entity_db::EntityProperties;
+use re_entity_db::{EntityDb, EntityProperties};
 use re_log_types::{EntityPath, EntityPathFilter};
 use re_types::{components::ViewCoordinates, Loggable};
 use re_viewer_context::{
@@ -71,6 +71,46 @@ impl SpaceViewClass for SpatialSpaceView3D {
         re_viewer_context::SpaceViewClassLayoutPriority::High
     }
 
+    fn recommended_root_for_entities(
+        &self,
+        entities: &IntSet<EntityPath>,
+        entity_db: &EntityDb,
+    ) -> Option<EntityPath> {
+        let common_ancestor = EntityPath::common_ancestor_of(entities.iter());
+
+        // For 3D space view, the origin of the subspace defined by the common ancestor is usually
+        // the best choice. However, if the subspace is defined by a pinhole, we should use its
+        // parent.
+        //
+        // Also, if a ViewCoordinate3D is logged somewhere between the common ancestor and the
+        // subspace origin, we use it as origin.
+        SpatialTopology::access(entity_db.store_id(), |topo| {
+            let subspace = topo.subspace_for_entity(&common_ancestor);
+
+            let subspace_origin = if subspace.supports_3d_content() {
+                Some(subspace)
+            } else {
+                topo.subspace_for_subspace_origin(subspace.parent_space)
+            }
+            .map(|subspace| subspace.origin.clone());
+
+            // Find the first ViewCoordinates3d logged, walking up from the common ancestor to the
+            // subspace origin.
+            EntityPath::incremental_walk(subspace_origin.as_ref(), &common_ancestor)
+                .collect::<Vec<_>>()
+                .into_iter()
+                .rev()
+                .find(|path| {
+                    subspace
+                        .heuristic_hints
+                        .get(path)
+                        .is_some_and(|hint| hint.contains(HeuristicHints::ViewCoordinates3d))
+                })
+                .or(subspace_origin)
+        })
+        .flatten()
+    }
+
     fn visualizable_filter_context(
         &self,
         space_origin: &EntityPath,

crates/re_viewer_context/src/space_view/dyn_space_view_class.rs

@@ -1,3 +1,4 @@
+use nohash_hasher::IntSet;
 use re_entity_db::{EntityProperties, EntityPropertyMap};
 use re_log_types::EntityPath;
 use re_types::ComponentName;
@@ -105,6 +106,16 @@ pub trait DynSpaceViewClass: Send + Sync {
     /// Controls how likely this space view will get a large tile in the ui.
     fn layout_priority(&self) -> SpaceViewClassLayoutPriority;
 
+    /// Determines a suitable origin given the provided set of entities.
+    ///
+    /// This function only considers the transform topology, disregarding the actual visualizability
+    /// of the entities (for this, use [`Self::visualizable_filter_context`]).
+    fn recommended_root_for_entities(
+        &self,
+        _entities: &IntSet<EntityPath>,
+        _entity_db: &re_entity_db::EntityDb,
+    ) -> Option<EntityPath>;
+
     /// Create context object that is passed to all of this classes visualizers
     /// to determine whether they can be visualized
     ///

crates/re_viewer_context/src/space_view/space_view_class.rs

@@ -1,4 +1,6 @@
-use re_entity_db::{EntityProperties, EntityPropertyMap};
+use nohash_hasher::IntSet;
+
+use re_entity_db::{EntityDb, EntityProperties, EntityPropertyMap};
 use re_log_types::EntityPath;
 use re_types::ComponentName;
 
@@ -65,6 +67,18 @@ pub trait SpaceViewClass: std::marker::Sized + Send + Sync {
     /// Controls how likely this space view will get a large tile in the ui.
     fn layout_priority(&self) -> crate::SpaceViewClassLayoutPriority;
 
+    /// Determines a suitable origin given the provided set of entities.
+    ///
+    /// This function only considers the transform topology, disregarding the actual visualizability
+    /// of the entities (for this, use [`Self::visualizable_filter_context`]).
+    fn recommended_root_for_entities(
+        &self,
+        _entities: &IntSet<EntityPath>,
+        _entity_db: &re_entity_db::EntityDb,
+    ) -> Option<EntityPath> {
+        Some(EntityPath::root())
+    }
+
     /// Create context object that is passed to all of this classes visualizers
     /// to determine whether they can be visualized.
     ///
@@ -220,6 +234,15 @@ impl<T: SpaceViewClass + 'static> DynSpaceViewClass for T {
         self.layout_priority()
     }
 
+    #[inline]
+    fn recommended_root_for_entities(
+        &self,
+        entities: &IntSet<EntityPath>,
+        entity_db: &EntityDb,
+    ) -> Option<EntityPath> {
+        self.recommended_root_for_entities(entities, entity_db)
+    }
+
     #[inline]
     fn visualizable_filter_context(
         &self,

crates/re_viewport/src/context_menu/actions.rs

@@ -452,8 +452,8 @@ fn recommended_space_views_for_selection(
     let entities_of_interest = ctx
         .selection
         .iter()
-        .filter_map(|(item, _)| item.entity_path())
-        .collect::<Vec<_>>();
+        .filter_map(|(item, _)| item.entity_path().cloned())
+        .collect::<IntSet<_>>();
 
     let mut output: IntSet<SpaceViewClassIdentifier> = IntSet::default();
 
@@ -463,12 +463,19 @@ fn recommended_space_views_for_selection(
         space_view_class_registry.applicable_entities_for_visualizer_systems(entity_db.store_id());
 
     for entry in space_view_class_registry.iter_registry() {
+        let Some(suggested_root) = entry
+            .class
+            .recommended_root_for_entities(&entities_of_interest, entity_db)
+        else {
+            continue;
+        };
+
         let visualizable_entities = determine_visualizable_entities(
             &applicable_entities_per_visualizer,
             entity_db,
             &space_view_class_registry.new_visualizer_collection(entry.class.identifier()),
             &*entry.class,
-            &EntityPath::root(),
+            &suggested_root,
         );
 
         // We consider a space view class to be recommended if all selected entities are
@@ -498,18 +505,24 @@ fn create_space_view_for_selected_entities(
     ctx: &ContextMenuContext<'_>,
     identifier: SpaceViewClassIdentifier,
 ) {
-    let origin = EntityPath::root();
+    let entities_of_interest = ctx
+        .selection
+        .iter()
+        .filter_map(|(item, _)| item.entity_path().cloned())
+        .collect::<IntSet<_>>();
+
+    let origin = ctx
+        .viewer_context
+        .space_view_class_registry
+        .get_class_or_log_error(&identifier)
+        .recommended_root_for_entities(&entities_of_interest, ctx.viewer_context.entity_db)
+        .unwrap_or_else(EntityPath::root);
 
     let mut filter = EntityPathFilter::default();
-    ctx.selection
-        .iter()
-        .filter_map(|(item, _)| item.entity_path())
-        .for_each(|path| {
-            filter.add_rule(
-                RuleEffect::Include,
-                EntityPathRule::including_subtree(path.clone()),
-            );
-        });
+
+    for path in entities_of_interest {
+        filter.add_rule(RuleEffect::Include, EntityPathRule::including_subtree(path));
+    }
 
     let target_container_id = ctx
         .clicked_item_enclosing_container_id_and_position()

tests/python/release_checklist/check_context_menu_suggested_origin.py

@@ -0,0 +1,83 @@
+from __future__ import annotations
+
+import math
+import os
+from argparse import Namespace
+from uuid import uuid4
+
+import numpy as np
+import rerun as rr
+
+README = """
+# Context Menu - Test the origin selection heuristics
+
+Right click on each of the following entities and check that for the given space view class, the resulting suggested origin is as expected.
+
+```plaintext
+ENTITY                      CLASS       EXPECTED ORIGIN
+
+/                           3D          /
+/world                      3D          /world
+/world/camera               3D          /world
+/world/camera/image         3D          /world
+/world/camera/keypoint      3D          /world
+
+/world                      2D          <not suggested>
+/world/camera               2D          <not suggested>
+/world/camera/image         2D          /world/camera/image
+/world/camera/keypoint      2D          /world/camera/image
+```
+"""
+
+
+def log_readme() -> None:
+    rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True)
+
+
+def log_some_space_views() -> None:
+    rr.set_time_sequence("frame_nr", 0)
+    rr.log("/", rr.Boxes3D(centers=[0, 0, 0], half_sizes=[1, 1, 1]))
+    rr.log("/world", rr.ViewCoordinates.RIGHT_HAND_Y_DOWN, timeless=True)
+    rr.log(
+        "/world/camera/image",
+        # rr.Pinhole(fov_y=0.7853982, aspect_ratio=1, camera_xyz=rr.ViewCoordinates.RUB, resolution=[10, 10]),
+        rr.Pinhole(
+            resolution=[10, 10],
+            focal_length=[4, 4],
+            principal_point=[5, 5],
+        ),
+    )
+    rr.log("/world/camera/image", rr.Image(np.random.rand(10, 10, 3)))
+    rr.log("/world/camera/image/keypoint", rr.Points2D(np.random.rand(10, 2) * 10, radii=0.5))
+
+    for i in range(100):
+        rr.set_time_sequence("frame_nr", i)
+        angle = 2 * math.pi * i / 100
+
+        rr.log(
+            "/world/camera",
+            rr.Transform3D(
+                rr.TranslationRotationScale3D(
+                    translation=[math.cos(angle), math.sin(angle), 0],
+                    rotation=rr.RotationAxisAngle(axis=[0, 0, 1], angle=angle),
+                )
+            ),
+        )
+
+
+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_some_space_views()
+
+
+if __name__ == "__main__":
+    import argparse
+
+    parser = argparse.ArgumentParser(description="Interactive release checklist")
+    rr.script_add_args(parser)
+    args = parser.parse_args()
+    run(args)