Skip to content
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

Remove groups from blueprints panel #5326

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Remove groups from blueprints panel #5326

merged 4 commits into from
Feb 29, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Feb 28, 2024

What

Ui 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 & recursive which together get accumulated. The rule for accumulated is to combine for each entity all of it's parent recursive item with its single local individual one. 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 recursive and any edit on an entity path edited individual. Easy!
Now we'd arguably always like to edit recursive but if we were not to show accumulated things 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 recursive and indicate accumulated visibility by greying out - the eye icons however are strictly following recursive. Everything on the selection panel both shows and edits individual.
This way we don't run into strange cases where looking at a thing gives us edits (like showing accumulated but 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 individual for 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

  • 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):
  • 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!

@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🟦 blueprint The data that defines our UI include in changelog labels Feb 28, 2024
@Wumpf Wumpf force-pushed the andreas/remove-groups branch 2 times, most recently from 1be5267 to 56ee868 Compare February 28, 2024 13:31
@Wumpf Wumpf requested a review from jleibs February 28, 2024 14:19
@Wumpf Wumpf force-pushed the andreas/remove-groups branch from 56ee868 to 278a1e4 Compare February 28, 2024 15:01
// 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);
Copy link
Member Author

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

@Wumpf Wumpf force-pushed the andreas/remove-groups branch from 278a1e4 to f47b335 Compare February 28, 2024 18:18
Comment on lines +27 to +28
/// The recursive property set in this `DataResult`, if any.
pub recursive_properties: Option<EntityProperties>,
Copy link
Member

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?

Copy link
Member

@abey79 abey79 Feb 29, 2024

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.

Copy link
Member Author

@Wumpf Wumpf Feb 29, 2024

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?

Copy link
Member

@abey79 abey79 Feb 29, 2024

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 :)

Copy link
Member

@abey79 abey79 left a 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...

@Wumpf Wumpf merged commit 4dc09f1 into main Feb 29, 2024
39 checks passed
@Wumpf Wumpf deleted the andreas/remove-groups branch February 29, 2024 10:15
Wumpf added a commit that referenced this pull request Feb 29, 2024
… 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)
abey79 added a commit that referenced this pull request Mar 4, 2024
### 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)
@emilk emilk changed the title Remove groups from blueprint Remove groups from blueprints panel Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI include in changelog 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate "groups" as a concept from the DataResultTree.
2 participants