More context menu 4: create a new space view with selected entities #5411

abey79 · 2024-03-06T09:49:43Z

What

As the title says ☝🏻

Fixes Context menu: add actions for DataResults #5299

Also adds a "no action available for this selection" notice that got lost in #5392.

Makes it very easy to bump into:

Blueprint tree: provide feedback when a data result not visualisable #5410

context_menu_new_sv_with_entities_v2.mp4

Design decisions

Note: includes changes in #5423

The origin of the newly created space view is set according to the following heuristics:
- For all but 2D/3D space views: /
- For 2D space views: the origin of the subspace that contains the selected entities common ancestor.
  - If all entities are below the same pinhole, that pinhole is used as origin.
  - Otherwise, the likely subspace origin is / and the 2D space view is not suggested on ground of lack of visualizability.
- For 3D space views: the origin of the subspace that contains the selected entities common ancestor, or, if that one is rooted on a pinhole, the origin of the parent sub-space. Further, if a ViewCoordinate is logged in between the subspace origin and the common ancestor, then it's used as suggested origin. In most case, that means /, unless a DisconnectedSpace is encountered.
- Alternative considered and rejected:
  1. Set it to the clicked item. Strong reject: too arcane, different results for the same multi-selection depending on which item is actually clicked.
  2. Set it to the common ancestor of all selected entities. Weak reject: less predictable, occasionally wrong (but works around some visualisable issue we have with some space views).
  3. Always set to /. Reject: this behaviour is really broken for 2D space views.
We show a list of suggested space view classes.
- The list is the intersection of the suggested classes for each of the selected entities.
- For each entity, the suggested classes are determined based on the union of suggested classes for the entity itself, and for every entity of its subtree. This enables meaningful suggestion when selecting a pure TreePrefix.
The newly created space view is selected.

Known "phenomenons"

~~2D space views are rarely suggested, because of the origin is set to "/" and that's outside of a pinhole transform.~~ Mostly fixed by Better origin heuristics for "Add entities to new space view" #5423
Text Document and Text Log are often suggested for time series scalar, because of the Text document.
Tensor is recommended but will (sometime?) display nothing, e.g. structure_from_motion -> /camera/image
- Tensor space view refuses to display an image (e.g from structure_from_motion) #5420
If enabled, Dataframe is always the top-most suggested Space View, because of the lexicographic sorting.

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 the web demo (if applicable):
- Using newly built examples: app.rerun.io
- Using examples from latest main build: app.rerun.io
- Using full set of examples from nightly build: app.rerun.io
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
If applicable, add a new check to the release checklist!

nikolausWest · 2024-03-06T09:51:10Z

Awesome! Related to your comment on the need to show what is visualizable. That would ideally also be shown in the list of choices for view

abey79 · 2024-03-06T09:58:05Z

Awesome! Related to your comment on the need to show what is visualizable. That would ideally also be shown in the list of choices for view

Good call. I'm assuming it would be rather easy to do based only on the space origin (aka the clicked item). It might be tricker to do if we reason on all selected entities. @Wumpf and/or @jleibs we may want to shortly discuss that this afternoon.

Wumpf · 2024-03-06T10:10:55Z

I ~~think~~ believe we're in a great spot to implement a visualizability filter right away (:

abey79 · 2024-03-06T17:11:14Z

@Wumpf PR updated with the recommendation. Happy to take your feedback on the following weirdnesses.

Doesn't recommend 2D when clearly this is a 2D thing (as it shows up in a 2D space view):

Doesn't recommend anything when clearly that's a very good thing to set in space view:

crates/re_viewport/src/context_menu/actions.rs

Wumpf · 2024-03-06T19:27:02Z

Doesn't recommend 2D when clearly this is a 2D thing (as it shows up in a 2D space view):

This is the exact same issue as the "black space view" we discussed earlier. Under current rules the keypoints are not visualizable in / because that would put them into a space that requires inverting a 2D->3D transform to display something 2D which doesn't make sense.... well we'll make it work anyways though, but later.

Doesn't recommend anything when clearly that's a very good thing to set in space view:

If I'm not mistaken your code checks for the exact path to have a visualizer. curves clearly doesn't have a visualizer, it's a TreePrefix. If you want that to show up you have to take a look at the entire subtree and mark it recommended if any is visualizable.

