-
Notifications
You must be signed in to change notification settings - Fork 612
Remove groups from blueprints panel #5326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1be5267 to
56ee868
Compare
56ee868 to
278a1e4
Compare
| // Ignore empty nodes. | ||
| // Since we recurse downwards, this prunes any branches that don't have anything to contribute to the scene | ||
| // and aren't directly included. | ||
| let direct_included = self.entity_path_filter.is_exact_included(entity_path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong and fixed in follow-up PR: direct_included should still just use is_included. But the if check here indeed wants to work with is_exact_included
…lection panel always operates on individual
278a1e4 to
f47b335
Compare
| /// The recursive property set in this `DataResult`, if any. | ||
| pub recursive_properties: Option<EntityProperties>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, I'm still confused by the meaning of "accumulated" and "recursive". Why isn't this the same thing? Also, why is recursive_properities and option. Shouldn't a data result always inherit from some properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm nvm, coffee finally kicked in. Still probably worth expending a little bit in the comments, because the nomenclature is non-obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can expand on it similar to how I did in the pull request description here. Would that help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, let's fix the semantics first before patching up with comments :)
abey79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look mainly for learning purposes, but FWIW I loved how much code got removed and the simplification of the DataResult API. I didn't catch anything shocking. Well, except for the entire property cascading story that we already discussed...
… called out explicitely (#5342) ### What * Direct follow up of #5326 * Fixes #4156 https://github.com/rerun-io/rerun/assets/1220815/77ed6a42-50db-4ca3-b596-8dcf5bce9baf Follows the design proposal from the ticket directly. Some detail decisions on how things are exactly handled, but code should be a bit more composable now (albeit admittedly still a bit messy) ### 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/5342/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5342/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/5342/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/5342) - [Docs preview](https://rerun.io/preview/6011f651a1ebd432d77d25d3edd7ea746053fd99/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/6011f651a1ebd432d77d25d3edd7ea746053fd99/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
### What This PR adds a subtly different icon for "empty" entities. By "empty", I mean "no component logged _on the current timeline_". This means that changing timeline may potentially change the icon of some entity (though it takes a somewhat contrived example to do so). TODO: - [x] do the same in blueprint tree after #5326 is merged <img width="187" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/197d99ce-9f51-4468-85ce-978e6fd92b43"> <img width="252" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/bde725d6-aa6b-44ed-9faa-3ce1c6ec7915"> <img width="249" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/23e4168f-ddd9-4f10-91c5-fbfc79736d2b"> <img width="195" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/b0dc0113-9b43-4248-9718-c33b7a2444b7"> Fixes #5302 ### 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/5338/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5338/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/5338/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/5338) - [Docs preview](https://rerun.io/preview/33ddbf880c630f2053e269d9403230dde1ab63ca/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/33ddbf880c630f2053e269d9403230dde1ab63ca/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
What
DataResultTree. #4441Ui changes were almost trivial, all the devil in the details is which exact property overrides are shown and edited where, mostly by virtue of that never showing up because of groups
Today, we have two kinds of overrides:
individual&recursivewhich together getaccumulated. The rule foraccumulatedis to combine for each entity all of it's parentrecursiveitem with its single localindividualone. Note that combination rules are a bit strange at times and have no concept of "this was not edited", e.g. a thing is visible if both parties are visible with no regard to whether any of the sources has actually been set (no tri-state as would be mandated by upcoming property overrides!)Before, any edit on a group edited
recursiveand any edit on an entity path editedindividual. Easy!Now we'd arguably always like to edit
recursivebut if we were not to showaccumulatedthings get extremely messy as you're not editing what you see.So instead for now I went the path that visibility edits on the blueprint panel edit
recursiveand indicateaccumulatedvisibility by greying out - the eye icons however are strictly followingrecursive. Everything on the selection panel both shows and editsindividual.This way we don't run into strange cases where looking at a thing gives us edits (like showing
accumulatedbut editing something else would cause continuous edits without effect!) but it's overall a pretty bad scheme.@abey79 and me discussed this situation for a bit and concluded what we'll have to embrace the null-state (often less correct dubbed tri-state: null, visible, invisible) given by component overrides one way or the other, especially to support cases like having visible objects with a parent that inherits invisible otherwise. While there's still many open questions, I believe that we should have null-states everywhere with a simple inheritance scheme of "inherit until non-null is found". The elegance in that is that "accumulated == override if override set" - to cement that I'd propose to drop
individualfor the forseeable future.The tricky thing with this is that each ui element needs to present "not set" in some way and allow to reset it.
Checklist
mainbuild: app.rerun.ionightlybuild: app.rerun.io