Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view #5164

jleibs · 2024-02-09T18:21:40Z

What

Resolves: Bad incremental heuristics for Detect and Track Objects #5152

This evaluates the queries of newly added space-views and doesn't add them if there already exists a space-view that would contain the same contents. This eliminates most of the worst offenders of unneeded-space-view creation, though re-loading recordings can still cause spurious root-space-views since those wouldn't technically be redundant.

Because this check operates on the query expressions it's independent of the actual state of the entity-tree.

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!

crates/re_log_types/src/path/entity_path_filter.rs

emilk · 2024-02-12T08:36:20Z

crates/re_space_view/src/space_view.rs

+        // TODO(jleibs): Handle multi-query-aggregation
+
+        // If other has no query, by definition we contain all entities from it.
+        let Some(q_other) = other.queries.first() else {
+            return true;
+        };
+
+        // If other has any query, but self has no query, we clearly can't contain it
+        let Some(q_self) = self.queries.first() else {
+            return false;
+        };
+
+        // If this query fully contains the other, then we have all its entities
+        q_self.fully_contains(q_other)


This seems like a big shortcoming that this only handles queries.len() == 1 on both sides, and not even returning a conservative estimate in the other cases

crates/re_viewport/src/viewport.rs

emilk · 2024-02-12T08:39:09Z

I don't understand the nuances of the heuristics enough to feel confident merging this without @Wumpf taking a look first

Wumpf

commentary only so far - looks almost good, but I need to test now the issue in should_auto_add_space_view

crates/re_viewport/src/viewport.rs

crates/re_space_view/src/space_view.rs

crates/re_space_view/src/data_query_blueprint.rs

crates/re_log_types/src/path/entity_path_filter.rs

Wumpf

it's a clear improvement. I'll fix up comments & naming and merge

worth noting that naturally, at this point we still have the problem of the / space view being created prematurely and then not removed as images come in

Wumpf · 2024-02-12T09:34:43Z

actually something weird here: Why is the root view added, that shouldn't have happend.
So on initial run I get the root and only the root

but then reset. Close. open again. I get the root again as in my screenshot from the previous comment:

Wumpf · 2024-02-12T09:38:31Z

I believe the problem is that the heuristic still proposes / after it already has the other two space views which were loaded on startup. / is clearly not a superset of the other two so it's added.

Wumpf · 2024-02-12T09:39:41Z

Seems to be all addressed in the follow-up PR :) (started testing it without reading yet)

### What - Builds on top of #5164 Adds a store subscriber for tracking the dimensions of image entities. Gets rid of the previous bucketing logic and replaces it with a new implementation that starts at the top of the subspace and incrementally splits when it finds conflicting image entities within the space. ### 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/5166/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5166/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/5166/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/5166) - [Docs preview](https://rerun.io/preview/da9a0d206235016d14d9826e8c42ed8481d29643/docs)  - [Examples preview](https://rerun.io/preview/da9a0d206235016d14d9826e8c42ed8481d29643/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --------- Co-authored-by: Andreas Reich <[email protected]>

…e completely covered by an existing view (#5164) ### What - Resolves: #5152 This evaluates the queries of newly added space-views and doesn't add them if there already exists a space-view that would contain the same contents. This eliminates most of the worst offenders of unneeded-space-view creation, though re-loading recordings can still cause spurious root-space-views since those wouldn't technically be redundant. Because this check operates on the query expressions it's independent of the actual state of the entity-tree. ### 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/5164/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5164/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/5164/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/5164) - [Docs preview](https://rerun.io/preview/e35bd2e974540f0b3b2b9c2759f64dbfa635d515/docs)  - [Examples preview](https://rerun.io/preview/e35bd2e974540f0b3b2b9c2759f64dbfa635d515/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --------- Co-authored-by: Emil Ernerfeldt <[email protected]> Co-authored-by: Andreas Reich <[email protected]>

### What - Builds on top of #5164 Adds a store subscriber for tracking the dimensions of image entities. Gets rid of the previous bucketing logic and replaces it with a new implementation that starts at the top of the subspace and incrementally splits when it finds conflicting image entities within the space. ### 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/5166/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5166/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/5166/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/5166) - [Docs preview](https://rerun.io/preview/da9a0d206235016d14d9826e8c42ed8481d29643/docs)  - [Examples preview](https://rerun.io/preview/da9a0d206235016d14d9826e8c42ed8481d29643/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --------- Co-authored-by: Andreas Reich <[email protected]>

jleibs added 3 commits February 9, 2024 18:58

New EntityPathFilter check for fully containing another filter

0ee9f72

Use the new contains check

b7515fe

Also check for excluded inclusions

1f0137b

jleibs added heuristics Heuristics for placing views, fallback provider, etc. 🦟 regression A thing that used to work in an earlier release labels Feb 9, 2024

jleibs added this to the 0.13 milestone Feb 9, 2024

jleibs added the include in changelog label Feb 9, 2024

jleibs marked this pull request as ready for review February 9, 2024 18:22

jleibs added 2 commits February 9, 2024 19:43

Clean up docs

0c83448

lint

7f1244a

This was referenced Feb 9, 2024

Refactor the 2D heuristic again #5166

Merged

Heuristic shouldn't add overall general space-views if the existing views cover it in aggregate #5167

Open

emilk self-requested a review February 12, 2024 08:17

emilk added 2 commits February 12, 2024 09:25

Use self_ and other_ prefixes in superset test

54109af

Improve unit tests slightly

522b7fb

emilk reviewed Feb 12, 2024

View reviewed changes

emilk requested a review from Wumpf February 12, 2024 08:38

Wumpf reviewed Feb 12, 2024

View reviewed changes

Wumpf approved these changes Feb 12, 2024

View reviewed changes

Wumpf added the 📺 re_viewer affects re_viewer itself label Feb 12, 2024

comment, naming & test module fixes

e35bd2e

Wumpf merged commit 2ebc621 into main Feb 12, 2024
40 of 41 checks passed

Wumpf deleted the jleibs/no_redundant_space_view branch February 12, 2024 10:13

Wumpf mentioned this pull request Feb 12, 2024

Bad SpaceView heuristics #5125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view #5164

Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view #5164

jleibs commented Feb 9, 2024 •

edited by Wumpf

Loading

emilk Feb 12, 2024 •

edited

Loading

emilk commented Feb 12, 2024

Wumpf left a comment

Wumpf left a comment

Wumpf commented Feb 12, 2024 •

edited

Loading

Wumpf commented Feb 12, 2024 •

edited

Loading

Wumpf commented Feb 12, 2024

Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view #5164

Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view #5164

Conversation

jleibs commented Feb 9, 2024 • edited by Wumpf Loading

What

Checklist

emilk Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

emilk commented Feb 12, 2024

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf commented Feb 12, 2024 • edited Loading

Wumpf commented Feb 12, 2024 • edited Loading

Wumpf commented Feb 12, 2024

jleibs commented Feb 9, 2024 •

edited by Wumpf

Loading

emilk Feb 12, 2024 •

edited

Loading

Wumpf commented Feb 12, 2024 •

edited

Loading

Wumpf commented Feb 12, 2024 •

edited

Loading