Fix bug in origin selection ui (#6199)

### What
Fixes a cause of non-determinism.


I also opened an issue:
* #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 <[email protected]>

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/)).
 

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)
 }

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<EntityPath> = 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::<RecommendedSpaceView>::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<EntityPath> = 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::<RecommendedSpaceView>::new();
+
+                recommended_space_views_with_image_splits(
+                    ctx,
+                    &image_dimensions,
+                    &recommended_root,
+                    &relevant_entities,
+                    &mut recommended_space_views,
+                );
+
+                recommended_space_views
+            }))
         })
         .unwrap_or_default()
     }

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::<Vec<_>>();
-
-                    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::<Vec<_>>();
+
+                        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()
     }

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()
         }
     }
 

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.

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(

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<Item = &SpaceViewClassRegistryEntry> {
-        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.

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<RecommendedSpaceView>,
+    recommended_space_views: Vec<RecommendedSpaceView>,
+}
+
+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<Item = RecommendedSpaceView>) -> Self {
+        let mut recommended_space_views: Vec<RecommendedSpaceView> = 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<RecommendedSpaceView> {
+        self.recommended_space_views
+    }
 }
 
 impl RecommendedSpaceView {

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<ContainerId>,
@@ -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();