Separation & improved book-keeping of entity -> visualizer assignment (…

…#4612)

### What

* Part of:
   * #4377 
   * #4388

Previously, we assumed that the `heuristic_filter` on applicable
entities (== entities that fullfill all required components and some
additional global properties as determined by store subscriber) applies
on a per space view _class_ basis.
However, this is not entirely accurate and misses that "visualizability"
is determined on a per-space-view-instance basis: The most prominent
example of this is that a 2D primitive may or may not be visualizable in
a 3D view depending on where the 3D origin is.
Another, more forward looking problem was had in this area is that
`heuristic_filter` of visualizers should be applied as part of the
_query_ since a query determines whether visualizers are picked
automaticall/all/single.

This PR separates all these concerns more clearly and introduces types
wrapping lists of entities to make it harder to confuse which set
represents what:

* `ApplicableEntities`
   * global set, already produced by a store subscriber
* `VisualizableEntities`
* produced per space view instance. Space view instances can compute a
context (replaces `HeuristicContext`!) to guide their systems. This is
an `Any` which allows visualizer systems to react to different "parent"
space views!
* `IndicatorMatchingEntities`
   * global set, already produced by a store subscriber
* to be used as base for the "ActiveEntities" for each visualizer. For
simplicity the "ActiveEntities" itself is right now not a type. Instead,
the query uses `IndicatorMatchingEntities`and passed in
`VisualizableEntities` on the fly to determine this final set. In the
**future** it:
* may opt out to do so because a query warrants a specific visualizer(s)
* may opt out to do so because a query warrants all possible visualizers
       * visualizers and/or classes may modify the heuristic set


Great care was taken to not regress performance of the (still per frame
applied) heuristics. However, since we're doing a lot more work now
there's still a bit of a performance hit:

Before:

![image](https://github.com/rerun-io/rerun/assets/1220815/e6fe0acc-535b-40c4-be7a-e5e66a752834)

After:

![image](https://github.com/rerun-io/rerun/assets/1220815/14d91e6f-2f9c-4de0-92af-03ea4fff0b74)

I decided that any further improvements in this area should be done in
the context of not recomputing heuristics every frame and instead react
to relevant changes. (#4388)


### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4612/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4612/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4612/index.html?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

- [PR Build Summary](https://build.rerun.io/pr/4612)
- [Docs
preview](https://rerun.io/preview/7b0485ba1d74511929facf0c06a42f6c95eb5881/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/7b0485ba1d74511929facf0c06a42f6c95eb5881/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Jeremy Leibs <jleibs@users.noreply.github.com>

.vscode/settings.json

@@ -37,6 +37,8 @@
         "Tonemapper",
         "tonemapping",
         "unsmoothed",
+        "visualizable",
+        "visualizability",
         "voronoi",
         "vram",
         "Wgsl"
@@ -50,7 +52,7 @@
         "--workspace",
         "--message-format=json",
         "--all-targets",
-        "--all-features", // --all-features will set the `__ci` feature flag, which stops crates/re_web_viewer_server/build.rs from building the web viewer
+        "--all-features" // --all-features will set the `__ci` feature flag, which stops crates/re_web_viewer_server/build.rs from building the web viewer
     ],
     "rust-analyzer.cargo.buildScripts.overrideCommand": [
         "cargo",
@@ -83,12 +85,10 @@
     "ruff.lint.args": [
         "--config=rerun_py/pyproject.toml"
     ],
-
     "prettier.configPath": ".prettierrc.toml",
     "[python]": {
-        "editor.defaultFormatter": "charliermarsh.ruff",
+        "editor.defaultFormatter": "charliermarsh.ruff"
     },
-
     "[javascript]": {
         "editor.defaultFormatter": "esbenp.prettier-vscode"
     },

crates/re_space_view/src/data_query.rs

@@ -1,5 +1,7 @@
 use re_data_store::{EntityProperties, EntityPropertyMap};
-use re_viewer_context::{DataQueryResult, EntitiesPerSystem, StoreContext};
+use re_viewer_context::{
+    DataQueryResult, IndicatorMatchingEntities, PerVisualizer, StoreContext, VisualizableEntities,
+};
 
 pub struct EntityOverrideContext {
     pub root: EntityProperties,
@@ -28,6 +30,7 @@ pub trait DataQuery {
     fn execute_query(
         &self,
         ctx: &StoreContext<'_>,
-        entities_per_system: &EntitiesPerSystem,
+        visualizable_entities_for_visualizer_systems: &PerVisualizer<VisualizableEntities>,
+        indicator_matching_entities_per_visualizer: &PerVisualizer<IndicatorMatchingEntities>,
     ) -> DataQueryResult;
 }

crates/re_space_view/src/data_query_blueprint.rs

@@ -1,3 +1,4 @@
+use nohash_hasher::IntMap;
 use slotmap::SlotMap;
 use smallvec::SmallVec;
 
@@ -10,8 +11,9 @@ use re_log_types::{
 use re_types_core::archetypes::Clear;
 use re_viewer_context::{
     DataQueryId, DataQueryResult, DataResult, DataResultHandle, DataResultNode, DataResultTree,
-    EntitiesPerSystem, PropertyOverrides, SpaceViewClassIdentifier, SpaceViewId, StoreContext,
-    SystemCommand, SystemCommandSender as _, ViewerContext,
+    IndicatorMatchingEntities, PerVisualizer, PropertyOverrides, SpaceViewClassIdentifier,
+    SpaceViewId, StoreContext, SystemCommand, SystemCommandSender as _, ViewSystemIdentifier,
+    ViewerContext, VisualizableEntities,
 };
 
 use crate::{
@@ -176,13 +178,18 @@ impl DataQuery for DataQueryBlueprint {
     fn execute_query(
         &self,
         ctx: &re_viewer_context::StoreContext<'_>,
-        entities_per_system: &EntitiesPerSystem,
+        visualizable_entities_for_visualizer_systems: &PerVisualizer<VisualizableEntities>,
+        indicator_matching_entities_per_visualizer: &PerVisualizer<IndicatorMatchingEntities>,
     ) -> DataQueryResult {
         re_tracing::profile_function!();
 
         let mut data_results = SlotMap::<DataResultHandle, DataResultNode>::default();
 
-        let executor = QueryExpressionEvaluator::new(self, entities_per_system);
+        let executor = QueryExpressionEvaluator::new(
+            self,
+            visualizable_entities_for_visualizer_systems,
+            indicator_matching_entities_per_visualizer,
+        );
 
         let root_handle = ctx.recording.and_then(|store| {
             re_tracing::profile_scope!("add_entity_tree_to_data_results_recursive");
@@ -202,19 +209,26 @@ impl DataQuery for DataQueryBlueprint {
 /// used to efficiently determine if we should continue the walk or switch
 /// to a pure recursive evaluation.
 struct QueryExpressionEvaluator<'a> {
-    per_system_entity_list: &'a EntitiesPerSystem,
+    visualizable_entities_for_visualizer_systems: &'a PerVisualizer<VisualizableEntities>,
+    indicator_matching_entities_per_visualizer:
+        &'a IntMap<ViewSystemIdentifier, IndicatorMatchingEntities>,
     entity_path_filter: EntityPathFilter,
 }
 
 impl<'a> QueryExpressionEvaluator<'a> {
     fn new(
         blueprint: &'a DataQueryBlueprint,
-        per_system_entity_list: &'a EntitiesPerSystem,
+        visualizable_entities_for_visualizer_systems: &'a PerVisualizer<VisualizableEntities>,
+        indicator_matching_entities_per_visualizer: &'a IntMap<
+            ViewSystemIdentifier,
+            IndicatorMatchingEntities,
+        >,
     ) -> Self {
         re_tracing::profile_function!();
 
         Self {
-            per_system_entity_list,
+            visualizable_entities_for_visualizer_systems,
+            indicator_matching_entities_per_visualizer,
             entity_path_filter: blueprint.entity_path_filter.clone(),
         }
     }
@@ -242,11 +256,30 @@ impl<'a> QueryExpressionEvaluator<'a> {
         // Only populate view_parts if this is a match
         // Note that allowed prefixes that aren't matches can still create groups
         let view_parts: SmallVec<_> = if any_match {
-            self.per_system_entity_list
+            self.visualizable_entities_for_visualizer_systems
                 .iter()
-                .filter_map(|(part, ents)| {
+                .filter_map(|(visualizer, ents)| {
                     if ents.contains(entity_path) {
-                        Some(*part)
+                        // TODO(andreas):
+                        // * not all queries do just heuristic filtering of visualizers,
+                        //   some set the visualizer upfront, others should skip this check and visualize all
+                        // * Space view classes should be able to modify this check.
+                        //   As of writing this hasn't been done yet in order to simplify things
+                        // * querying the per-visualizer lists every time is silly
+                        //   -> at beginning of query squash all visualizers in `visualizable_entities_for_visualizer_systems`
+                        //      to a single `IntSet<EntityPathHash>`
+                        //   -> consider three steps of query: list entities, list their visualizers, list their properties
+                        if self
+                            .indicator_matching_entities_per_visualizer
+                            .get(visualizer)
+                            .map_or(false, |matching_list| {
+                                matching_list.contains(&entity_path.hash())
+                            })
+                        {
+                            Some(*visualizer)
+                        } else {
+                            None
+                        }
                     } else {
                         None
                     }
@@ -460,7 +493,7 @@ impl<'a> PropertyResolver for DataQueryPropertyResolver<'a> {
 mod tests {
     use re_data_store::StoreDb;
     use re_log_types::{example_components::MyPoint, DataRow, RowId, StoreId, TimePoint, Timeline};
-    use re_viewer_context::StoreContext;
+    use re_viewer_context::{StoreContext, VisualizableEntities};
 
     use super::*;
 
@@ -487,17 +520,21 @@ mod tests {
             recording.add_data_row(row).unwrap();
         }
 
-        let mut entities_per_system = EntitiesPerSystem::default();
+        let mut visualizable_entities_for_visualizer_systems =
+            PerVisualizer::<VisualizableEntities>::default();
 
-        entities_per_system
+        visualizable_entities_for_visualizer_systems
+            .0
             .entry("Points3D".into())
             .or_insert_with(|| {
-                [
-                    EntityPath::from("parent"),
-                    EntityPath::from("parent/skipped/child1"),
-                ]
-                .into_iter()
-                .collect()
+                VisualizableEntities(
+                    [
+                        EntityPath::from("parent"),
+                        EntityPath::from("parent/skipped/child1"),
+                    ]
+                    .into_iter()
+                    .collect(),
+                )
             });
 
         let ctx = StoreContext {
@@ -603,7 +640,25 @@ mod tests {
                 entity_path_filter: EntityPathFilter::parse_forgiving(filter),
             };
 
-            let query_result = query.execute_query(&ctx, &entities_per_system);
+            let indicator_matching_entities_per_visualizer =
+                PerVisualizer::<IndicatorMatchingEntities>(
+                    visualizable_entities_for_visualizer_systems
+                        .iter()
+                        .map(|(id, entities)| {
+                            (
+                                *id,
+                                IndicatorMatchingEntities(
+                                    entities.0.iter().map(|path| path.hash()).collect(),
+                                ),
+                            )
+                        })
+                        .collect(),
+                );
+            let query_result = query.execute_query(
+                &ctx,
+                &visualizable_entities_for_visualizer_systems,
+                &indicator_matching_entities_per_visualizer,
+            );
 
             let mut visited = vec![];
             query_result.tree.visit(&mut |handle| {

crates/re_space_view_spatial/src/contexts/transform_context.rs

@@ -130,7 +130,7 @@ impl ViewContextSystem for TransformContext {
         let mut encountered_pinhole = None;
         let mut reference_from_ancestor = glam::Affine3A::IDENTITY;
         while let Some(parent_path) = current_tree.path.parent() {
-            let Some(parent_tree) = &entity_tree.subtree(&parent_path) else {
+            let Some(parent_tree) = entity_tree.subtree(&parent_path) else {
                 // Unlike not having the space path in the hierarchy, this should be impossible.
                 re_log::error_once!(
                     "Path {} is not part of the global Entity tree whereas its child {} is",

crates/re_space_view_spatial/src/parts/boxes2d.rs

@@ -1,15 +1,14 @@
-use nohash_hasher::IntSet;
 use re_data_store::{EntityPath, InstancePathHash};
-use re_log_types::EntityPathHash;
 use re_query::{ArchetypeView, QueryError};
 use re_types::{
     archetypes::Boxes2D,
     components::{HalfSizes2D, Position2D, Text},
     Archetype, ComponentNameSet,
 };
 use re_viewer_context::{
-    HeuristicFilterContext, IdentifiedViewSystem, ResolvedAnnotationInfos,
+    ApplicableEntities, IdentifiedViewSystem, ResolvedAnnotationInfos,
     SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
+    VisualizableEntities,
 };
 
 use crate::{
@@ -19,8 +18,9 @@ use crate::{
 };
 
 use super::{
-    entity_iterator::process_archetype_views, picking_id_from_instance_key, process_annotations,
-    process_colors, process_radii, SpatialViewPartData,
+    entity_iterator::process_archetype_views, filter_visualizable_2d_entities,
+    picking_id_from_instance_key, process_annotations, process_colors, process_radii,
+    SpatialViewPartData,
 };
 
 pub struct Boxes2DPart {
@@ -179,25 +179,13 @@ impl ViewPartSystem for Boxes2DPart {
         std::iter::once(Boxes2D::indicator().name()).collect()
     }
 
-    fn heuristic_filter(
+    fn filter_visualizable_entities(
         &self,
-        entities_with_matching_indicator: &IntSet<EntityPathHash>,
-        ent_path: &EntityPath,
-        ctx: HeuristicFilterContext,
-    ) -> bool {
-        if !entities_with_matching_indicator.contains(&ent_path.hash()) {
-            return false;
-        }
-
-        // 2D parts can only ever be rendered properly as part of a 3D scene if the
-        // transform graph includes a pinhole projection. Pinholes only map from 2D children
-        // to 3D parents, so if the required pinhole exists, it must be an ancestor.
-        // Filter them otherwise to avoid ending up with 2D content mixed into 3D scenes.
-        if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
-            return false;
-        }
-
-        true
+        entities: ApplicableEntities,
+        context: &dyn std::any::Any,
+    ) -> VisualizableEntities {
+        re_tracing::profile_function!();
+        filter_visualizable_2d_entities(entities, context)
     }
 
     fn execute(

crates/re_space_view_spatial/src/parts/images.rs

@@ -19,15 +19,15 @@ use re_types::{
     Archetype as _, ComponentNameSet,
 };
 use re_viewer_context::{
-    gpu_bridge, DefaultColor, HeuristicFilterContext, SpaceViewClass,
-    SpaceViewSystemExecutionError, TensorDecodeCache, TensorStatsCache, ViewPartSystem, ViewQuery,
-    ViewerContext, VisualizerAdditionalApplicabilityFilter,
+    gpu_bridge, ApplicableEntities, DefaultColor, IdentifiedViewSystem, SpaceViewClass,
+    SpaceViewSystemExecutionError, TensorDecodeCache, TensorStatsCache, ViewContextCollection,
+    ViewPartSystem, ViewQuery, ViewerContext, VisualizableEntities,
+    VisualizerAdditionalApplicabilityFilter,
 };
-use re_viewer_context::{IdentifiedViewSystem, ViewContextCollection};
 
 use crate::{
     contexts::{EntityDepthOffsets, SpatialSceneEntityContext, TransformContext},
-    parts::SIZE_BOOST_IN_POINTS_FOR_POINT_OUTLINES,
+    parts::{filter_visualizable_2d_entities, SIZE_BOOST_IN_POINTS_FOR_POINT_OUTLINES},
     query_pinhole,
     view_kind::SpatialSpaceViewKind,
     SpatialSpaceView2D, SpatialSpaceView3D,
@@ -691,25 +691,13 @@ impl ViewPartSystem for ImagesPart {
         Some(Box::new(ImageVisualizerEntityFilter))
     }
 
-    fn heuristic_filter(
+    fn filter_visualizable_entities(
         &self,
-        entities_with_matching_indicator: &IntSet<EntityPathHash>,
-        ent_path: &EntityPath,
-        ctx: HeuristicFilterContext,
-    ) -> bool {
-        if !entities_with_matching_indicator.contains(&ent_path.hash()) {
-            return false;
-        }
-
-        // 2D parts can only ever be rendered properly as part of a 3D scene if the
-        // transform graph includes a pinhole projection. Pinholes only map from 2D children
-        // to 3D parents, so if the required pinhole exists, it must be an ancestor.
-        // Filter them otherwise to avoid ending up with 2D content mixed into 3D scenes.
-        if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
-            return false;
-        }
-
-        true
+        entities: ApplicableEntities,
+        context: &dyn std::any::Any,
+    ) -> VisualizableEntities {
+        re_tracing::profile_function!();
+        filter_visualizable_2d_entities(entities, context)
     }
 
     fn execute(