Fix recommendation from one Space View class affecting recommendation from another

crates/re_types/Cargo.toml

@@ -71,6 +71,7 @@ itertools.workspace = true
 linked-hash-map.workspace = true
 mime_guess2.workspace = true
 ndarray.workspace = true
+nohash-hasher.workspace = true
 once_cell.workspace = true
 ply-rs.workspace = true
 smallvec.workspace = true

crates/re_types/src/blueprint/components/viewer_recommendation_hash_ext.rs

@@ -0,0 +1,10 @@
+use super::ViewerRecommendationHash;
+
+impl std::hash::Hash for ViewerRecommendationHash {
+    #[inline]
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        state.write_u64(self.0 .0);
+    }
+}
+
+impl nohash_hasher::IsEnabled for ViewerRecommendationHash {}

crates/re_viewer_context/src/space_view/spawn_heuristics.rs

@@ -1,7 +1,9 @@
-use re_log_types::{EntityPath, EntityPathFilter, EntityPathSubs};
+use re_log_types::{hash::Hash64, EntityPath, EntityPathFilter, EntityPathSubs};
+
+use crate::SpaceViewClassIdentifier;
 
 /// Properties of a space view that as recommended to be spawned by default via space view spawn heuristics.
-#[derive(Hash, Debug, Clone)]
+#[derive(Debug, Clone)]
 pub struct RecommendedSpaceView {
     pub origin: EntityPath,
     pub query_filter: EntityPathFilter,
@@ -42,4 +44,22 @@ impl RecommendedSpaceView {
     pub fn root() -> Self {
         Self::new_subtree(EntityPath::root())
     }
+
+    /// Hash together with the Space View class id to the `ViewerRecommendationHash` component.
+    ///
+    /// Recommendations are usually tied to a specific Space View class.
+    /// Therefore, to identify a recommendation for identification purposes, the class id should be included in the hash.
+    pub fn recommendation_hash(
+        &self,
+        class_id: SpaceViewClassIdentifier,
+    ) -> re_types::blueprint::components::ViewerRecommendationHash {
+        let Self {
+            origin,
+            query_filter,
+        } = self;
+
+        Hash64::hash((origin, query_filter, class_id))
+            .hash64()
+            .into()
+    }
 }

crates/re_viewport/src/viewport_blueprint.rs

@@ -8,7 +8,6 @@ use smallvec::SmallVec;
 
 use re_data_store::LatestAtQuery;
 use re_entity_db::EntityPath;
-use re_log_types::hash::Hash64;
 use re_query::query_archetype;
 use re_space_view::SpaceViewBlueprint;
 use re_types::blueprint::components::ViewerRecommendationHash;
@@ -59,7 +58,7 @@ pub struct ViewportBlueprint {
     auto_space_views: AtomicBool,
 
     /// Hashes of all recommended space views the viewer has already added and that should not be added again.
-    past_viewer_recommendation_hashes: IntSet<Hash64>,
+    past_viewer_recommendations: IntSet<ViewerRecommendationHash>,
 
     /// Channel to pass Blueprint mutation messages back to the [`crate::Viewport`]
     tree_action_sender: std::sync::mpsc::Sender<TreeAction>,
@@ -80,7 +79,7 @@ impl ViewportBlueprint {
             maximized,
             auto_layout,
             auto_space_views,
-            past_viewer_recommendations: past_viewer_recommendation_hashes,
+            past_viewer_recommendations,
         } = match query_archetype(blueprint_db.store(), query, &VIEWPORT_PATH.into())
             .and_then(|arch| arch.to_archetype())
         {
@@ -149,10 +148,9 @@ impl ViewportBlueprint {
             root_container,
         );
 
-        let past_viewer_recommendation_hashes = past_viewer_recommendation_hashes
+        let past_viewer_recommendations = past_viewer_recommendations
             .unwrap_or_default()
             .into_iter()
-            .map(|h| Hash64::from_u64(h.0 .0))
             .collect();
 
         ViewportBlueprint {
@@ -163,7 +161,7 @@ impl ViewportBlueprint {
             maximized: maximized.map(|id| id.0.into()),
             auto_layout,
             auto_space_views,
-            past_viewer_recommendation_hashes,
+            past_viewer_recommendations,
             tree_action_sender,
         }
     }
@@ -301,8 +299,8 @@ impl ViewportBlueprint {
             // Remove all space views that we already spawned via heuristic before.
             recommended_space_views.retain(|recommended_view| {
                 !self
-                    .past_viewer_recommendation_hashes
-                    .contains(&Hash64::hash(recommended_view))
+                    .past_viewer_recommendations
+                    .contains(&recommended_view.recommendation_hash(class_id))
             });
 
             // Each of the remaining recommendations would individually be a candidate for spawning if there were
@@ -321,11 +319,14 @@ impl ViewportBlueprint {
             // mean we should suddenly add `/camera/**` to the viewport.
             if !recommended_space_views.is_empty() {
                 let new_viewer_recommendation_hashes = self
-                    .past_viewer_recommendation_hashes
+                    .past_viewer_recommendations
                     .iter()
                     .cloned()
-                    .chain(recommended_space_views.iter().map(Hash64::hash))
-                    .map(|hash| ViewerRecommendationHash(hash.hash64().into()))
+                    .chain(
+                        recommended_space_views
+                            .iter()
+                            .map(|recommendation| recommendation.recommendation_hash(class_id)),
+                    )
                     .collect::<Vec<_>>();
 
                 ctx.save_blueprint_component(

tests/python/release_checklist/check_annotations.py

@@ -43,8 +43,6 @@ def log_annotations() -> None:
 
 
 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()

tests/python/release_checklist/check_container_hierarchy.py

@@ -83,8 +83,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_add_entity_to_new_space_view.py

@@ -44,8 +44,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_collapse_expand_all.py

@@ -40,8 +40,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_invalid_sub_container.py

@@ -38,8 +38,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_multi_selection.py

@@ -84,8 +84,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_single_selection.py

@@ -94,8 +94,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_single_selection_blueprint_tree.py

@@ -79,8 +79,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_context_menu_suggested_origin.py

@@ -66,8 +66,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_focus.py

@@ -40,8 +40,6 @@ def log_some_space_views() -> None:
 
 
 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()

tests/python/release_checklist/check_heuristics_2d.py

@@ -63,8 +63,6 @@ def log_images() -> None:
 
 
 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()

tests/python/release_checklist/check_heuristics_mixed_2d_and_3d.py

@@ -15,14 +15,15 @@
 Reset the blueprint to make sure you are viewing new heuristics and not a cached blueprint.
 
 ### Action
-You should see 4 space-views:
+You should see 5 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
+ - Text: This readme.
 """
 
 
@@ -49,8 +50,6 @@ def log_3d_scene() -> None:
 
 
 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()

tests/python/release_checklist/check_heuristics_mixed_all_root.py

@@ -0,0 +1,39 @@
+from __future__ import annotations
+
+import os
+from argparse import Namespace
+from uuid import uuid4
+
+import rerun as rr
+
+README = """
+# Mixed objects Heuristics with everything directly under root
+
+This checks whether the heuristics do the right thing with mixed 2D, 3D and test data.
+
+### Action
+You should see 4 space-views:
+ - 2D: Boxes and points, `$origin` == `/`
+ - 3D: 3D boxes, `$origin` == `/`
+ - Text log: A single log line, `$origin` == `/`
+ - Text: This readme, `$origin` == `/readme`
+"""
+
+
+def run(args: Namespace) -> None:
+    rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4())
+
+    rr.log("boxes3d", rr.Boxes3D(centers=[[0, 0, 0], [1, 1.5, 1.15], [3, 2, 1]], half_sizes=[0.5, 1, 0.5] * 3))
+    rr.log("boxes2d", rr.Boxes2D(centers=[[0, 0], [1.3, 0.5], [3, 2]], half_sizes=[0.5, 1] * 3))
+    rr.log("text_logs", rr.TextLog("Hello, world!", level=rr.TextLogLevel.INFO))
+    rr.log("points2d", rr.Points2D([[0, 0], [1, 1], [3, 2]], labels=["a", "b", "c"]))
+    rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True)
+
+
+if __name__ == "__main__":
+    import argparse
+
+    parser = argparse.ArgumentParser(description="Interactive release checklist")
+    rr.script_add_args(parser)
+    args = parser.parse_args()
+    run(args)

tests/python/release_checklist/check_hover_select_reset.py

@@ -80,8 +80,6 @@ def log_points_2d() -> None:
 
 
 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()

tests/python/release_checklist/check_parallelism_caching_reentrancy.py

@@ -138,8 +138,6 @@ def log_spatial() -> None:
 
 
 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()

tests/python/release_checklist/check_plot_overrides.py

@@ -54,8 +54,6 @@ def log_plots() -> None:
 
 
 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()

tests/python/release_checklist/check_scalar_clears.py

@@ -42,8 +42,6 @@ def log_plots() -> None:
 
 
 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()