-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Hierarchy optimized systems and storage #4141
Comments
Yes: this is one of the core use cases. However, as you see, there's a huge level of design and benchmarking required. This may be a good starting point to allow us to build-out a generalizable solution.
IMO this is required. |
Alternative proposal: Hierarchy as a resourceRather than storing Benefits
Drawbacks
Limitations
|
One downsidde to this is that this blocks any concurrent editing of the hierarchy, at least not without a However, mutation, beyond the local scope is probably not something we should be optimizing for here, since the heaviest operations involve traversing and reading the hierarchy. It's probably best to think of the ways we want to be querying this structure what other non-mutative operations we want on it.
Some additional observations:
|
I will note that this is currently the case with commands, but this is a limitation.
Yep: however by storing e.g. a
This is correct for some but not all of the potential designs :) The index-backed solutions can be custom-optimized for these situations. |
@alice-i-cecile another big drawback to "hierarchy as a resource" is that it is not particularly scene friendly. It assumes (1) that we support loading resources from scenes and (2) that each time we load a scene's instance of the hierarchy resource, we "merge" that with the global version. I do think a Hierarchy / left+right pointer based approach is worth considering (note that I read the bitsquid article linked above way back when I was first deciding how hierarchy would work in Bevy). The ability to update the hierarchy "transactionally" is really nice (although this could be accomplished with split components, with the right scoping). I also really like the O(1) entity deparenting. The main reasons I opted for split components vs "Hierarchy left+right":
However during this early decision making process, I very clearly undervalued "always consistent" hierarchies. Given how central hierarchy can be to game logic, being able to trust it at a given point in time (and not just after the Hierarchy sync) is pretty important. The "consistency problem" must be solved, but I do think we lose a lot of valuable functionality by moving to the "Hierarchy left/right/parent" approach. |
As suggested by the bitsquid article, which has been referenced several times in this thread already, you can keep what is effectively a |
Also for "hierarchy as a resource", it creates a "garbage collection" problem. Every time an entity is despawned, we now need to check if it exists in the hierarchy. You can't know ahead of time, so you need to check every despawned entity. |
Is this something we can't deal with using either Resources that wrap HashSet (i.e.
Wouldn't a inverted filter work here too (i.e.
Can you explain this more? Read-only queries should be easy enough to work with without clashes. Overall, I don't think the underlying representation here is something we should encourage users to directly query, much like ECS itself, but rather provide solid wrappers and abstractions (i.e. custom SystemParams, dedicated hierarchical query types) that provide the ergonomics needed. |
FYI, #3580 does this. |
IIRC that PR doesn't support merging resources with one in a scene. We'd likely need a specialized solution that handles merging and unmerging the resource upon scene load and unload. Though admittedly, so would any scene load/unload that directly handles Entity references in components. |
We could add a |
Instead of #[derive(Component)]
pub struct Parent(Entity);
#[derive(Component)]
pub struct FirstChild(Entity);
#[derive(Component)]
pub struct PrevSibling(Entity);
#[derive(Component)]
pub struct NextSibling(Entity); Pros
Cons
|
The archetype fragmentation could be addressed down the line with archetype invariants. In practice, I'm not sure that users will be manually building these things, and we'll only get fragmentation if we're inserting this one component at a time. |
What if we have both the I'd vote against splitting |
One more note: edited the description to include the previous attempt to split the hierarchy from bevy_transform. Not sure if it's doable given these new proposals. |
The way I interpreted #2789 was that the transform systems are too special since they involve two components ( I did like the suggestion in there to parameterize #[derive(Component)]
pub struct Hierarchy<T> {
parent: Option<Entity>,
first_child: Option<Entity>, // should there be a `last_child` field as well?
prev_sibling: Option<Entity>,
next_sibling: Option<Entity>,
_phantom: PhantomData<T>,
}
#[derive(Component)]
pub struct IsParent<T>(PhantomData<T>);
#[derive(Component)]
pub struct IsChild<T>(PhantomData<T>); |
Arguably, the same parenting structure for transforms should be used for the others, since it's the logical expectation here. Not too keen on the idea of multiple overlapping and conflicting hierarchies, particularly when it comes to showing it to users in an editor. We could split it out so that the hierarchy exists separately from a "f32-based Transform system operating in Euclidean space" that bevy_transform currently provides. It would allow others to implement their own transform systems like a |
Honestly, I would parameterize the types "just for the heck of it" since it would help clarify how the components are used. That way, coupling other stuff to the transform hierarchy becomes an intentional choice, instead of users just accidentally using the same components not knowing they're for transforms (bike-shedding, but renaming the types to something like And there are clearly hierarchies people want to express that have nothing to do with transforms. |
This was @cart's take: we should try to have a single hierarchy that we use for everything (if feasible). I'm not sure I personally agree, but I'm willing to explore the direction the design takes. However, I think it's critical to support the inheritance of other properties (hopefully even user-defined properties), and to be able to control what is and isn't inherited (translation? scale? visibility?) at an entity-level. |
I think I would rather just provide the tools for managing, traversing, and querying the hierarchy. Operations like dirtying, propagation, and inheritance may be too use case specific to make generic systems for. |
Getting mixed signals here. Is this issue about hierarchies in general or specifically the transform hierarchy (since you mentioned animation, visibility, and working in an editor)? A tree of linked nodes would be useful in general (in the same way ECS relations would be), but I see no reason to discuss splitting Anyway, I was just saying that people misuse |
Agreed: we should either do this, or allow people to opt-out of this coupling. |
Not fully caught up yet: but I'd like to first state that hierarchy is already decoupled from Transforms. Transforms just choose to use it for transform propagation. The only real coupling is "the crate these things live in". Opting out does seem like a reasonable feature request, but I'd want real motivating use cases before adding that complexity. I also agree that how Transforms relate to the "generic hierarchy" should be a separate conversation. |
Whipped up a summary of the discussions here as an RFC: bevyengine/rfcs/pull/53. Please take a look! |
# 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>
# 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>
What I found inconvenient about the current system is that when I store a scene, I have to store |
# 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>
What problem does this solve or what need does it fill?
Several common entity hierarchy based systems have started to crop up in multiple upcoming and current focus areas.
Transform
propagation is reliant on a depth-first traversal of theTransform
hierarchy.Visibility
affect children entities #3874 -ComputedVisbility
should be inherited from parents/ancestors.root/hips/chest/shoulder_L
is a top-down hierarchy query based on theName
components of each entity.There are also several existing cases where the current
Transform
based hierarchy is either buggy or unwieldy to manage the need to wait for the consistency of the system to settle.GlobalTransform
not being updated #3329GlobalTransform
consistency whenParent
is spawned withoutChildren
#3340Other issues brought up about the hierarchy:
What solution would you like?
This will require additional design but it'd be best to have a hierarchy structure that is:
What alternative(s) have you considered?
Using the current
Parent
andChildren
components as is.Possible Ideas
One of the potential options that doesn't rock the boat that hard is using a tree-based linked list approach.
This has a few benefits/drawbacks over the current
Parent
/Children
component approach:Parent
andChildren
as well as let users query for the siblingsof a given
Entity
without a reference to the parent.HierarchyQuery<T>::iter_all_in_descendants
orHierarchyQuery<T>::iter_all_in_ancestors
)Query<&mut HierarchicalRelations>
.Query::get_multiple
or something similar can be used to update multiple relations at the same time to "atomically" update each of them. Removes the reliance on a secondary system to keepChildren
up to date.Children
andParent
, assuming we haveOption<Entity>
niched.SmallVec<[Entity; 8]>
is 64 bytes in ECS storage, and more if it overflows, andParent
is 8 bytes. This single component is only 32 bytes.Children
solution are both more or less useless without a secondaryQuery::get
to fetch corresponding components, this might not be a big loss.This is an approach seen in bitsquid http://bitsquid.blogspot.com/2014/10/building-data-oriented-entity-system.html, where dedicated storage for the hierarchy was also used.
Additional context
The text was updated successfully, but these errors were encountered: