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

Shadow Hierarchies (aka fragments/auxilary nodes) #15238

Closed
wants to merge 1 commit into from

Conversation

villor
Copy link
Contributor

@villor villor commented Sep 15, 2024

This is my first ever draft/PR for Bevy, so please let me know if there is anything missing

Objective

See for background: #14826

  • TLDR: In some cases (like reactive UIs), we need a way to "skip" certain entities in the tree, treating their children as children of their parent in some contexts, like layout. Similar to how 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 as shadow 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 a Query<Option<&Children>, Option<&T>> where T 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 or None if not present.

Testing

  • New testcase added for iter_children_shadowed to test traversal with shadow entities both as a parent and leaf node.

Questions

  • Naming of stuff?
  • Should there be one "generic" marker component for shadow entities used by engine things like UI layout, transform propagation, visibility propagation etc? E.g. a Fragment/ShadowChild. Or should things be kept separate with their own shadow hierarchies? E.g a LayoutShadowChild/SpatialShadowChild etc
  • Is iter_children_shadowed enough for updating the UI layout code, or are further optimizations necessary?
  • Do we also want to implement shadow hierarchies for transform/visibility propagation?

TODO

  • Implement iter_children_shadowed
  • Implement get_parent_shadowed
  • Update UI layout system (and maybe also transform/visibility propagation?) to use a shadow hierarchy
  • Optimize? see below

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 like Parent and Children, and update them automatically.

We could add a new pair of components:

#[derive(Component)]
struct ShadowParent<T: Component>(pub(crate) Entity);

#[derive(Component)]
struct ShadowChildren<T: Component>(pub(crate) SmallVec<[Entity; 8]>);

To make use of these, the shadow hierarchy would need to be initialized upfront:

app.init_shadow_hierarchy::<ShadowChild>();

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 and Children in systems where its necessary to skip certain entities.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@villor villor marked this pull request as draft September 15, 2024 21:45
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets A-Hierarchy Parent-child entity hierarchies M-Needs-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 15, 2024
@alice-i-cecile
Copy link
Member

Thanks a ton for tackling this :)

@viridia
Copy link
Contributor

viridia commented Sep 15, 2024

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.

@villor
Copy link
Contributor Author

villor commented Sep 15, 2024

We had a small discussion on Discord

Most importantly
DreamerTalin (viridia):

Some questions I would like to ask are:

  • Is this idea even feasible?
  • Is it hard?
  • What parts of Bevy would be affected?
  • Is anyone else interested in this idea?
  • Are there other use cases for this besides the ones I have thought of?

@villor
Copy link
Contributor Author

villor commented Sep 16, 2024

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.

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.

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.

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 shadow hierarchy.

An alternative, more focused approach

Taking the above into consideration, we could make this issue a lot leaner by not changing anything outside of bevy_ui and possibly bevy_hierarchy for the traversal extension.

The approach could be something like:

  • Keep the iter_children_shadowed
  • Add a new component ShadowNode for marking shadow nodes (maybe we should call it GhostNode or something instead to avoid confusion with actual box/drop shadows)
    • This component would require any components that is currently added by SpatialBundle, making sure the transform and visibility propagation systems still work as intended.
  • Update the UI layout system to iterate children in a shadow hierarchy (ghost hierarchy?) shadowed (ghosted?) by ShadowNode using iter_children_shadowed instead of iterating Children directly.
  • Potentially optimize by retaining (caching) the shadowed children on a component. But only if there is a significant reason to do so. The traversal might just be fast enough for most cases unless there will be deeply nested shadow nodes somewhere.

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

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Sep 16, 2024

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 iter_descendants_skipping_marked (bikeshed pending) generic method. Ultimately we don't really need a "new kind of hierarchy", we just need a helper method on recursive tree search, which is much less controversial and disruptive to teach users. This is very similar to your " alternative, more focused approach" :)

Then, the marker component can live inside of bevy_ui (in a follow-up PR), and it can have a clear name and great domain-specific documentation.

@viridia
Copy link
Contributor

viridia commented Sep 16, 2024

@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.

@cart
Copy link
Member

cart commented Sep 17, 2024

I'm already pretty convinced of the value of "shadow hierarchies". My general take is:

  1. They should still use Parent/Children. From a hierarchy perspective, a shadowed child is just a "normal child". By default, they are still visible.
  2. For contexts that need to care, add a marker component to mark a child as somehow different (ex: ShadowNode for UI layout stuff). Those contexts then choose how those entities should behave as a reaction to the presence of the component. They can also choose how to check for that components presence.
  3. We should consider providing generic tools to make (2) easier.

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 ShadowNode case first, then try to generalize it if we find multiple use cases with the same requirements. Theres nothing preventing people from implementing this logic in userspace, and I suspect that each case might have its own constraints (ex: maybe there are multiple components involved, maybe they need to iterate in a different order, etc).

@villor
Copy link
Contributor Author

villor commented Sep 20, 2024

@alice-i-cecile @cart
Thanks for the feedback!

It makes sense to wait and not prematurely generalize this. I'll try to implement ghost nodes within bevy_ui instead. I have a WIP branch of this, will post a draft PR soon.

@viridia
I hope the solution will be still be sufficient enough for you. I don't think you would need different kinds of if statements etc. But you would have to add different components to every entity depending on the context.

  • In a UI hierarchy: bevy_ui::GhostNode would be added to any auxilary nodes, as well as any "view root" nodes. The whole hierarchy has to consist of either Node or GhostNode for the layout system to work.
  • In a 3D hierarchy, shouldn't it be enough to just add the SpatialBundle components to every entity? That way you get rid of the warnings and the propagation will work.

In bevy_reactor_views, maybe you could have a View::to_ui_root() and View::to_spatial_root() to keep track of what components to add to all sub-entities.

@villor villor mentioned this pull request Sep 20, 2024
21 tasks
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2024
# 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 <alice.i.cecile@gmail.com>
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
# 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 <alice.i.cecile@gmail.com>
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants