Make AutoSpaceView default consistently when resetting blueprint #3077
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
Wumpf
approved these changes
Aug 23, 2023
There was a problem hiding this comment.
I think this is the right thing to do. In #3078 heuristics already get a lot faster and we know a way forward on how to not run them all the time and get them faster still.
What I'm a little bit worried about is the discrepancy between default_for and default which is #3078 uses in case of serialization failure
|
Replaced by: #3129 |
3 tasks
jleibs
added a commit
that referenced
this pull request
Aug 28, 2023
### What arrow2-convert forces us into needing to provide data even if serde fail to deserialize the field. Fall back to default in this case, but specify invalid. Additionally, this checks to see if a blueprint is totally invalid and if so resets it to the default state. Because this can reset the blueprint immediately upon loading, it was important that this properly set auto-space-views. Needed to pull in a slightly different version of #3077 that still works with the now-required usage of Default. Resolves: #3127 ### 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/3129) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3129) - [Docs preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/examples) <!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --------- Co-authored-by: Andreas Reich <[email protected]>
jleibs
added a commit
that referenced
this pull request
Aug 29, 2023
### What * Fixes #2285 (.. at least fixes it well enough for the moment I reckon) Improves both the per space view rendering and the space view adding heuristic. In particular on the later there's still significant gains to be made. Most importantly we should be scaling now much better with number of added systems. Went fairly early with the decision to have the primary datastructure to pass around be a `Map<System, Vec<Entity>>` (conceptual) instead of the other way round since this is what we need in the systems most of the time. This is another step towards a stronger contract of systems specifying what components they will query ahead of time! The way it is implemented on thr (rename from `BlueprintTree` 💥) `SpaceViewContents` also paves the way for ui selection of systems for a given entity path which complements the ongoing work on our new data ingestion interfaces. ----- Testcase: `examples/python/open_photogrammetry_format/main.py --no-frames`, **fully reset viewer** and then hiding **NOT REMOVING** the points (the world/pcl entity) and deselecting them. (The bold marked pieces are very important as they have an influence on what heuristics run - see #3077) Before:  After:  (release runs, averaged over a bunch of frames on my M1 Max) Highlights: * `AppState::show`: 24.0ms ➡️ 14.6ms * `SpaceViewBlueprint::scene_ui`: 4.9ms ➡️ 2.6ms * this was the original primary optimization target! * `Viewport::on_frame_start`: 15.2ms ➡️ 7.4ms * `default_created_space_view`: 12.7ms ➡️ 5.1ms * quite some time spent on this, but we want to do a paradigm shift there: #3079 (slight number discrepancies from the screenshot are due to doing a different run) ---- Draft todo: Code sanity check. Suspecting some heuristics might be slightly broken, need to go through examples ### 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/3078) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3078) - [Docs preview](https://rerun.io/preview/fc5bb0b8908e2e8c666a6d5daa1dee105fe2aae8/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/fc5bb0b8908e2e8c666a6d5daa1dee105fe2aae8/examples) <!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --------- Co-authored-by: Jeremy Leibs <[email protected]>
jleibs
added a commit
that referenced
this pull request
Aug 31, 2023
### What arrow2-convert forces us into needing to provide data even if serde fail to deserialize the field. Fall back to default in this case, but specify invalid. Additionally, this checks to see if a blueprint is totally invalid and if so resets it to the default state. Because this can reset the blueprint immediately upon loading, it was important that this properly set auto-space-views. Needed to pull in a slightly different version of #3077 that still works with the now-required usage of Default. Resolves: #3127 ### 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/3129) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3129) - [Docs preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/examples) <!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --------- Co-authored-by: Andreas Reich <[email protected]>
jleibs
added a commit
that referenced
this pull request
Aug 31, 2023
### What arrow2-convert forces us into needing to provide data even if serde fail to deserialize the field. Fall back to default in this case, but specify invalid. Additionally, this checks to see if a blueprint is totally invalid and if so resets it to the default state. Because this can reset the blueprint immediately upon loading, it was important that this properly set auto-space-views. Needed to pull in a slightly different version of #3077 that still works with the now-required usage of Default. Resolves: #3127 ### 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/3129) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3129) - [Docs preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/examples) <!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --------- Co-authored-by: Andreas Reich <[email protected]>
jleibs
added a commit
that referenced
this pull request
Aug 31, 2023
### What arrow2-convert forces us into needing to provide data even if serde fail to deserialize the field. Fall back to default in this case, but specify invalid. Additionally, this checks to see if a blueprint is totally invalid and if so resets it to the default state. Because this can reset the blueprint immediately upon loading, it was important that this properly set auto-space-views. Needed to pull in a slightly different version of #3077 that still works with the now-required usage of Default. Resolves: #3127 ### 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/3129) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3129) - [Docs preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/examples) <!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/) --------- Co-authored-by: Andreas Reich <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
We noticed that although the blueprint "reset" button resets the view and re-runs the heuristics (once), the state we end up in is slightly different from the "default" blueprint state, which is to rerun the heuristics on every frame.
This actually explains some inconsistent behavior we have seen recently as once a blueprint is cached you never actually get the full incremental auto-heuristic behavior again (for better of for worse).
WARNING: this actually introduces a performance regression into scenes with large number of entities because the auto heuristic is currently slow. A potential point of discussion is whether this is the right thing to do.
Checklist