When duplicating a SpaceViewBlueprint, also duplicate its Query

crates/re_space_view/src/data_query_blueprint.rs

@@ -29,7 +29,14 @@ use crate::{
 /// The results of recursive expressions are only included if they are found within the [`EntityTree`]
 /// and for which there is a valid `ViewPart` system. This keeps recursive expressions from incorrectly
 /// picking up irrelevant data within the tree.
-#[derive(Clone, PartialEq, Eq)]
+///
+/// Note: [`DataQueryBlueprint`] doesn't implement Clone because it stores an internal
+/// uuid used for identifying the path of its data in the blueprint store. It's ambiguous
+/// whether the intent is for a clone to write to the same place.
+///
+/// If you want a new space view otherwise identical to an existing one, use
+/// [`DataQueryBlueprint::duplicate`].
+#[derive(PartialEq, Eq)]
 pub struct DataQueryBlueprint {
     pub id: DataQueryId,
     pub space_view_class_identifier: SpaceViewClassIdentifier,
@@ -78,6 +85,15 @@ impl DataQueryBlueprint {
         })
     }
 
+    /// Creates a new [`DataQueryBlueprint`] with a the same contents, but a different [`DataQueryId`]
+    pub fn duplicate(&self) -> Self {
+        Self {
+            id: DataQueryId::random(),
+            space_view_class_identifier: self.space_view_class_identifier,
+            expressions: self.expressions.clone(),
+        }
+    }
+
     pub fn clear(&self, ctx: &ViewerContext<'_>) {
         let clear = Clear::recursive();
         ctx.save_blueprint_component(&self.id.as_entity_path(), clear.is_recursive);

crates/re_viewer/src/ui/selection_panel.rs

@@ -522,8 +522,7 @@ fn blueprint_ui(
                     .clicked()
                 {
                     if let Some(space_view) = viewport.blueprint.space_view(space_view_id) {
-                        let mut new_space_view = space_view.clone();
-                        new_space_view.id = SpaceViewId::random();
+                        let new_space_view = space_view.duplicate();
                         viewport.blueprint.add_space_views(std::iter::once(new_space_view), ctx, &mut viewport.deferred_tree_actions);
                         viewport.blueprint.mark_user_interaction(ctx);
                     }

crates/re_viewport/src/space_view.rs

@@ -22,7 +22,13 @@ use crate::viewport_blueprint::add_delta_from_single_component;
 // ----------------------------------------------------------------------------
 
 /// A view of a space.
-#[derive(Clone)]
+///
+/// Note: [`SpaceViewBlueprint`] doesn't implement Clone because it stores an internal
+/// uuid used for identifying the path of its data in the blueprint store. It's ambiguous
+/// whether the intent is for a clone to write to the same place.
+///
+/// If you want a new space view otherwise identical to an existing one, use
+/// [`SpaceViewBlueprint::duplicate`].
 pub struct SpaceViewBlueprint {
     pub id: SpaceViewId,
     pub display_name: String,
@@ -173,6 +179,20 @@ impl SpaceViewBlueprint {
             ));
     }
 
+    /// Creates a new [`SpaceViewBlueprint`] with a the same contents, but a different [`SpaceViewId`]
+    ///
+    /// Also duplicates all of the queries in the space view.
+    pub fn duplicate(&self) -> Self {
+        Self {
+            id: SpaceViewId::random(),
+            display_name: self.display_name.clone(),
+            class_identifier: self.class_identifier,
+            space_origin: self.space_origin.clone(),
+            queries: self.queries.iter().map(|q| q.duplicate()).collect(),
+            entities_determined_by_user: self.entities_determined_by_user,
+        }
+    }
+
     pub fn clear(&self, ctx: &ViewerContext<'_>) {
         let clear = Clear::recursive();
         ctx.save_blueprint_component(&self.entity_path(), clear.is_recursive);

crates/re_viewport/src/space_view_heuristics.rs

@@ -311,35 +311,42 @@ pub fn default_created_space_views(
 
         // `AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot` means we're competing with other candidates for the same root.
         if let AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(score) = spawn_heuristic {
-            let mut should_spawn_new = true;
+            // [`SpaceViewBlueprint`]s don't implement clone so wrap in an option so we can
+            // track whether or not we've consumed it.
+            let mut candidate_still_considered = Some(candidate);
+
             for (prev_candidate, prev_spawn_heuristic) in &mut space_views {
-                if prev_candidate.space_origin == candidate.space_origin {
-                    #[allow(clippy::match_same_arms)]
-                    match prev_spawn_heuristic {
-                        AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(prev_score) => {
-                            // If we're competing with a candidate for the same root, we either replace a lower score, or we yield.
-                            should_spawn_new = false;
-                            if *prev_score < score {
-                                // Replace the previous candidate with this one.
-                                *prev_candidate = candidate.clone();
-                                *prev_spawn_heuristic = spawn_heuristic;
-                            } else {
-                                // We have a lower score, so we don't spawn.
+                if let Some(candidate) = candidate_still_considered.take() {
+                    if prev_candidate.space_origin == candidate.space_origin {
+                        #[allow(clippy::match_same_arms)]
+                        match prev_spawn_heuristic {
+                            AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(prev_score) => {
+                                // If we're competing with a candidate for the same root, we either replace a lower score, or we yield.
+                                if *prev_score < score {
+                                    // Replace the previous candidate with this one.
+                                    *prev_candidate = candidate;
+                                    *prev_spawn_heuristic = spawn_heuristic;
+                                }
+
+                                // Either way we're done with this candidate.
                                 break;
                             }
-                        }
-                        AutoSpawnHeuristic::AlwaysSpawn => {
-                            // We can live side by side with always-spawn candidates.
-                        }
-                        AutoSpawnHeuristic::NeverSpawn => {
-                            // Never spawn candidates should not be in the list, this is weird!
-                            // But let's not fail on this since our heuristics are not perfect anyways.
+                            AutoSpawnHeuristic::AlwaysSpawn => {
+                                // We can live side by side with always-spawn candidates.
+                            }
+                            AutoSpawnHeuristic::NeverSpawn => {
+                                // Never spawn candidates should not be in the list, this is weird!
+                                // But let's not fail on this since our heuristics are not perfect anyways.
+                            }
                         }
                     }
+
+                    // If we didn't hit the break condition, continue to consider the candidate
+                    candidate_still_considered = Some(candidate);
                 }
             }
 
-            if should_spawn_new {
+            if let Some(candidate) = candidate_still_considered {
                 // Spatial views with images get extra treatment as well.
                 if is_spatial_2d_class(candidate.class_identifier()) {
                     #[derive(Hash, PartialEq, Eq)]