Filter entities in the UI (part 6): Refactor re_blueprint_tree and add more tests
#8795
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
fixed bug in data fixed data
test passing for old code improved test code fixed test
fixed blueprint
abey79
added
ui
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
abey79
force-pushed
the
antoine/filt6-blueprint-view-refactor
branch
from
974a30a to
e0dbdf4
Compare
emilk
reviewed
Jan 24, 2025
emilk
approved these changes
Jan 27, 2025
Comment on lines
+200
to
+207
| result_tree.visit(&mut |node| { | ||
| if node | ||
| .data_result | ||
| .entity_path | ||
| .starts_with(&view_blueprint.space_origin) | ||
| { | ||
| // If it's under the origin, we're not interested, stop recursing. | ||
| false |
There was a problem hiding this comment.
Reading this code it's not immediately obvious what returning false from visit means. It's not a huge issue, but it could probably be made easier to read by using something like std::ops::ControlFlow
There was a problem hiding this comment.
Good call! This existing visit method is used in a bunch of places so I won't change it here, but I'll keep that in mind and might do a separate cleanup PR.
Co-authored-by: Emil Ernerfeldt <[email protected]>
Co-authored-by: Emil Ernerfeldt <[email protected]>
Co-authored-by: Emil Ernerfeldt <[email protected]>
This was referenced Jan 31, 2025
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.
Related
CustomContentmore useful #8645re_blueprint_treeand add more tests #8795re_time_paneland add more tests #8863What
This PR introduces a complete refactor of the blueprint tree to split the data processing part (including filtering) and the UI part. It also introduce a bunch of UI snapshot and insta tests to protect us against regressions.
Visual changes
This refactor highlighted a number of bugs and inconsistencies in the way we're presenting the blueprint tree.
Example of a bug (wrong highlighting of the '(root)' label:
Example of an inconsistency (we display an origin stub but wouldn't match on it):
Performance
Technically, this PR introduce a performance regression in the sense that we now always walk the entire tree to extract data, even if such data is not needed—e.g. because the display is fully collapsed and not filtered.
However, benchmark have shown that this overhead is very small, and far outweighed by the gain in code clarity.
Full benchmark analysis/discussion on slack: https://rerunio.slack.com/archives/C041NHU952S/p1737709242394899
TL;DR: 0.3ms in a complex case where the entire blueprint tree takes 3.6ms.
Comparing with 0.21 shows not significant regression.
This PR:
0.21.0:
setup used: