-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Shadow Hierarchies (aka fragments/auxilary nodes) #15238
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Thanks a ton for tackling this :) |
Now that I have had a chance to think about this, I'm a bit concerned that not using Parent/Child may end up creating more work in the long run. Essentially, the code that iterates the entity hierarchy falls into two groups: code that wants to see the shadow nodes, and code that doesn't. The question is which group is larger. If the code that wants to hide the shadow nodes is the larger group, then this approach makes sense: the smaller body of code will have to be changed to use the new APIs. OTOH, if there's more code that wants to treat the shadow hierarchy as part of the regular hierarchy than code that wants to elide the shadow nodes, then this approach means that more code will have to be changed. For example, message bubbling wants to see the shadow nodes: that is, events which are triggered on a child entity want to bubble upwards, visiting the shadow entities as if they were normal entities - because shadow nodes can and will have observers that watch these events (that's part of the reason for having them in the hierarchy). If we go with the ShadowChild approach, it will require changing the implementation of observers to be able to work with either regular children or shadow children. So we'll end up not only having to change layout and rendering, but observers as well - and possibly other things I haven't thought of. |
We had a small discussion on Discord Most importantly
|
I'm not sure I understand. Just to be clear, nothing in the proposed solution is preventing the use of Parent/Child. They are even required for the shadow hierarchy to work. The proposed extension is very much opt-in, and even allows using different shadow hierarchies for different purposes, all backed by the real Parent/Child hierarchy.
I would say this proposed approach is more focused on the latter. Shadow hierarchies are opt-in so they would only be used where needed. BUT I intially thought that we don't already support "fragments" in non-UI spatial hierarchies, but I realized that we already kind of do. Simply adding a SpatialBundle::INHERITED_IDENTITY (or whatever will replace it in the near future) should be enough for a node to be treated as a shadow node in both transform and visibility propagation. It might not be the prettiest, but it should work, and does not require any changes to those propagation systems (which I'm not sure I am comfortable doing as a first contribution 😅) That leaves us with Bevy UI layouts as the only currently known use-case for a An alternative, more focused approachTaking the above into consideration, we could make this issue a lot leaner by not changing anything outside of The approach could be something like:
This approach would allow "Fragments" in UI hierarchies without having to worry about how it affects the rest of the Bevy codebase, making this issue a lot narrower in scope. @viridia Any thoughts on this approach? Is anyone else interested in this idea?Can't answer for everyone else, but for me personally I see it this way: I am very impressed by all the work/experiments @viridia has put into reactiveness 😍 . Working a lot with frontend web development in my day job I have really fallen for the way we write code in frameworks/libraries like React and Vue. My personal opinion is that a UI library today is not complete without a solid reactive solution. I would love to have a first-party reactiveness solution in Bevy not just for UI, but also for other parts of scenes. I imagine some games could be coded almost entirely as a reactive scene. While looking for a way to contribute to this vision, Shadow Nodes seemed like an important step towards allowing further experimentation. And with the alternative approach above there wouldn't be much to implement to make it possible. I don't know how these things are usually decided, how much blessing would this need to actually be considered for merging? @alice-i-cecile |
So far, it seems that the position is largely indifference, and I don't think these are particularly intrusive. I'm broadly in favor of this, no one is pushing back against them. That said, I suspect @cart will want to take a look: if you can prepare a clean, concise summary of why they're useful I think this work will be mergeable as soon as the implementation is complete and he has some time to consider the proposal. The generic idea seems useful, but personally, in terms of feedback, I think that the best way to do this is to use semantically meaningful names for the marker components, and write a Then, the marker component can live inside of |
@alice-i-cecile @villor I've given a much longer motivation in my latest comments to #14826 . One thing I really want to avoid is having different kinds of "if" statements for UI hierarchies and 3D hierarchies - I want to make sure that the same shadowing mechanism works for both cases. |
I'm already pretty convinced of the value of "shadow hierarchies". My general take is:
In general it seems like this PR is on the right track in that it defaults to parents/children existing. And the changes here seem pretty uncontroversial given that they are generically useful, even outside of the "shadow hierarchy" context. That being said, I kind of think we should wait, manually implement the |
@alice-i-cecile @cart It makes sense to wait and not prematurely generalize this. I'll try to implement ghost nodes within @viridia
In |
# Objective - Fixes #14826 - For context, see #15238 ## Solution Add a `GhostNode` component to `bevy_ui` and update all the relevant systems to use it to traverse for UI children. - [x] `ghost_hierarchy` module - [x] Add `GhostNode` - [x] Add `UiRootNodes` system param for iterating (ghost-aware) UI root nodes - [x] Add `UiChildren` system param for iterating (ghost-aware) UI children - [x] Update `layout::ui_layout_system` - [x] Use ghost-aware root nodes for camera updates - [x] Update and remove children in taffy - [x] Initial spawn - [x] Detect changes on nested UI children - [x] Use ghost-aware children traversal in `update_uinode_geometry_recursive` - [x] Update the rest of the UI systems to use the ghost hierarchy - [x] `stack::ui_stack_system` - [x] `update::` - [x] `update_clipping_system` - [x] `update_target_camera_system` - [x] `accessibility::calc_name` ## Testing - [x] Added a new example `ghost_nodes` that can be used as a testbed. - [x] Added unit tests for _some_ of the traversal utilities in `ghost_hierarchy` - [x] Ensure this fulfills the needs for currently known use cases - [x] Reactivity libraries (test with `bevy_reactor`) - [ ] Text spans (mentioned by koe [on discord](https://discord.com/channels/691052431525675048/1285371432460881991/1285377442998915246)) --- ## Performance [See comment below](#15341 (comment)) ## Migration guide Any code that previously relied on `Parent`/`Children` to iterate UI children may now want to use `bevy_ui::UiChildren` to ensure ghost nodes are skipped, and their first descendant Nodes included. UI root nodes may now be children of ghost nodes, which means `Without<Parent>` might not query all root nodes. Use `bevy_ui::UiRootNodes` where needed to iterate root nodes instead. ## Potential future work - Benchmarking/optimizations of hierarchies containing lots of ghost nodes - Further exploration of UI hierarchies and markers for root nodes/leaf nodes to create better ergonomics for things like `UiLayer` (world-space ui) --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: UkoeHB <[email protected]>
# Objective - Fixes bevyengine#14826 - For context, see bevyengine#15238 ## Solution Add a `GhostNode` component to `bevy_ui` and update all the relevant systems to use it to traverse for UI children. - [x] `ghost_hierarchy` module - [x] Add `GhostNode` - [x] Add `UiRootNodes` system param for iterating (ghost-aware) UI root nodes - [x] Add `UiChildren` system param for iterating (ghost-aware) UI children - [x] Update `layout::ui_layout_system` - [x] Use ghost-aware root nodes for camera updates - [x] Update and remove children in taffy - [x] Initial spawn - [x] Detect changes on nested UI children - [x] Use ghost-aware children traversal in `update_uinode_geometry_recursive` - [x] Update the rest of the UI systems to use the ghost hierarchy - [x] `stack::ui_stack_system` - [x] `update::` - [x] `update_clipping_system` - [x] `update_target_camera_system` - [x] `accessibility::calc_name` ## Testing - [x] Added a new example `ghost_nodes` that can be used as a testbed. - [x] Added unit tests for _some_ of the traversal utilities in `ghost_hierarchy` - [x] Ensure this fulfills the needs for currently known use cases - [x] Reactivity libraries (test with `bevy_reactor`) - [ ] Text spans (mentioned by koe [on discord](https://discord.com/channels/691052431525675048/1285371432460881991/1285377442998915246)) --- ## Performance [See comment below](bevyengine#15341 (comment)) ## Migration guide Any code that previously relied on `Parent`/`Children` to iterate UI children may now want to use `bevy_ui::UiChildren` to ensure ghost nodes are skipped, and their first descendant Nodes included. UI root nodes may now be children of ghost nodes, which means `Without<Parent>` might not query all root nodes. Use `bevy_ui::UiRootNodes` where needed to iterate root nodes instead. ## Potential future work - Benchmarking/optimizations of hierarchies containing lots of ghost nodes - Further exploration of UI hierarchies and markers for root nodes/leaf nodes to create better ergonomics for things like `UiLayer` (world-space ui) --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: UkoeHB <[email protected]>
This is my first ever draft/PR for Bevy, so please let me know if there is anything missing
Objective
See for background: #14826
Fragment
works in react.Solution
It seems like this would benefit from some kind of generic solution for this in
bevy_hierarchy
, I'm going to call it shadow hierarchies for now.We want to iterate children as if some entities in the hierarchy does not exist, let's call those
shadow entities
. The children of those shadow entities are considered children of the shadow entity's first non-shadow ancestor, unless the child is also a shadow entity, in which case the traversal continues...So, we have a
shadow hierarchy
. A hierarchy where some entities are treated asshadow entities
. (naming open for discussion)The proposed solution is to use a marker component to mark shadow entities, for this example let's call it
ShadowChild
.I have added a generic extension to traverse the shadow hierarchy finding the children of an entity:
iter_children_shadowed
. The function can be called on aQuery<Option<&Children>, Option<&T>>
whereT
is the component marking the shadow entities.For completeness, there should probably also be a
get_parent_shadowed
(not yet implemented), which would get the closest non-shadow ancestor orNone
if not present.Testing
iter_children_shadowed
to test traversal with shadow entities both as a parent and leaf node.Questions
Fragment
/ShadowChild
. Or should things be kept separate with their own shadow hierarchies? E.g aLayoutShadowChild
/SpatialShadowChild
etciter_children_shadowed
enough for updating the UI layout code, or are further optimizations necessary?TODO
iter_children_shadowed
get_parent_shadowed
Potential optimization: Retained shadow hierarchies
Needs opinions. Is this necessary?
One downside of the extensions above are that they might do some unnecessary tree traversal on every frame. Compare this to how layout code works today, iterating
Children
directly, no traversal necessary.One way to solve this could be to allow defining shadow hierarchies upfront in
bevy_hierarchy
, which would store them just likeParent
andChildren
, and update them automatically.We could add a new pair of components:
To make use of these, the shadow hierarchy would need to be initialized upfront:
This would set up the systems/hooks/observers necessary to keep them up-to-date.
Those components could then be used as drop-in replacements for
Parent
andChildren
in systems where its necessary to skip certain entities.