ComponentHook based Parent/Children Management

[bushrat011899]

Objective

• Partially adresses Removing Parent / Children from the world and then reinserting them can break hierarchies #15575
• Fixes Use component lifecycle hooks to make Parent + Children hierarchy code simpler and more robust #12235
• Indirectly fixes Move methods on Children that mutate to EntityCommands #15270
• Contributes to Add ChildBuilder::spawn_batch #15869

Solution

• Made Parent and Child specific types of a One-to-Many relationship (relationship type Family)

Testing

• Ran CI locally.

Benchmarks

I ran the despawn_world_recursive benchmarks on my laptop and saw a 25% - 35% performance improvement moving to a ComponentHooks based relationship system in this PR.

despawn_world_recursive/10000_entities

I also used tracy to check that alien_cake_addict (my go-to example of a "real" game in Bevy) performed well.

alien_cake_addict tracy fame-time plot

While alien_cake_addict doesn't use the hierarchy extensively and explicitly (I think just for the score board at the end), I still think it's important to include.

---

Showcase

Users can now directly insert Parent onto child entities without the need for the child-builder methods.

let parent = world.spawn_empty();
let child = world.spawn(Parent::new(parent));

// Automatically called at every sync point
world.flush();

assert!(world.get::<Children>(parent).is_some());
assert!(world.get::<Parent>(child).is_some());

world.entity_mut(parent).remove::<Children>();

world.flush();

assert!(world.get::<Children>(parent).is_none());
assert!(world.get::<Parent>(child).is_none());

Recursive despawn is now also offered in a generalised form operating on any Component which implements VisitEntities.

let bus = world.spawn_empty();
let passenger = world.spawn(Passenger::new(bus));

commands.despawn_recursive_with_options::<Passengers>(bus, true);

Note that currently this recursive despawn can only traverse a single relationship type.

Migration Guide

HierarchyEvent

The HierarchyEvent type has been replaced. The following mapping from old to new is required:

• ChildAdded -> Added
• ChildRemoved -> Removed
• ChildMoved -> Removed and then Added in succession

DespawnRecursive / DespawnChildrenRecursive / despawn_with_children_recursive

• DespawnChildrenRecursive has been removed and replaced with a parameter, inclusive, on the existing DespawnRecursive. The old DespawnChildrenRecursive is identical in functionality to the new DespawnRecursive with the parameter inclusive: true.
• The function despawn_with_children_recursive has been removed to reduce the public API surface area. Instead, construct a DespawnRecursive command and use apply(...) from the Command trait to immediately invoke it on a World.
• DespawnRecursive's fields are now private. Use new and the appropriate with_x methods to change parameter defaults.
• DespawnRecursive now has a type parameter, C, which controls which relationship to follow recursively. Use Children to match the previous behaviour.

Notes

In previous iterations of this PR, the possibility of generalised relationships was included, including one-to-one and many-to-many. These were removed during the review process based on feedback from subject matter experts. They may be re-added once MVP relationships lands at a TBD date. See #3742 for further details.

[bushrat011899]

I want to explicitly call out that I believe this test was wrong before this PR, since it left an empty Children component on the grandparent entity.

[bushrat011899]

These associate and disassociate methods will always defer actions onto the command queue, even if it's possible to act immediately. The reason is to ensure all actions are properly ordered.

[ItsDoot]

Quick passover. Also, this PR indirectly addresses #15270

[iiYese]

since you can still access private components via component IDs

You still need a type to cast to which you can't do if the component itself it private. Unless there's something I'm missing. Either way it's significantly harder to accidentally do.

[nakedible]

My two cents on this:

This PR is like an aery upstream minus many features.

I agree 😉

• There is no reason for the components that store the edge data (OneToOne, ManyToOne, etc.) to have the exclusivity be expressed at a type level. This makes it so if you want to query for a relationship you need to know/provide this information as well which makes it awkward & verbose. Aery provides this configuration via a trait const it does not make it type level.

There is one important reason why, and that is of the interface for using these. A one-to-many relationship component derefs to Entity, where as as a many-to-one relationship component derefs to [Entity]. I believe it is nicer that the type has a different interface based on the exclusivity - it makes the difference explicit in code, and incorrect usage not compile, and improves error messages. Unwrapping single element arrays, or having two different APIs (there's only on Defer) is not what I want to do.

• The component types that store the edges are public. This means you can break invariants by modifying these components since they're held up by hooks which rely on archetypal changes. The Hosts & Targets components in aery are private for this reason. They can only be changed by commands & querying for edge information is done via an immutable QueryData struct.

While I agree it is safer, I personally think entity.insert(Parent::new(other)); feels very Bevy, where as entity.set::<Parent>(other); does not. It's easier to reason about what is happening, and it leverages the component model fully. The magic happens in the component hooks to also set up respective components on the other side. There isn't a new concept to learn (relations), or wondering how it is implemented and where the data is stored - it's just a component with entities. It's also clear how this interfaces with scenes, reflection, etc. in a readable manner. You can't write entity.set::<Parent>(other); in a ron file if you are not even told what the underlying storage component is and it's private, but you can add a ManyToOne<Family> component, even if the type alias wouldn't work.

It's also closer to what Parent and Children are currently, so it's a much less invasive change for the user in the beginning, regardless of what the final interface for relations is going to look like. And naming the two halves is important, as it's much easier to reason about Parent and Children than some Family relationship.

The downside here is of course that if these are exposed, then they become a part of the public API, so then it's harder to change the implementation of relations while keeping the same API. Private components would be much easier to change.

• I avoided upstreaming aery. Aery originally started out as a PR for bevy but when studying the subject more and specifically flecs's implementation I decided I didn't want it to be bevy's solution. The big difference between this approach (storing edges inside a component) & fragmenting relationships is the edge data is stored at an archtype level with fragmenting relationships. This means we can reuse many of our ECS tools for query operations as well as the optimizations that come with them. Not to say that it is an easy task but it is significantly easier to extend & optimize fragmenting relationships than it is to extend & optimize this. Aery was always meant to be deprecated when proper relationships were added to bevy. That is much harder for bevy to do if it is the one providing this tooling because that kind of breakage will bleed much deeper into the ecosystem. I think hierarchy as it currently is needs to be dropped & that is easier to do when it is still very limited in what it does. Extending it beyond that makes it harder. For that reason (especially given that mvp relationships is a lot closer now than it was when I first released aery) I think all of the generic relationship stuff in this PR should be dropped & it should focus on:

  • hierarchy QOL
  • making cleanup use hooks
  • making hierarchy invariants unbreakable by making Parent & Children private

While I do understand where you are coming from, I'm not sure I agree on all points. This is a larger discussion, but quickly a couple of points:

• Fragmenting vs. non-fragmenting is a performance optimization issue, not an API issue. This pull takes the API in the opposite direction from what I've seen of the relations work so far. If this API is exposed to users, it will make it significantly harder to migrate to another kind of API, as compared to just having Parent and Children to migrate. Then again, Bevy is no stranger to changes which require a lot of work to upgrade from those who use the API, so it's a judgement call. A decision should be made on the API as well, or the relationship API discussion should be separated from the relationship implementation discussion.
• Fragmenting relationships are totally useless for my use cases, and will just lead to one archetype per entity. Non-fragmenting relationships based on relationship type (exactly like it is here) is what I need, regardless of the API. Even Sanders suggested me to use non-fragmenting relationships for my needs, so getting those exactly right is also really important. I'd be really happy if non-fragmenting relationships would not be gated behind all the work required to make fragmenting relationships work – but I do understand that getting the API right for both of them is probably easier if fragmenting relationships are a bit further along.

[bushrat011899]

This PR is like an aery upstream minus many features.

Full transparency: I haven't personally used Aery. I'm coming at this from the perspective of someone who works with relational databases (MSSQL, etc.). Looking at Aery, it's definitely far more feature-complete than what I'm proposing here. What I will say is it's not my intention to match (nor exceed!) what Aery offers. And it is certainly not my intention to ship a half-baked dead-end feature that will only add to migration pains once MVP relations lands in Bevy.

In my opinion, the features that I am adding:

• One-to-One Directed and Undirected
• One-to-Many Directed
• Many-to-Many Directed and Undirected

Are all fundamental to any relations feature Bevy eventually ships (so much so that we already have Parent and Children in a worse state than what this PR provides). If MVP relations lands without support for these fundamental relationships I don't think it's really a viable MVP to be brutally honest. This PR doesn't include new syntax, Query tools, or any changes to Bevy's ECS model, so migration from this to true relations will be as simple as replacing a Component insertion with whatever syntax true relations will use.

* There is no reason for the components that store the edge data (`OneToOne`, `ManyToOne`, etc.) to have the exclusivity be expressed at a type level. This makes it so if you want to query for a relationship you need to know/provide this information as well which makes it awkward & verbose. Aery provides this configuration via a trait const it does not make it type level.

It is possible to refactor my current PR to make the relationships have runtime validation of these invariants instead of compile-time, but I don't think I understand why you would want this. If you don't want these invariants managed, you can use ManyToMany and ignore that the other Components even exist. This PR is quite simplistic in that you can use a ManyToMany as a one-to-one relationship, or as a one-to-many, if you really want to. All it does is gives the user the option of having the invariant managed at a type level if they wish to.

Looking at Parent and Children as an example, we already desire that invariant be codified in the type system, since it's required by many areas of Bevy that every Entity have at-most one Parent. I do think having a more loosely managed relationship type is important, but that role is provided by ManyToMany already.

* The component types that store the edges are public. This means you can break invariants by modifying these components since they're held up by hooks which rely on archetypal changes. The `Hosts` & `Targets` components in aery are private for this reason. They can only be changed by commands & querying for edge information is done via an immutable `QueryData` struct.

I do agree with this idea, which is why all mutable methods provided for these edge types require owned (self) access to the component. This doesn't prevent users swap-ing data, but this is an existing issue improved by this PR (removing Parent and Children is a trivial example of breaking these invariants that this PR resolves). This PR makes all trivial Component operations invariant-preserving.

• Inserting a relationship onto an Entity will always create the appropriate partner component on the foreign Entity
• Removing will always dispose of dangling components
• Replacing will always update old and new entities to the new arrangement
• There are no mutable methods on any relationship components, you must use an invariant preserving archetypical operation (insert, remove, replace), or a method like core::mem::swap.

If the above guarantees are insufficient, we can adopt the techniques you've outlined (private components, QueryData struct, etc.) as well, they are not mutually exclusive to the techniques used in this PR (as can be seen by leaving ChildBuilder, etc. available).

* I avoided upstreaming aery. Aery originally started out as a PR for bevy but when studying the subject more and specifically flecs's implementation I decided I didn't want it to be bevy's solution. The big difference between this approach (storing edges inside a component) & fragmenting relationships is the edge data is stored at an _archtype level_ with fragmenting relationships. This means we can reuse many of our ECS tools for query operations as well as the optimizations that come with them.

Not a major point for or against, but I think it's self-evident that this PR does reuse existing ECS tooling, since it is exclusively confined to the bevy_hierarchy crate with no changes required in bevy_ecs. I do think fragmenting relationships is a highly important feature for Bevy to ship, but non-fragmenting ones are also an important feature to support.

Not to say that it is an easy task but it is significantly easier to extend & optimize fragmenting relationships than it is to extend & optimize this. Aery was always meant to be deprecated when proper relationships were added to bevy. That is much harder for bevy to do if it is the one providing this tooling because that kind of breakage will bleed much deeper into the ecosystem.

I personally don't see how this PR creates a migration issue any worse than any other PR added to Bevy; it introduces 4 components which should be trivially replaceable once Relations lands. Further, they don't need to be removed to land relations; they can be marked as depreciated and left as-is for as long as Bevy supports Components and ComponentHooks.

I think hierarchy as it currently is needs to be dropped & that is easier to do when it is still very limited in what it does. Extending it beyond that makes it harder.

Again, I'm only introducing 4 components which could be moved to wherever the ChildBuilder methods get moved to. Alternatively, since these components exclusively rely on the public API of bevy_ecs, the worst-case-scenario migration guide would be to provide those components to users to include in their project themselves. Definitely not an option I would champion, but absolutely viable.

For that reason (especially given that mvp relationships is a lot closer now than it was when I first released aery) I think all of the generic relationship stuff in this PR should be dropped

I personally disagree with this conclusion. I think at most, we should put this behind an experimental feature flag similar to GhostNodes in #15961. Again, I believe these components offer a very simple API which would be very straightforward to migrate from with the real relations MVP.

it should focus on:

  * hierarchy QOL
  * making cleanup use hooks

I agree with these points, and I do believe this PR addresses these concerns in ergonomics, reliability, and performance.

  * making hierarchy invariants unbreakable by making `Parent` & `Children` private

Addressed above. I'm not against this, but as others have mentioned, this creates issues for BSN and ron, since you don't have a named component to use in those formats. I'm honestly not sure how we can integrate relations into those foundational parts of Bevy without named components, but I am not a subject matter expert like yourself so this may be something already considered and I am just unaware!

I really want to stress that I greatly appreciate you taking the time to review this PR. I disagree with some of your points but I am absolutely willing to change this PR if it's the general consensus that what you're proposing is the correct direction for Bevy. I am pushing back because I have seen several instances of people requesting this functionality as-presented, and I genuinely believe it is a positive direction to take Bevy, even if it's only for one or two releases.

[iiYese]

@nakedible

There is one important reason why, and that is of the interface for using these. A one-to-many relationship component derefs to Entity, where as as a many-to-one relationship component derefs to [Entity]. I believe it is nicer that the type has a different interface based on the exclusivity - it makes the difference explicit in code, and incorrect usage not compile, and improves error messages. Unwrapping single element arrays, or having two different APIs (there's only on Defer) is not what I want to do.

The correctness here is actually worse because you can have both OneToMany<R> & OneToOne<R> on the same entity. This kind of API design has been misguiding bevy for a long time. Things being type level does not mean magically more correct. And besides that it makes the ergonomics terrible. You should be able to query for a relationship without knowing its edge rules.

While I agree it is safer, I personally think entity.insert(Parent::new(other)); feels very Bevy, where as entity.set::(other); does not...

You can have a bundle API with a hook instead of a command if you really want. Either way the edge types should be private. Sacrificing upholding edge invariants for a slightly different method call is weird especially looking at your previous comment. The energy spent on upholding correctness is misplaced.

Fragmenting vs. non-fragmenting is a performance optimization issue

No it's not. Archetype fragmentation is a different tool entirely. You cannot do many query operations when edge information is not exposed at an archetype level.

Fragmenting relationships are totally useless for my use cases, and will just lead to one archetype per entity

No it won't. You're either imagining only top down edge scenarios or you have the worst case scenario. If you have a relation like Child then you create a lot of needless fragmentation. If you instead have a relation like Parent or flecs's ChildOf you get perfect fragmentation. All children of an entity end up in the same table which gives you cache friendly dense iteration when traversing hierarchies.

If you instead have the actual worst case scenario and lots just of relations this is already the case in mainstream OO engines which perform more than fine.

Non-fragmenting relationships aren't mutually exclusive to fragmenting relationships either but they're best not implemented like this PR or Aery. The way flecs does them iirc is with sparse components.

[iiYese]

@bushrat011899

I don't think it's really a viable MVP to be brutally honest

I think you don't understand what an MVP is. An MVP is not complete functionality it is a minimal viable product. MVP relations will probably just be stuff like Parent to begin with. Enough to replace the current hierarchy. The tools extend them to do things like having x-to-x rules & cleanup are already there (hooks & observers) & shouldn't be a complicated follow up.

All it does is gives the user the option of having the invariant managed at a type level if they wish to.

Consistency is more important here than options. Why should users have to care about whether or not the information is erased & if it's not need to know that information every time they want to operate on edges? For what benefit? Being at a type level is not a benefit it is the option.

Looking at Parent and Children as an example, we already desire that invariant be codified in the type system, since it's required by many areas of Bevy that every Entity have at-most one Parent

That is not the same. That is different sides of an edge not entirely different relationship types.

This PR makes all trivial Component operations invariant-preserving

The scenario that isn't covered was already mentioned

let e = world.entity(e0).get::<Parent>().unwrap().get();
*world.entity_mut(e1).get_mut::<Parent>().unwrap() = Parent::new(e);

Not a major point for or against, but I think it's self-evident that this PR does reuse existing ECS tooling...

That's not what I'm referring to & as mentioned in my previous response to @nakedible non-fragmenting relationships are best not implemented like this. You will end up with entirely different APIs to do queries for fragmenting & non-fragmenting relations.

I personally don't see how this PR creates a migration issue any worse than any other PR added to Bevy; it introduces 4 components which should be trivially replaceable once Relations lands.

It's not just replacing the component types it's also replacing all of the accompanying code doing any kind of operations on entities which will look very different. You will also have people relying on the fact that these are non-fragmenting which is just a complete break instead of a migration.

[bushrat011899]

Looking at Parent and Children as an example, we already desire that invariant be codified in the type system, since it's required by many areas of Bevy that every Entity have at-most one Parent

That is not the same. That is different sides of an edge not entirely different relationship types.

Sorry I don't think I made my point very clear here. What I was trying to convey is that a Bevy Family relationship (Parent and Children) already has a one-to-many invariant codified into the type system. Parent must refer to a single entity, it cannot under any circumstances refer to multiple entities. Building relationships without support for those kinds of type-level constraints degrades the user experience and sacrifices correctness. Requiring a user to write:

// not valid code, representative only
let [parent] = e0.get::<Parent>() else { unreachable!() }; 

Is a terrible user experience. As such, the MVP for relationships must support defining a one-to-many relationship to be able to replace the current Parent and Children components. From personal experience, I can also say that implementing a one-to-one relationship is definitely simpler, so would likely be included in the MVP anyway.

As for including OneToOne<R> and OneToMany<R> on the same entity, I personally don't consider that a correctness issue, because the parameter R is a marker used in a relationship, the relationship itself is the component and the marker combined. That's why in all the examples I've provided I've included type aliases for those components with their markers.

Knowing that a relationship is one-to-one, one-to-many, etc. is also a perfectly reasonable assumption to make of the user in my opinion. If the user doesn't know what the relationship is, what could they meaningfully write with them? Again, if they don't care, a many-to-many is always there.

[iiYese]

Bevy Family relationship (Parent and Children) already has a one-to-many invariant codified into the type system. Parent must refer to a single entity, it cannot under any circumstances refer to multiple entities.

That's more to do with the way hierarchy is implemented. When you have components for edges on entities ofcourse the entities mean something different depending on if you're at the base or head of an edge. With proper relationships we will only have Parent. Children will be gone. The edge rules will still be "codified" but it will be a part of the relation itself. The only time you should be stating a relation type with its edge rules is when you're defining it. It's implicitly apart of the relation form there. It makes no sense for users to need to restate this definition every time they query for a relationship.

As for including OneToOne and OneToMany on the same entity, I personally don't consider that a correctness issue

I do. This is just erroneous & I would block on this if I had the power. When would it ever make sense to have both? It is not useful to just provide more ways to do things. You need to provide ways to easily do things that make sense that people actually want to do.

That's why in all the examples I've provided I've included type aliases for those components with their markers. If the user doesn't know what the relationship is, what could they meaningfully write with them?

You're contradicting yourself here. It can't both be that "they should always know this information" & "but I erased the information they should always know in every example". It's not just needing to know it's also having to restate it every time. Redundant repetition is a big source of errors in programming.

[nakedible]

@iiYese I'll answer to the rest a bit later, but:

No it's not. Archetype fragmentation is a different tool entirely. You cannot do many query operations when edge information is not exposed at an archetype level.

Can you list the query operations that require and archetype based relations implementation, as opposed to component based?

[iiYese]

Can you list the query operations that require and archetype based relations

Some of the important ones off the top of my head:

• Wildcard queries (*, e)
• Named wildcard queries &X, (Parent, $p), Ship($p) (get the Xs from all entities that have a parent that is a ship).
• Nested joins * -R-> * -R-> *
• Efficient up traversal
• Efficient sibling queries

[bushrat011899]

Based on pushback on the functionality provided here I have removed all new functionality from this PR with the exception of hook-based management of Parent and Children, the generalisation of DespawnRecursive, and the ability to insert Parent and Children onto an entity directly. If someone else wishes to ship this functionality they are more than welcome to add them back in through a follow-up PR. I apologise to anyone who was excited for this functionality to come in this PR. I encourage you to look towards #3742 for true relationships coming in the not-too-distant future.

[nakedible]

I'm sad that this is the outcome, but on the other hand, I do believe it's the right outcome for this pull, so that the non-controversial parts are not gated behind the controversial parts.

However, I would suggest that there is a follow-up pull which would reinstate the removed generic relations functionality behind an experimental flag. Then we can discuss those in isolation without combining it with the fixes to the current Parent/Children functionality.

Also, I don't think this pull is the right place to discuss @iiYese 's objections – so I'll take to answering to those elsewhere.

[iiYese]

So a lot of the relationship stuff is still there which becomes unused complexity that has to be maintained. The component types are still public which is the last thing that allows breaking invariants. I would suggest:

• Getting rid of all the remaining relationship stuff. Traits, generics etc. turning OneToMany<R> into ChildrenInner with an accompanying ParentInner that are private.
• Making immutable QueryData structs Children & Parent for easy migration.

This PR got quite big & is trying to address multiple issues at once. Before this discussion it was also trying to shoehorn new features in with the PR that were unrelated to the issues. In future keep the PR small & focused on a single issue & make different PRs for new features. Small PRs are easier to review & merge & will save you from these kinds of head aches.

[bushrat011899]

Closing as I don't see any reason to continue changing this PR. Based on feedback, a more generally accepted solution would be to take the associate and disassociate hooks from this PR, add them to the existing Parentand Children components, rename them, privatise them, and create QueryData structs with the original names to allow read-only access. You'll still be able to break invariants (e.g., by using the ComponentId), but the surface area would be reduced.

I personally disagree with this approach, so I would invite someone else to take over from here who has a resolute belief in this approach. Thanks again to everyone who reviewed this PR, and I apologise I couldn't see it through.

[nakedible]

Okay, now it became the saddest outcome 😭

[NthTensor]

Missed this initially. Disappointed this ended up getting closed. I'd like to float this by the ECS SME team. @alice-i-cecile @JoJoJet @maniwani do yall think you could give this some consideration after the 0.15 release?

[alice-i-cecile]

To summarize:

• hook based parent / child is good, which can be used to eliminate despawn_recursive
• move from current to ideal design with no detours
• smaller PRs with a clearer plan
• read only components are a generally useful tool for further ensuring hierarchy validity

My broad position is that I'm fine with a very slow initial relations implementation, but we should try to target the ideal correctness/API design to avoid confusion. Moving incrementally towards that is welcome, and prototyping with just Parent/Children is going to be easiest :)