From f128e543a4e06edee55f1a089b1b6a9837ca43b0 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 01:41:59 +0100 Subject: [PATCH] When duplicating a SpaceViewBlueprint, also duplicate its Query --- .../re_space_view/src/data_query_blueprint.rs | 18 ++++++- crates/re_viewer/src/ui/selection_panel.rs | 3 +- crates/re_viewport/src/space_view.rs | 22 ++++++++- .../re_viewport/src/space_view_heuristics.rs | 49 +++++++++++-------- 4 files changed, 67 insertions(+), 25 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 143139e4de89..e4fc06361dfc 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/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); diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 2371a7e8fa7f..34bf83021913 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/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); } diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 8b73b93d23cf..3872813fd11d 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/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); diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 79adb089dcab..c7b02a2282ed 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/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)]