# Conflicts: # crates/re_viewport/src/context_menu/actions.rs

abey79 · 2024-03-07T08:57:56Z

If you want that to show up you have to take a look at the entire subtree and mark it recommended if any is visualizable.

I've now done exactly that.

Wumpf · 2024-03-07T09:06:56Z

Known "phenomenons"

there's definitely some more weirdness going on beyond the known bugs. E.g. another one (?): I tried putting a markdown document in a text log since it suggested that and it doesn't show (I expected a line to show).
We'll need to study and categorize all the oddness once we figured out the main bad ones :)

Wumpf

Mostly nits, but the potentially superfluous label should be fixed before going in.
Looking good otherwise!

Wumpf · 2024-03-07T09:12:15Z

crates/re_viewport/src/context_menu/mod.rs

@@ -173,6 +174,11 @@ fn show_context_menu_for_selection(ctx: &ContextMenuContext<'_>, ui: &mut egui::

        should_display_separator |= any_action_displayed;
    }
+
+    // nothing was shown, make sure the context menu isn't empty
+    if !should_display_separator {


[not actionable] this method is getting weirder, heh. Last time I already was staring at it trying to figure out how to express this whole thing simpler, but I'm still a bit at a loss 🤷

Yeah, that flag has double-duty. I started by adding a new was_anything_added flag, and it turned out to be implemented exactly like should_display_separator. Let's see if some renaming may help here.

crates/re_viewport/src/context_menu/mod.rs

crates/re_viewport/src/context_menu/actions.rs

Wumpf · 2024-03-07T09:24:15Z

crates/re_viewport/src/context_menu/actions.rs

+        let covered = entities_of_interest.iter().all(|entity| {
+            visualizable_entities.0.iter().any(|(_, entities)| {
+                entities
+                    .0
+                    .iter()
+                    .any(|e| e == *entity || e.is_descendant_of(entity))
+            })
+        });


[not actionable]
oof that's a lot of entities to walk through if the user happens to select many. Ah well it happens only on-click and what are the odds of the user selecting everything.. right.. ;-)
We need to get establish better patterns & algorithms using prefix tree operations for things like this 🤔

Hopefully all's short-circuit should help us in most cases 🤞🏻

crates/re_viewport/src/context_menu/actions.rs

### What Follow up to #5411. This PR provides a better heuristics for the the default space origin with 2D and 3D space view. See the original PR for the (updated) design decision section. ### 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/5423/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5423/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/5423/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 * [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/5423) - [Docs preview](https://rerun.io/preview/d50861028fe4f09576a16ed46196c04bf4e5dd04/docs)  - [Examples preview](https://rerun.io/preview/d50861028fe4f09576a16ed46196c04bf4e5dd04/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

### What NOTE: review commit by commit (first commit is pure refactoring). Final PR in the series. This PR: - split `actions.rs` into many sub-files - fix capitalisation in some menu item - split the release check list in small checks Full series: - #5392 - #5397 - #5407 - #5411 - #5422 - #5433 - #5456 ### 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/5456/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5456/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/5456/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 * [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/5456) - [Docs preview](https://rerun.io/preview/a36360f586494f6648fdc4bbb9d806ab12911358/docs)  - [Examples preview](https://rerun.io/preview/a36360f586494f6648fdc4bbb9d806ab12911358/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

…5565) ### What Fix a bug introduced in #5411 and #5423 where the wrong origin would be suggested when creating a 3D space view from an entity part of a 2D space view. Quoting the original PR: > For 3D space views: the origin of the subspace that contains the selected entities common ancestor, _or_, if that one is rooted on a pinhole, the origin of the parent sub-space. Further, if a `ViewCoordinate` is logged in between the subspace origin and the common ancestor, then it's used as suggested origin. In most case, that means `/`, unless a `DisconnectedSpace` is encountered. The part of that logic where we check for a `ViewCoordinate` in the parent subspace was implemented incorrectly, which resulted in `check_context_menu_suggested_origin` to fail. With this PR, this test now passes. ### 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/5565/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5565/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/5565/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 * [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/5565) - [Docs preview](https://rerun.io/preview/268125aaddc63e0524743528cf3b34c9ea0640e3/docs)  - [Examples preview](https://rerun.io/preview/268125aaddc63e0524743528cf3b34c9ea0640e3/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

