Skip to content

Commit

Permalink
Rethink view selection & filtering + make all views opt-in (#3323)
Browse files Browse the repository at this point in the history
This refactors view selection & heuristic filtering based on the lengthy
standup discussions we had regarding the difficulties met during the
implementation of the mesh-related archetypes.

The most important changes are to the `ViewPartSystem` trait which now
looks like this:
```rust
// [...]

    /// Returns the minimal set of components that the system _requires_ in order to be instantiated.
    fn required_components(&self) -> IntSet<ComponentName>;

    /// Implements a filter to heuristically determine whether or not to instantiate the system.
    ///
    /// If and when the system can be instantiated (i.e. because there is at least one entity that satisfies
    /// the minimal set of required components), this method applies an arbitrary filter to determine whether
    /// or not the system should be instantiated by default.
    ///
    /// The passed-in set of `components` corresponds to all the different component that have ever been logged
    /// on the entity path.
    ///
    /// By default, this always returns true.
    /// Override this method only if a more detailed condition is required to inform heuristics whether or not
    /// the given entity is relevant for this system.
    fn heuristic_filter(
        &self,
        _store: &re_arrow_store::DataStore,
        _ent_path: &EntityPath,
        _components: &IntSet<ComponentName>,
    ) -> bool {
        true
    }

// [...]
```
as well as the modification made to [the heuristic filtering
logic](https://github.com/rerun-io/rerun/pull/3323/files#diff-3f3d4453e2c33e0cbe9e4d3be98acbeca9e8ae6425c7cfbc099cd40425e90698).


As a side-effect of these changes, our heuristics now support archetypes
with multiple required components.

---

This also makes all space views opt-in based on indicator components, as
opposed to the status quo where views are opt-out, even though there is
no way to opt-out.

This prevents issues like the following:

![image](https://github.com/rerun-io/rerun/assets/2910679/971a7ded-3777-41ee-8e38-be13a8a7a2b1)
where multiple view systems render the same data because they all can,
even though that wasn't the intention of the logger (which merely logged
a `Mesh3D` archetype in this case).
Same data with this PR:

![image](https://github.com/rerun-io/rerun/assets/2910679/4571c84c-a532-4521-a9c8-6d0759725ddd)

In the future, we'll want to provide ways for users to easily opt-in
into multiple views.
This is already possible today by extending existing archetypes, but
could definitely be more friendly using blueprints.
  • Loading branch information
teh-cmc authored Sep 15, 2023
1 parent 612135c commit 3322abd
Show file tree
Hide file tree
Showing 38 changed files with 451 additions and 256 deletions.
6 changes: 3 additions & 3 deletions crates/re_arrow_store/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use std::sync::atomic::AtomicU64;

use ahash::HashMap;
use arrow2::datatypes::DataType;
use nohash_hasher::{IntMap, IntSet};
use nohash_hasher::IntMap;
use parking_lot::RwLock;
use re_types::ComponentName;
use re_types::{ComponentName, ComponentNameSet};
use smallvec::SmallVec;

use re_log_types::{
Expand Down Expand Up @@ -400,7 +400,7 @@ pub struct IndexedTable {
/// Note that this set will never be purged and will continue to return components that may
/// have been set in the past even if all instances of that component have since been purged
/// to free up space.
pub all_components: IntSet<ComponentName>,
pub all_components: ComponentNameSet,

/// The number of rows stored in this table, across all of its buckets.
pub buckets_num_rows: u64,
Expand Down
5 changes: 2 additions & 3 deletions crates/re_arrow_store/src/store_read.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use std::{ops::RangeBounds, sync::atomic::Ordering};

use itertools::Itertools;
use nohash_hasher::IntSet;
use re_log::trace;
use re_log_types::{DataCell, EntityPath, RowId, TimeInt, TimePoint, TimeRange, Timeline};
use re_types::ComponentName;
use re_types::{ComponentName, ComponentNameSet};
use smallvec::SmallVec;

use crate::{DataStore, IndexedBucket, IndexedBucketInner, IndexedTable, PersistentIndexedTable};
Expand Down Expand Up @@ -107,7 +106,7 @@ impl DataStore {
"query started…"
);

let timeless: Option<IntSet<_>> = self
let timeless: Option<ComponentNameSet> = self
.timeless_tables
.get(&ent_path_hash)
.map(|table| table.columns.keys().cloned().collect());
Expand Down
6 changes: 3 additions & 3 deletions crates/re_arrow_store/src/store_write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use re_log_types::{
DataCell, DataCellColumn, DataCellError, DataRow, DataTable, RowId, SizeBytes as _, TimeInt,
TimePoint, TimeRange,
};
use re_types::{components::InstanceKey, ComponentName, Loggable};
use re_types::{components::InstanceKey, ComponentName, ComponentNameSet, Loggable};

use crate::{
store::MetadataRegistry, DataStore, DataStoreConfig, IndexedBucket, IndexedBucketInner,
Expand Down Expand Up @@ -283,7 +283,7 @@ impl IndexedTable {
) {
re_tracing::profile_function!();

let components: IntSet<_> = row.component_names().collect();
let components: ComponentNameSet = row.component_names().collect();

// borrowck workaround
let timeline = self.timeline;
Expand Down Expand Up @@ -427,7 +427,7 @@ impl IndexedBucket {
time: TimeInt,
generated_cluster_cell: Option<DataCell>,
row: &DataRow,
components: &IntSet<ComponentName>,
components: &ComponentNameSet,
) -> u64 {
re_tracing::profile_function!();

Expand Down
26 changes: 16 additions & 10 deletions crates/re_space_view_bar_chart/src/view_part_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,10 @@ use std::collections::BTreeMap;
use re_arrow_store::LatestAtQuery;
use re_data_store::EntityPath;
use re_log_types::{TimeInt, Timeline};
use re_types::{
archetypes::Tensor, datatypes::TensorData, Archetype, ComponentName, Loggable as _,
};
use re_types::{archetypes::Tensor, datatypes::TensorData, Archetype, ComponentNameSet};
use re_viewer_context::{
ArchetypeDefinition, NamedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, NamedViewSystem, SpaceViewSystemExecutionError,
ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
};

/// A bar chart system, with everything needed to render it.
Expand All @@ -24,17 +22,25 @@ impl NamedViewSystem for BarChartViewPartSystem {
}

impl ViewPartSystem for BarChartViewPartSystem {
fn archetype(&self) -> ArchetypeDefinition {
Tensor::all_components().try_into().unwrap()
fn required_components(&self) -> ComponentNameSet {
// TODO(#3327): make barchart an actual archetype
Tensor::required_components()
.iter()
.map(ToOwned::to_owned)
.collect()
}

fn indicator_components(&self) -> ComponentNameSet {
std::iter::once(Tensor::indicator_component()).collect()
}

fn queries_any_components_of(
fn heuristic_filter(
&self,
store: &re_arrow_store::DataStore,
ent_path: &EntityPath,
components: &[ComponentName],
entity_components: &ComponentNameSet,
) -> bool {
if !components.contains(&re_types::components::TensorData::name()) {
if !default_heuristic_filter(entity_components, &self.indicator_components()) {
return false;
}

Expand Down
15 changes: 9 additions & 6 deletions crates/re_space_view_spatial/src/contexts/annotation_context.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use re_types::{components::AnnotationContext, Loggable};
use re_viewer_context::{
AnnotationMap, ArchetypeDefinition, NamedViewSystem, ViewContextSystem, ViewSystemName,
};
use re_types::{archetypes::AnnotationContext, Archetype, ComponentNameSet};
use re_viewer_context::{AnnotationMap, NamedViewSystem, ViewContextSystem, ViewSystemName};

#[derive(Default)]
pub struct AnnotationSceneContext(pub AnnotationMap);
Expand All @@ -13,8 +11,13 @@ impl NamedViewSystem for AnnotationSceneContext {
}

impl ViewContextSystem for AnnotationSceneContext {
fn archetypes(&self) -> Vec<ArchetypeDefinition> {
vec![vec1::vec1![AnnotationContext::name()]]
fn compatible_component_sets(&self) -> Vec<ComponentNameSet> {
vec![
AnnotationContext::required_components()
.iter()
.map(ToOwned::to_owned)
.collect(), //
]
}

fn execute(
Expand Down
8 changes: 4 additions & 4 deletions crates/re_space_view_spatial/src/contexts/depth_offsets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use std::collections::{BTreeMap, BTreeSet};
use nohash_hasher::IntMap;

use re_log_types::EntityPathHash;
use re_types::{components::DrawOrder, Loggable as _};
use re_viewer_context::{ArchetypeDefinition, NamedViewSystem, ViewContextSystem};
use re_types::{components::DrawOrder, ComponentNameSet, Loggable as _};
use re_viewer_context::{NamedViewSystem, ViewContextSystem};

/// Context for creating a mapping from [`DrawOrder`] to [`re_renderer::DepthOffset`].
#[derive(Default)]
Expand All @@ -26,8 +26,8 @@ impl NamedViewSystem for EntityDepthOffsets {
}

impl ViewContextSystem for EntityDepthOffsets {
fn archetypes(&self) -> Vec<ArchetypeDefinition> {
vec![vec1::vec1![DrawOrder::name()]]
fn compatible_component_sets(&self) -> Vec<ComponentNameSet> {
vec![std::iter::once(DrawOrder::name()).collect()]
}

fn execute(
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/contexts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl NamedViewSystem for PrimitiveCounter {
}

impl ViewContextSystem for PrimitiveCounter {
fn archetypes(&self) -> Vec<re_viewer_context::ArchetypeDefinition> {
fn compatible_component_sets(&self) -> Vec<re_types::ComponentNameSet> {
Vec::new()
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use nohash_hasher::IntSet;
use re_log_types::EntityPathHash;
use re_types::ComponentNameSet;
use re_viewer_context::{NamedViewSystem, ViewContextSystem};

/// List of all non-interactive entities for lookup during picking evaluation.
Expand All @@ -15,7 +16,7 @@ impl NamedViewSystem for NonInteractiveEntities {
}

impl ViewContextSystem for NonInteractiveEntities {
fn archetypes(&self) -> Vec<re_viewer_context::ArchetypeDefinition> {
fn compatible_component_sets(&self) -> Vec<ComponentNameSet> {
Vec::new()
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use parking_lot::{MappedMutexGuard, Mutex, MutexGuard};
use re_renderer::{LineStripSeriesBuilder, PointCloudBuilder, RenderContext};
use re_viewer_context::{ArchetypeDefinition, NamedViewSystem, ViewContextSystem};
use re_types::ComponentNameSet;
use re_viewer_context::{NamedViewSystem, ViewContextSystem};

use crate::parts::{
SIZE_BOOST_IN_POINTS_FOR_LINE_OUTLINES, SIZE_BOOST_IN_POINTS_FOR_POINT_OUTLINES,
Expand Down Expand Up @@ -63,7 +64,7 @@ impl SharedRenderBuilders {
}

impl ViewContextSystem for SharedRenderBuilders {
fn archetypes(&self) -> Vec<ArchetypeDefinition> {
fn compatible_component_sets(&self) -> Vec<ComponentNameSet> {
Vec::new()
}

Expand Down
12 changes: 6 additions & 6 deletions crates/re_space_view_spatial/src/contexts/transform_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ use re_data_store::{EntityPath, EntityPropertyMap, EntityTree};
use re_space_view::UnreachableTransformReason;
use re_types::{
components::{DisconnectedSpace, Transform3D},
Loggable as _,
ComponentNameSet, Loggable as _,
};
use re_viewer_context::{ArchetypeDefinition, NamedViewSystem, ViewContextSystem};
use re_viewer_context::{NamedViewSystem, ViewContextSystem};

use crate::parts::image_view_coordinates;

Expand Down Expand Up @@ -64,11 +64,11 @@ impl Default for TransformContext {
}

impl ViewContextSystem for TransformContext {
fn archetypes(&self) -> Vec<ArchetypeDefinition> {
fn compatible_component_sets(&self) -> Vec<ComponentNameSet> {
vec![
vec1::vec1![Transform3D::name()],
vec1::vec1![Pinhole::name()],
vec1::vec1![DisconnectedSpace::name()],
std::iter::once(Transform3D::name()).collect(),
std::iter::once(Pinhole::name()).collect(),
std::iter::once(DisconnectedSpace::name()).collect(),
]
}

Expand Down
17 changes: 12 additions & 5 deletions crates/re_space_view_spatial/src/parts/arrows3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ use re_renderer::renderer::LineStripFlags;
use re_types::{
archetypes::Arrows3D,
components::{Origin3D, Text, Vector3D},
Archetype as _,
Archetype as _, ComponentNameSet,
};
use re_viewer_context::{
ArchetypeDefinition, NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError,
ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
};

use super::{picking_id_from_instance_key, process_annotations, SpatialViewPartData};
Expand Down Expand Up @@ -169,8 +169,15 @@ impl NamedViewSystem for Arrows3DPart {
}

impl ViewPartSystem for Arrows3DPart {
fn archetype(&self) -> ArchetypeDefinition {
Arrows3D::all_components().try_into().unwrap()
fn required_components(&self) -> ComponentNameSet {
Arrows3D::required_components()
.iter()
.map(ToOwned::to_owned)
.collect()
}

fn indicator_components(&self) -> ComponentNameSet {
std::iter::once(Arrows3D::indicator_component()).collect()
}

fn execute(
Expand Down
17 changes: 12 additions & 5 deletions crates/re_space_view_spatial/src/parts/boxes2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ use re_query::{ArchetypeView, QueryError};
use re_types::{
archetypes::Boxes2D,
components::{HalfSizes2D, Position2D},
Archetype,
Archetype, ComponentNameSet,
};
use re_viewer_context::{
ArchetypeDefinition, NamedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
NamedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem,
ViewQuery, ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -117,8 +117,15 @@ impl NamedViewSystem for Boxes2DPart {
}

impl ViewPartSystem for Boxes2DPart {
fn archetype(&self) -> ArchetypeDefinition {
Boxes2D::all_components().try_into().unwrap()
fn required_components(&self) -> ComponentNameSet {
Boxes2D::required_components()
.iter()
.map(ToOwned::to_owned)
.collect()
}

fn indicator_components(&self) -> ComponentNameSet {
std::iter::once(Boxes2D::indicator_component()).collect()
}

fn execute(
Expand Down
45 changes: 31 additions & 14 deletions crates/re_space_view_spatial/src/parts/boxes3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ use re_query::{EntityView, QueryError};
use re_renderer::Size;
use re_types::{
components::{ClassId, Color, InstanceKey, Radius, Text},
Loggable as _,
ComponentNameSet, Loggable as _,
};
use re_viewer_context::{
ArchetypeDefinition, DefaultColor, NamedViewSystem, SpaceViewSystemExecutionError,
ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
DefaultColor, NamedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -110,8 +110,32 @@ impl NamedViewSystem for Boxes3DPart {
}

impl ViewPartSystem for Boxes3DPart {
fn archetype(&self) -> ArchetypeDefinition {
vec1::vec1![
fn required_components(&self) -> ComponentNameSet {
std::iter::once(Box3D::name()).collect()
}

// TODO(#2786): use this instead
// fn required_components(&self) -> ComponentNameSet {
// Box3D::required_components().to_vec()
// }

// TODO(#2786): use this instead
// fn heuristic_filter(
// &self,
// _store: &re_arrow_store::DataStore,
// _ent_path: &EntityPath,
// components: &[re_types::ComponentName],
// ) -> bool {
// components.contains(&Box3D::indicator_component())
// }

fn execute(
&mut self,
ctx: &mut ViewerContext<'_>,
query: &ViewQuery<'_>,
view_ctx: &ViewContextCollection,
) -> Result<Vec<re_renderer::QueueableDrawData>, SpaceViewSystemExecutionError> {
let components = [
Box3D::name(),
InstanceKey::name(),
LegacyVec3D::name(), // obb.position
Expand All @@ -120,21 +144,14 @@ impl ViewPartSystem for Boxes3DPart {
Radius::name(), // stroke_width
Text::name(),
ClassId::name(),
]
}
];

fn execute(
&mut self,
ctx: &mut ViewerContext<'_>,
query: &ViewQuery<'_>,
view_ctx: &ViewContextCollection,
) -> Result<Vec<re_renderer::QueueableDrawData>, SpaceViewSystemExecutionError> {
process_entity_views::<Boxes3DPart, Box3D, 8, _>(
ctx,
query,
view_ctx,
0,
self.archetype(),
components.into_iter().collect(),
|_ctx, ent_path, entity_view, ent_context| {
self.process_entity_view(query, &entity_view, ent_path, ent_context)
},
Expand Down
Loading

0 comments on commit 3322abd

Please sign in to comment.