-
Notifications
You must be signed in to change notification settings - Fork 65
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
Consistent Hierarchies #53
Conversation
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.
A fair bit of editing feedback, along with some technical critiques.
Major thoughts:
- This should explicitly call out the creation of a
bevy_hierarchy
crate, and the ability to use a single hierarchy for distinct properties. - I don't think there's enough discussion of property inheritance here. How do we handle inheriting seperate properties? How do we ensure those values stay in sync? Can we opt-out of inheriting properties? This was listed as one of the motivations, but the proposals don't address it.
- We really need a half-decent benchmark suite for this: these decisions are very performance driven and discussing O(n) complexity for single digit n doesn't really make sense.
- I'm strongly in favor of incrementally improving the hierarchy situation ASAP: there are a lot of subtle, painful bugs, and it's seriously blocking UI work.
- I rather like the "commands-only" approach to mutation: this is much safer than the existing design.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
While I like the commands-only idea, the issue with it is it can't be fully enforced since despawning entities can easily break this system. The hooks mentioned in bevyengine/bevy#3742 is a possible solution to that. |
Can't we just use Although you can of course break this in an exclusive system still :/ |
It's a start, I guess. One of the main drawbacks listed is that bevy_ecs is not aware of the structure, and will require something like hooks to fully polish out. However, the merits of this current design still stand, even if not a complete 100% solution in it's current form. |
Made an initial draft implementation that addresses a number of the issues found here, listed under one of the current alternatives in the RFC. This seems to be the absolute minimum changes to get some of the benefits from this RFC. With some of the optimizations being made to the transform propagation system (see bevyengine/bevy#4180), it's probably best to minimize I'll update the RFC itself with these changes soon. |
It was not super clear to me whether this rfc is propsing to make Overall I like the changes here towards making the hierarchy more robust by having everything be immutable and requiring changes to be made via commands, this is stuff we will have to do even in a future with hooks/relations :) The (I found the title of the RFC somewhat confusing as I would have expected a "generalized hierarchy" feature to involve something like hooks/relations/ |
Updated the title/filename and clarified which commands we will be using or adding. |
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes #2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in #4141 (comment) and #2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in #2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>
On a purely use-facing perspective, the proposal is solid. I think it solves a lot of issues. And doesn't introduce any meaningful limitations. As a heavy user of the hierarchy system, it seems fine. I'll have a go with a multiple-hierarchy system RFC similar to the one introduced here. I got multiple use cases where this could be very useful. |
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 a short term solution to our hierarchy woes, this seems like a reasonable / scoped approach that we could probably land before the next release.
system and the consistency issues that come with it's design, which it delivers | ||
on. | ||
|
||
### Alternative: Hierarchy as a Resource |
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.
The biggest downside here is definitely scene management. We'd need to either:
- Add the concept of "resource merging". In practice I think this could be reasonably cheap, especially if we assume that scenes have a single root entity (and I'm relatively convinced at this point that they should).
- Add a secondary component-based hierarchy representation for scenes, which then gets "converted" into a runtime centralized Hierarchy resource when the scene is spawned.
Both of these are fundamental changes to the mental model that require serious thought.
Iteration requires mutative access to the resource, which can limit parallelism of systems traversing the hierarchy. The alternative requires a dedicated O(n) sweep over the hierarchy whenever it's mutated.
We don't necessarily need to couple these operations. I get that we're deferring topological sorts to avoid really expensive insertion (we don't want to be constantly shifting items around on each insert). But this is an optimization issue, not a correctness issue. We could choose to do it once per frame with traverse_hierarchy_and_toposort(&mut self)
(maybe in the transform propagation) and encourage users to just do traverse_hierarchy(&self)
unless they really want to toposort earlier. On average, iterations would be approximately topo-sorted. Or we could have a "dirty" bit and just topo sort on frames where it is dirty.
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.
Updated the RFC to reflect some of these + some pros/cons I initially missed. I actually like this a lot more now that it's clear that this even further addresses the archetype fragmentation and makes hierarchy edits on par with a SparseSet lookup.
If we know the roots of a scene, assuming we don't need to ensure it's sorted immediately afterwards and the scene's hierarchy resource is already sorted, copy-pasting the mapped contents of a scene hierarchy at the end of the "global hierarchy" then updating the spawned scene roots to their new parent(s) should be O(n)
in the number of entities in the scene, and should be very fast. It can then rely on the frame-to-frame topological sort to optimize out the kinks.
parallelism of systems traversing the hierarchy. The alternative requires a | ||
dedicated O(n) sweep over the hierarchy whenever it's mutated. | ||
|
||
### Alternative: Linked List Hierarchy |
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.
Linked list Hierarchy
components are growing on me. Immediate, always-in-sync O(1) hierarchy updates are compelling. I've fixated on needing Without<Parent>
queries in the past, but thats only important if we allow more than one root and/or treat "hierarchy membership / rootness" as a direct result of Hierarchy data. If we force explicit roots, this becomes a non-issue.
This creates a number of problems though:
- How do we choose roots? Do we just spawn a single "canonical" root entity on our users' behalf? Do users denote a
Root
component on entities? Do we just ignore "hierarchy orphans" for things like transform propagation? Do we detectparent == None
cases for entities withoutRoot
markers and warn? - Immediate hierarchy updates only work when both parent and child entities already have Hierarchy components. We would still need to rely on deferred parenting commands for the initial spawn of entities, or for entities that don't yet have the hierarchy component.
- In light of (2), to actually make "immediate hierarchy updates" possible and worthwhile in practice, we would need to ensure that all common entities/bundles/prefabs intended to be hierarchy members have Hierarchy when they are spawned. This would mean a lot more "unnecessary" / empty hierarchy data in the ECS and more complicated bundles. Users defining their own bundles would need to add Hierarchy to get the "immediate" benefits. This feels brittle / in some cases users might not be sure if the parent is "ready", so they might need to try both (or just default to commands).
- We would need to measure the extra
Query::get
child traversal perf relative to pre-computed child list iteration and decide if it is "worth it".
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.
After doing some work to parallelize transform propagation, I like this approach a lot less. Responses inline.
Linked list Hierarchy components are growing on me. Immediate, always-in-sync O(1) hierarchy updates are compelling. I've fixated on needing Without queries in the past, but thats only important if we allow more than one root and/or treat "hierarchy membership / rootness" as a direct result of Hierarchy data. If we force explicit roots, this becomes a non-issue.
The immediate/strong consistency of this model is definitely nice, but it comes at a hefty cost to iteration/reading. With the costs of Query::get
, the performance of the more common case of needing to do a read-only traversal of the hierarchy probably should take precedence over immediate consistency, so long as there is a consistent view of the hierarchy, which the commands provide.
How do we choose roots? Do we just spawn a single "canonical" root entity on our users' behalf? Do users denote a Root component on entities? Do we just ignore "hierarchy orphans" for things like transform propagation? Do we detect parent == None cases for entities without Root markers and warn?
A SparseSet marker component, maintained by the commands proposed here could work. We're already incurring heavy random read costs, so archetypal iteration at the root level likely isn't going to be much of a perf impact here.
I'd strongly recommend against a single root. Any propagation system we have now is parallelizing at the root level, and is reliant on efficiently identifying them and going wide splitting on the individual trees in the forest.
Other than the marker component, the only way I can think of is a resource root, which introduces more consistency problems.
Immediate hierarchy updates only work when both parent and child entities already have Hierarchy components. We would still need to rely on deferred parenting commands for the initial spawn of entities, or for entities that don't yet have the hierarchy component.
The design here still calls for a command-based approach, so iteration is read-only. The O(1)
nature just changes the costs of running those commands. Within those commands, we can update the state of the new child/sibling/parents by adding the Hierarchy
component.
In light of (2), to actually make "immediate hierarchy updates" possible and worthwhile in practice, we would need to ensure that all common entities/bundles/prefabs intended to be hierarchy members have Hierarchy when they are spawned. This would mean a lot more "unnecessary" / empty hierarchy data in the ECS and more complicated bundles. Users defining their own bundles would need to add Hierarchy to get the "immediate" benefits. This feels brittle / in some cases users might not be sure if the parent is "ready", so they might need to try both (or just default to commands).
Agreed here. This seems like a major downside. I'll add this to the RFC.
We would need to measure the extra Query::get child traversal perf relative to pre-computed child list iteration and decide if it is "worth it".
This is the real kicker. Query::get
has a lot of overhead when you're using it to iterate over a large portion of the World
. The tight cache-locality of Children
simplifies this by a long shot.
rfcs/53-consistent-hierarchy.md
Outdated
expensive to run, so the documentation for them should reflect this and | ||
discourage misuing it. | ||
|
||
### HierarchyEvents |
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 think its worth pointing out that in practice, this will increase the memory usage of the hierarchy system to be something like 2 * size_of::<Entity>() * MAX_HIERARCHY_CHANGES_ACROSS_TWO_FRAMES
. Given that hierarchical spawning for scenes will be pretty burst-ey, in practice this will be a per-hierarchical-entity 2 * size_of::<Entity>()
price unless we add garbage collection to Events.
Although that should be weighed against the cost of PreviousParent(Entity)
, which includes both additional memory footprint and (sometimes) additional runtime costs for archetype moves.
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.
PreviousParent
is a size_of::<Entity>()
memory cost and arbitrary archetype moves. Given that (Global)Transform
is 48 bytes, and one Entity
is 8, HierarchyEvent
is 28 bytes. Just in memory write costs alone, we'd need to write ~3.5x (28 bytes vs 96) the events to compare an archetype move for your typical transform hierarchy change, and that's not counting the archetype/table move overhead, the other components being moved, and the additional archetype fragmentation from the two-fold moves. If we make PreviousParent
a SparseSet component, we could reduce this down to 16 bytes (8 for the data, and 8 for the change detection ticks); however any query for them is going to use archetypal iteration. IMO, it's much more natural to rely on events here.
Building automatic event garbage collection seems perfectly workable given that events already track readers.
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.
Garbage collection for events is also pretty trivial to implement: bevyengine/bevy#4832
I'm down to approve and merge this once all outstanding comments have been resolved. The event conversation is the most important to sort out. |
I'm not seeing these changes. Did you push them? |
Looks like I didn't. Sorry about that! Should be up now. |
# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes bevyengine#2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>
## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in #3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.
## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.
## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.
## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.
# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes bevyengine#2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes #2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in bevyengine/bevy#4141 (comment) and bevyengine/bevy#2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in #2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>
RENDERED
This is a summary of the discussion in bevyengine/bevy#4141 as a RFC. The primary goal here is to resolve the ergonomics and consistency issues of dealing with hierarchies in Bevy, with a secondary focus on providing performant access and mutation patterns for it.