Rethink view selection & filtering + make all views opt-in by teh-cmc · Pull Request #3323 · rerun-io/rerun

teh-cmc · 2023-09-14T09:55:36Z

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:

// [...]

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

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:

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:

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.

Checklist

I have read and agree to Contributor Guide and the Code of Conduct
I've included a screenshot or gif (if applicable)
I have tested demo.rerun.io (if applicable)

teh-cmc · 2023-09-14T12:56:26Z

Drafting: had some looong discussions about this and more with @jleibs and @Wumpf, this generally the direction we want to go int right now but we need to tweak some more things in those traits first.

…uristic_filter

crates/re_viewport/src/space_view_heuristics.rs

jleibs

Yes! This is so much more clear.

crates/re_space_view_bar_chart/src/view_part_system.rs

crates/re_space_view_spatial/src/contexts/annotation_context.rs

crates/re_viewer_context/src/space_view/view_context_system.rs

jleibs · 2023-09-14T16:28:22Z

crates/re_viewer_context/src/space_view/view_part_system.rs

-    /// The archetype queried by this scene element.
-    fn archetype(&self) -> ArchetypeDefinition;
+    /// Returns the minimal set of components that the system _requires_ in order to be instantiated.
+    fn required_components(&self) -> IntSet<ComponentName>;


Is there any scenario where this needs the flexibillity from ViewContextSystem of returning Vec<IntSet<ComponentName>> instead?

The situation of our combined (Image, SegmentationImage, DepthImage) System is maybe one example?

Yep, it's the only example I stumbled onto iirc.

EDIT: Oh no actually the transform one is the one I had in mind:

vec![ std::iter::once(Transform3D::name()).collect(), std::iter::once(Pinhole::name()).collect(), std::iter::once(DisconnectedSpace::name()).collect(), ]

jleibs · 2023-09-14T16:35:54Z

examples/rust/custom_space_view/src/color_coordinates_view_part_system.rs