abey79 added ui concerns graphical user interface include in changelog labels Mar 6, 2024

abey79 marked this pull request as draft March 6, 2024 10:29

abey79 force-pushed the antoine/mcm3-dataresult-remove branch from 18a252f to 4a94ddc Compare March 6, 2024 10:39

abey79 force-pushed the antoine/mcm4-dataresult-add-to-new-view branch from 510f743 to 30ed5d2 Compare March 6, 2024 10:39

Base automatically changed from antoine/mcm3-dataresult-remove to main March 6, 2024 12:27

More context menu 4: create a new space view with selected entities

9ac7d5f

abey79 force-pushed the antoine/mcm4-dataresult-add-to-new-view branch from 30ed5d2 to 9ac7d5f Compare March 6, 2024 12:28

Use / as origin and suggest space view class

3bfe20e

Wumpf reviewed Mar 6, 2024

View reviewed changes

crates/re_viewport/src/context_menu/actions.rs Show resolved Hide resolved

abey79 added 3 commits March 7, 2024 09:20

Merge branch 'main' into antoine/mcm4-dataresult-add-to-new-view

27024f0

# Conflicts: # crates/re_viewport/src/context_menu/actions.rs

Post-merge fix + profile scope

f393eb3

Check sub-entities for visualizability

c54542e

Merge branch 'main' into antoine/mcm4-dataresult-add-to-new-view

7f0c607

abey79 marked this pull request as ready for review March 7, 2024 08:58

Wumpf self-requested a review March 7, 2024 09:01

Wumpf approved these changes Mar 7, 2024

View reviewed changes

abey79 added 4 commits March 7, 2024 11:23

Addressed review comments

6705925

Add release check list

d6f1000

Select the newly created space view

e24a87a

lint

979f276

abey79 merged commit 5eebbac into main Mar 7, 2024
34 of 36 checks passed

abey79 deleted the antoine/mcm4-dataresult-add-to-new-view branch March 7, 2024 13:12

This was referenced Mar 7, 2024

Tensor space view refuses to display an image (e.g from structure_from_motion) #5420

Closed

Better origin heuristics for "Add entities to new space view" #5423

Merged

abey79 mentioned this pull request Mar 11, 2024

More context menu 7: cleanup #5456

Merged

5 tasks

abey79 mentioned this pull request Mar 18, 2024

Fix a bug where an incorrect 3D space view origin could be suggested #5565

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More context menu 4: create a new space view with selected entities #5411

More context menu 4: create a new space view with selected entities #5411

abey79 commented Mar 6, 2024 •

edited

Loading

nikolausWest commented Mar 6, 2024

abey79 commented Mar 6, 2024

Wumpf commented Mar 6, 2024 •

edited

Loading

abey79 commented Mar 6, 2024

Wumpf commented Mar 6, 2024

abey79 commented Mar 7, 2024

Wumpf commented Mar 7, 2024

Wumpf left a comment

Wumpf Mar 7, 2024

abey79 Mar 7, 2024

Wumpf Mar 7, 2024

abey79 Mar 7, 2024

More context menu 4: create a new space view with selected entities #5411

More context menu 4: create a new space view with selected entities #5411

Conversation

abey79 commented Mar 6, 2024 • edited Loading

What

Design decisions

Known "phenomenons"

Checklist

nikolausWest commented Mar 6, 2024

abey79 commented Mar 6, 2024

Wumpf commented Mar 6, 2024 • edited Loading

abey79 commented Mar 6, 2024

Wumpf commented Mar 6, 2024

abey79 commented Mar 7, 2024

Wumpf commented Mar 7, 2024

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf Mar 7, 2024

Choose a reason for hiding this comment

abey79 Mar 7, 2024

Choose a reason for hiding this comment

Wumpf Mar 7, 2024

Choose a reason for hiding this comment

abey79 Mar 7, 2024

Choose a reason for hiding this comment

abey79 commented Mar 6, 2024 •

edited

Loading

Wumpf commented Mar 6, 2024 •

edited

Loading