From 8f17737121bafa069583d447efc265ae63acb61f Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 2 May 2024 17:04:33 +0200 Subject: [PATCH] Fix bug in origin selection ui (#6199) ### What Fixes a cause of non-determinism. I also opened an issue: * https://github.com/rerun-io/rerun/issues/6198 ### Before: ![flicker](https://github.com/rerun-io/rerun/assets/1148717/dd6c32b7-77ce-40ff-89d8-279fda3ca74c) ### 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 examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6199?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6199?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/6199) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --------- Co-authored-by: Antoine Beyeler <49431240+abey79@users.noreply.github.com> --- CODE_STYLE.md | 5 + crates/re_space_view/src/heuristics.rs | 7 +- .../src/space_view_2d.rs | 83 +++++++------ .../src/space_view_3d.rs | 112 +++++++++--------- .../src/space_view_class.rs | 10 +- .../src/space_view_class.rs | 18 +-- .../space_view_class_placeholder.rs | 4 +- .../space_view/space_view_class_registry.rs | 8 +- .../src/space_view/spawn_heuristics.rs | 31 ++++- .../src/add_space_view_or_container_modal.rs | 6 +- crates/re_viewport/src/context_menu/mod.rs | 2 - .../re_viewport/src/space_view_heuristics.rs | 2 +- crates/re_viewport/src/viewport_blueprint.rs | 7 +- .../src/color_coordinates_space_view.rs | 8 +- 14 files changed, 157 insertions(+), 146 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 054e65b10512..6239ca4653d6 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -41,6 +41,11 @@ let Some(first) = vec.get(0) else { }; ``` +### Iterators +Be careful when iterating over `HashSet`s and `HashMap`s, as the order is non-deterministic. +Whenever you return a list or an iterator, sort it first. +If you don't want to sort it for performance reasons, you MUST put `unsorted` in the name as a warning. + ### Error handling and logging We log problems using our own `re_log` crate (which is currently a wrapper around [`tracing`](https://crates.io/crates/tracing/)). diff --git a/crates/re_space_view/src/heuristics.rs b/crates/re_space_view/src/heuristics.rs index 386264427582..6968c15cdaa8 100644 --- a/crates/re_space_view/src/heuristics.rs +++ b/crates/re_space_view/src/heuristics.rs @@ -45,10 +45,7 @@ where } else { Some(RecommendedSpaceView::new_single_entity(entity.clone())) } - }) - .collect(); + }); - re_viewer_context::SpaceViewSpawnHeuristics { - recommended_space_views, - } + re_viewer_context::SpaceViewSpawnHeuristics::new(recommended_space_views) } diff --git a/crates/re_space_view_spatial/src/space_view_2d.rs b/crates/re_space_view_spatial/src/space_view_2d.rs index 34424f1a7016..e333c32a9660 100644 --- a/crates/re_space_view_spatial/src/space_view_2d.rs +++ b/crates/re_space_view_spatial/src/space_view_2d.rs @@ -189,49 +189,46 @@ impl SpaceViewClass for SpatialSpaceView2D { // Spawn a space view at each subspace that has any potential 2D content. // Note that visualizability filtering is all about being in the right subspace, // so we don't need to call the visualizers' filter functions here. - SpatialTopology::access(ctx.recording_id(), |topo| SpaceViewSpawnHeuristics { - recommended_space_views: topo - .iter_subspaces() - .flat_map(|subspace| { - if !subspace.supports_2d_content() - || subspace.entities.is_empty() - || indicated_entities.is_disjoint(&subspace.entities) - { - return Vec::new(); - } - - // Collect just the 2D-relevant entities in this subspace - let relevant_entities: IntSet = subspace - .entities - .iter() - .filter(|e| indicated_entities.contains(e)) - .cloned() - .collect(); - - // For explicit 2D spaces with a pinhole at the origin, otherwise start at the common ancestor. - // This generally avoids the `/` root entity unless it's required as a common ancestor. - let recommended_root = if subspace - .connection_to_parent - .contains(SubSpaceConnectionFlags::Pinhole) - { - subspace.origin.clone() - } else { - EntityPath::common_ancestor_of(relevant_entities.iter()) - }; - - let mut recommended_space_views = Vec::::new(); - - recommended_space_views_with_image_splits( - ctx, - &image_dimensions, - &recommended_root, - &relevant_entities, - &mut recommended_space_views, - ); - - recommended_space_views - }) - .collect(), + SpatialTopology::access(ctx.recording_id(), |topo| { + SpaceViewSpawnHeuristics::new(topo.iter_subspaces().flat_map(|subspace| { + if !subspace.supports_2d_content() + || subspace.entities.is_empty() + || indicated_entities.is_disjoint(&subspace.entities) + { + return Vec::new(); + } + + // Collect just the 2D-relevant entities in this subspace + let relevant_entities: IntSet = subspace + .entities + .iter() + .filter(|e| indicated_entities.contains(e)) + .cloned() + .collect(); + + // For explicit 2D spaces with a pinhole at the origin, otherwise start at the common ancestor. + // This generally avoids the `/` root entity unless it's required as a common ancestor. + let recommended_root = if subspace + .connection_to_parent + .contains(SubSpaceConnectionFlags::Pinhole) + { + subspace.origin.clone() + } else { + EntityPath::common_ancestor_of(relevant_entities.iter()) + }; + + let mut recommended_space_views = Vec::::new(); + + recommended_space_views_with_image_splits( + ctx, + &image_dimensions, + &recommended_root, + &relevant_entities, + &mut recommended_space_views, + ); + + recommended_space_views + })) }) .unwrap_or_default() } diff --git a/crates/re_space_view_spatial/src/space_view_3d.rs b/crates/re_space_view_spatial/src/space_view_3d.rs index 61a60a9dd135..09dab4ba5e41 100644 --- a/crates/re_space_view_spatial/src/space_view_3d.rs +++ b/crates/re_space_view_spatial/src/space_view_3d.rs @@ -210,65 +210,65 @@ impl SpaceViewClass for SpatialSpaceView3D { // Spawn a space view at each subspace that has any potential 3D content. // Note that visualizability filtering is all about being in the right subspace, // so we don't need to call the visualizers' filter functions here. - SpatialTopology::access(ctx.recording_id(), |topo| SpaceViewSpawnHeuristics { - recommended_space_views: topo - .iter_subspaces() - .filter_map(|subspace| { - if !subspace.supports_3d_content() { - return None; - } + SpatialTopology::access(ctx.recording_id(), |topo| { + SpaceViewSpawnHeuristics::new( + topo.iter_subspaces() + .filter_map(|subspace| { + if !subspace.supports_3d_content() { + return None; + } - let mut pinhole_child_spaces = subspace - .child_spaces - .iter() - .filter(|child| { - topo.subspace_for_subspace_origin(child.hash()).map_or( - false, - |child_space| { - child_space.connection_to_parent.is_connected_pinhole() - }, - ) - }) - .peekable(); // Don't collect the iterator, we're only interested in 'any'-style operations. - - // Empty space views are still of interest if any of the child spaces is connected via a pinhole. - if subspace.entities.is_empty() && pinhole_child_spaces.peek().is_none() { - return None; - } + let mut pinhole_child_spaces = subspace + .child_spaces + .iter() + .filter(|child| { + topo.subspace_for_subspace_origin(child.hash()).map_or( + false, + |child_space| { + child_space.connection_to_parent.is_connected_pinhole() + }, + ) + }) + .peekable(); // Don't collect the iterator, we're only interested in 'any'-style operations. + + // Empty space views are still of interest if any of the child spaces is connected via a pinhole. + if subspace.entities.is_empty() && pinhole_child_spaces.peek().is_none() { + return None; + } - // Creates space views at each view coordinates if there's any. - // (yes, we do so even if they're empty at the moment!) - // - // An exception to this rule is not to create a view there if this is already _also_ a subspace root. - // (e.g. this also has a camera or a `disconnect` logged there) - let mut origins = subspace - .heuristic_hints - .iter() - .filter(|(path, hint)| { - hint.contains(HeuristicHints::ViewCoordinates3d) - && !subspace.child_spaces.contains(path) - }) - .map(|(path, _)| path.clone()) - .collect::>(); - - let path_not_covered_yet = - |e: &EntityPath| origins.iter().all(|origin| !e.starts_with(origin)); - - // If there's no view coordinates or there are still some entities not covered, - // create a view at the subspace origin. - if !origins.iter().contains(&subspace.origin) - && (indicated_entities - .intersection(&subspace.entities) - .any(path_not_covered_yet) - || pinhole_child_spaces.any(path_not_covered_yet)) - { - origins.push(subspace.origin.clone()); - } + // Creates space views at each view coordinates if there's any. + // (yes, we do so even if they're empty at the moment!) + // + // An exception to this rule is not to create a view there if this is already _also_ a subspace root. + // (e.g. this also has a camera or a `disconnect` logged there) + let mut origins = subspace + .heuristic_hints + .iter() + .filter(|(path, hint)| { + hint.contains(HeuristicHints::ViewCoordinates3d) + && !subspace.child_spaces.contains(path) + }) + .map(|(path, _)| path.clone()) + .collect::>(); + + let path_not_covered_yet = + |e: &EntityPath| origins.iter().all(|origin| !e.starts_with(origin)); + + // If there's no view coordinates or there are still some entities not covered, + // create a view at the subspace origin. + if !origins.iter().contains(&subspace.origin) + && (indicated_entities + .intersection(&subspace.entities) + .any(path_not_covered_yet) + || pinhole_child_spaces.any(path_not_covered_yet)) + { + origins.push(subspace.origin.clone()); + } - Some(origins.into_iter().map(RecommendedSpaceView::new_subtree)) - }) - .flatten() - .collect(), + Some(origins.into_iter().map(RecommendedSpaceView::new_subtree)) + }) + .flatten(), + ) }) .unwrap_or_default() } diff --git a/crates/re_space_view_text_log/src/space_view_class.rs b/crates/re_space_view_text_log/src/space_view_class.rs index af23aa113a2b..6df995289b5c 100644 --- a/crates/re_space_view_text_log/src/space_view_class.rs +++ b/crates/re_space_view_text_log/src/space_view_class.rs @@ -5,9 +5,9 @@ use re_data_ui::item_ui; use re_log_types::{EntityPath, TimePoint, Timeline}; use re_types::{components::TextLogLevel, SpaceViewClassIdentifier}; use re_viewer_context::{ - level_to_rich_text, IdentifiedViewSystem as _, RecommendedSpaceView, SpaceViewClass, - SpaceViewClassRegistryError, SpaceViewId, SpaceViewSpawnHeuristics, SpaceViewState, - SpaceViewStateExt, SpaceViewSystemExecutionError, ViewQuery, ViewerContext, + level_to_rich_text, IdentifiedViewSystem as _, SpaceViewClass, SpaceViewClassRegistryError, + SpaceViewId, SpaceViewSpawnHeuristics, SpaceViewState, SpaceViewStateExt, + SpaceViewSystemExecutionError, ViewQuery, ViewerContext, }; use super::visualizer_system::{Entry, TextLogSystem}; @@ -93,9 +93,7 @@ impl SpaceViewClass for TextSpaceView { { SpaceViewSpawnHeuristics::default() } else { - SpaceViewSpawnHeuristics { - recommended_space_views: vec![RecommendedSpaceView::root()], - } + SpaceViewSpawnHeuristics::root() } } diff --git a/crates/re_space_view_time_series/src/space_view_class.rs b/crates/re_space_view_time_series/src/space_view_class.rs index bf5fe56d4d7e..514ce635ccd2 100644 --- a/crates/re_space_view_time_series/src/space_view_class.rs +++ b/crates/re_space_view_time_series/src/space_view_class.rs @@ -227,9 +227,7 @@ It can greatly improve performance (and readability) in such situations as it pr .iter() .any(|(_, subtree)| indicated_entities.contains(&subtree.path)) { - return SpaceViewSpawnHeuristics { - recommended_space_views: vec![RecommendedSpaceView::root()], - }; + return SpaceViewSpawnHeuristics::root(); } // If there's other entities that have the right indicator & didn't match the above, @@ -240,17 +238,11 @@ It can greatly improve performance (and readability) in such situations as it pr child_of_root_entities.insert(child_of_root); } } - let recommended_space_views = child_of_root_entities - .into_iter() - .map(|path_part| { - let entity = EntityPath::new(vec![path_part.clone()]); - RecommendedSpaceView::new_subtree(entity) - }) - .collect(); - SpaceViewSpawnHeuristics { - recommended_space_views, - } + SpaceViewSpawnHeuristics::new(child_of_root_entities.into_iter().map(|path_part| { + let entity = EntityPath::new(vec![path_part.clone()]); + RecommendedSpaceView::new_subtree(entity) + })) } /// Choose the default visualizers to enable for this entity. diff --git a/crates/re_viewer_context/src/space_view/space_view_class_placeholder.rs b/crates/re_viewer_context/src/space_view/space_view_class_placeholder.rs index 550a8a507fee..b68a7766003c 100644 --- a/crates/re_viewer_context/src/space_view/space_view_class_placeholder.rs +++ b/crates/re_viewer_context/src/space_view/space_view_class_placeholder.rs @@ -43,9 +43,7 @@ impl SpaceViewClass for SpaceViewClassPlaceholder { } fn spawn_heuristics(&self, _ctx: &ViewerContext<'_>) -> SpaceViewSpawnHeuristics { - SpaceViewSpawnHeuristics { - recommended_space_views: Vec::new(), - } + SpaceViewSpawnHeuristics::empty() } fn ui( diff --git a/crates/re_viewer_context/src/space_view/space_view_class_registry.rs b/crates/re_viewer_context/src/space_view/space_view_class_registry.rs index 6d00ba61ee9a..e34b32861658 100644 --- a/crates/re_viewer_context/src/space_view/space_view_class_registry.rs +++ b/crates/re_viewer_context/src/space_view/space_view_class_registry.rs @@ -1,4 +1,6 @@ use ahash::{HashMap, HashSet}; +use itertools::Itertools as _; + use re_data_store::DataStore; use re_types::SpaceViewClassIdentifier; @@ -273,9 +275,11 @@ impl SpaceViewClassRegistry { } } - /// Iterates over all registered Space View class types. + /// Iterates over all registered Space View class types, sorted by name. pub fn iter_registry(&self) -> impl Iterator { - self.space_view_classes.values() + self.space_view_classes + .values() + .sorted_by_key(|entry| entry.class.display_name()) } /// For each visualizer, return the set of entities that is applicable to it. diff --git a/crates/re_viewer_context/src/space_view/spawn_heuristics.rs b/crates/re_viewer_context/src/space_view/spawn_heuristics.rs index edaf7bbec7fd..d20035b10490 100644 --- a/crates/re_viewer_context/src/space_view/spawn_heuristics.rs +++ b/crates/re_viewer_context/src/space_view/spawn_heuristics.rs @@ -16,7 +16,36 @@ pub struct RecommendedSpaceView { #[derive(Default)] pub struct SpaceViewSpawnHeuristics { /// The recommended space views to spawn - pub recommended_space_views: Vec, + recommended_space_views: Vec, +} + +impl SpaceViewSpawnHeuristics { + #[inline] + pub fn empty() -> Self { + Self { + recommended_space_views: Vec::new(), + } + } + + #[inline] + pub fn root() -> Self { + Self { + recommended_space_views: vec![RecommendedSpaceView::root()], + } + } + + pub fn new(iter: impl IntoIterator) -> Self { + let mut recommended_space_views: Vec = iter.into_iter().collect(); + recommended_space_views.sort_by(|a, b| a.origin.cmp(&b.origin)); + Self { + recommended_space_views, + } + } + + #[inline] + pub fn into_vec(self) -> Vec { + self.recommended_space_views + } } impl RecommendedSpaceView { diff --git a/crates/re_viewport/src/add_space_view_or_container_modal.rs b/crates/re_viewport/src/add_space_view_or_container_modal.rs index b1c6e00f73fd..259733523d8f 100644 --- a/crates/re_viewport/src/add_space_view_or_container_modal.rs +++ b/crates/re_viewport/src/add_space_view_or_container_modal.rs @@ -1,14 +1,13 @@ //! Modal for adding a new space view of container to an existing target container. -use itertools::Itertools; - -use crate::{icon_for_container_kind, ViewportBlueprint}; use re_space_view::SpaceViewBlueprint; use re_ui::ReUi; use re_viewer_context::{ blueprint_id_to_tile_id, ContainerId, RecommendedSpaceView, ViewerContext, }; +use crate::{icon_for_container_kind, ViewportBlueprint}; + #[derive(Default)] pub struct AddSpaceViewOrContainerModal { target_container: Option, @@ -111,7 +110,6 @@ fn modal_ui( for space_view in ctx .space_view_class_registry .iter_registry() - .sorted_by_key(|entry| entry.class.display_name()) .map(|entry| SpaceViewBlueprint::new(entry.identifier, RecommendedSpaceView::root())) { let icon = space_view.class(ctx.space_view_class_registry).icon(); diff --git a/crates/re_viewport/src/context_menu/mod.rs b/crates/re_viewport/src/context_menu/mod.rs index fdeaadf334a2..f06f52df8921 100644 --- a/crates/re_viewport/src/context_menu/mod.rs +++ b/crates/re_viewport/src/context_menu/mod.rs @@ -1,4 +1,3 @@ -use itertools::Itertools; use once_cell::sync::OnceCell; use re_entity_db::InstancePath; @@ -129,7 +128,6 @@ fn action_list( actions: ctx .space_view_class_registry .iter_registry() - .sorted_by_key(|entry| entry.class.display_name()) .map(|entry| { Box::new(AddSpaceViewAction(entry.identifier)) as Box diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index c6f74bf3c0a6..15c79faf7c6d 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -14,7 +14,7 @@ pub fn default_created_space_views(ctx: &ViewerContext<'_>) -> Vec