-    fn archetype(&self) -> ArchetypeDefinition {
-        ColorArchetype::all_components().try_into().unwrap()
+    fn required_components(&self) -> IntSet<ComponentName> {
+        ColorArchetype::required_components()


Should required_components just return an IntSet natively?

I want to keep these core methods allocation-free but... maybe we can generate a static intset in addition to the static array... hmm, I'll have a look.

Eeeeh no actually there's still all the mesh stuff to ship so I'll just leave a comment for now, but yeah something to keep in mind

crates/re_space_view_spatial/src/parts/images.rs

jleibs

Meant to approve on the last review.

Wumpf · 2023-09-14T17:23:30Z

for the record, I skimmed it and agree: great step forward!

crates/re_viewport/src/space_view_heuristics.rs

emilk · 2023-09-15T06:56:53Z

crates/re_space_view_spatial/src/contexts/transform_context.rs

+    fn all_required_components(&self) -> Vec<IntSet<ComponentName>> {
        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(),


These are not all required - only the first one is.

These are different sets -- any one of them is sufficient to trigger the system

crates/re_space_view_bar_chart/src/view_part_system.rs

crates/re_viewer_context/src/space_view/view_part_system.rs

teh-cmc · 2023-09-15T09:47:58Z

This has surfaced a completely unrelated dormant bug with the way the Box2D view handles annotation labels.
It drove me completely nuts until @Wumpf explained to me that this wasn't in fact something introduced by this PR.

I'll open a new PR for said bug, this has already scope creeped way too much.

emilk

LGTM after a quick look, except I don't understand compatible_component_sets from its docstrng

emilk · 2023-09-15T09:46:21Z

crates/re_space_view_spatial/src/parts/points2d.rs

+            .iter()
+            .map(ToOwned::to_owned)
+            .collect()


.iter() .map(ToOwned::to_owned) .collect()

is starting to get tedious

crates/re_viewer_context/src/space_view/view_context_system.rs

What the title says. Requires #3323, which made the underlying bug visible to begin with. ### What ### 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 [demo.rerun.io](https://demo.rerun.io/pr/3331) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3331) - [Docs preview](https://rerun.io/preview/85a4a018bb55e34053f0fcbe475a719e7a0207a4/docs)  - [Examples preview](https://rerun.io/preview/85a4a018bb55e34053f0fcbe475a719e7a0207a4/examples)  - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/)

teh-cmc added 📺 re_viewer affects re_viewer itself 🍏 primitives Relating to Rerun primitives labels Sep 14, 2023

teh-cmc marked this pull request as ready for review September 14, 2023 10:04

teh-cmc marked this pull request as draft September 14, 2023 12:56

teh-cmc changed the title ~~Make all views and view parts opt-in by checking for indicator components~~ Rethink view selection & filtering + make all views opt-in Sep 14, 2023

teh-cmc added 7 commits September 14, 2023 18:08

make all views opt-in by checking for indicator components

82ae272

rethink ViewPartSytem::archetype as ViewPartSytem::required_components

e0eb83b

rethink ViewPartSytem::queries_any_components_of as ViewPartSytem::he…

e09b755

…uristic_filter

lints

929f141

lints

5fa15d5

fix legacy meshes

c46e9e3

fix legacy 3d boxes

2aa04d8

teh-cmc force-pushed the cmc/view_part_filters branch from f6b2448 to 2aa04d8 Compare September 14, 2023 16:08

teh-cmc marked this pull request as ready for review September 14, 2023 16:09

outdated comments

78425c7

teh-cmc commented Sep 14, 2023

View reviewed changes

crates/re_viewport/src/space_view_heuristics.rs Outdated Show resolved Hide resolved

jleibs reviewed Sep 14, 2023

View reviewed changes

jleibs approved these changes Sep 14, 2023

View reviewed changes

Wumpf mentioned this pull request Sep 14, 2023

BarChart should have its own archetype #3327

Closed

emilk requested changes Sep 15, 2023

View reviewed changes

crates/re_viewport/src/space_view_heuristics.rs Outdated Show resolved Hide resolved

emilk reviewed Sep 15, 2023

View reviewed changes

crates/re_space_view_bar_chart/src/view_part_system.rs Outdated Show resolved Hide resolved

emilk reviewed Sep 15, 2023

View reviewed changes

crates/re_viewer_context/src/space_view/view_part_system.rs Outdated Show resolved Hide resolved

teh-cmc added 4 commits September 15, 2023 10:35

addressed PR comments

0542c92

typo

2613dc0

fix legacy plots

9d8e3ad

revert TextDocument's PrimaryNotFound check

0218525

teh-cmc requested a review from emilk September 15, 2023 09:45

emilk approved these changes Sep 15, 2023

View reviewed changes

teh-cmc mentioned this pull request Sep 15, 2023

Fix Boxes2D view not processing annotation labels #3331

Merged

3 tasks

update compatible_component_sets docs

bf0c154

teh-cmc merged commit 3322abd into main Sep 15, 2023

teh-cmc deleted the cmc/view_part_filters branch September 15, 2023 10:46

jleibs mentioned this pull request Sep 15, 2023

When no heuristics match, still pick a fallback view if possible #3342

Closed

Conversation

teh-cmc commented Sep 14, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

teh-cmc commented Sep 14, 2023

Uh oh!

Uh oh!

jleibs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jleibs Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

jleibs Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

teh-cmc Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleibs Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

teh-cmc Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

teh-cmc Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jleibs left a comment

Choose a reason for hiding this comment

Uh oh!

Wumpf commented Sep 14, 2023

Uh oh!

Uh oh!

emilk Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teh-cmc Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

teh-cmc commented Sep 15, 2023

Uh oh!

emilk left a comment

Choose a reason for hiding this comment

Uh oh!

emilk Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

teh-cmc commented Sep 14, 2023 •

edited by github-actions bot

Loading

teh-cmc Sep 15, 2023 •

edited

Loading

emilk Sep 15, 2023 •

edited

Loading