Fix double parenting. #1545
Fix double parenting. #1545
Conversation
remove some duplicate child insertion code fix duplicates this way
alice-i-cecile
added
C-Bug
|
Also, as a note, I changed the test to use Commands as I don't expect we can provide any sort of contract to people who are fiddling with the insides of the parent/child relationships. |
|
I've been working on some related changes (which got sidetracked due to my ECS V2 work). The idea is to make parenting completely transactional (children can only be added via World/Command builders or newly added HierarchyQuries), which means we don't need to check for duplicates, use a "hierarchy maintenance system", or maintain a https://github.com/cart/bevy/tree/hierarchy/crates/bevy_transform I think I want to wait and see how that experiment pans out. I'm curious to hear what you think! |
|
(it is very much a WIP at this point) |
|
That looks pretty cool, looks like it would solve the issue. I'm a fan of the solution for making parenting operations transactional and better change tracking will solve the transform side of things. This PR might be worth merging in the short term just to fix parenting in cases where people re-parent, I'm definitely going to be using it locally since it is broken otherwise. Depends on when you're planning on getting back to the transforms. I wouldn't mind helping out there a bit, but the Query code looks kind of like magic to me and I don't really understand how we are guaranteeing that we have singular write access to the two entities in the query. Thanks for taking a look, cart! |
|
As the carts hierarchy changes were ultimately sidelined and the 0.5 release is imminante, I think this PR should be updated, to include it in 0.5 . |
I agree: this is a good stop-gap fix for the release. |
alice-i-cecile
added
the
S-Ready-For-Final-Review
alice-i-cecile
removed
the
S-Ready-For-Final-Review
| @@ -45,31 +40,7 @@ pub fn parent_update_system( | |||
| } else { | |||
| commands.insert(entity, PreviousParent(parent.0)); | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Wont removing this break manual insertion of the Parent(Entity) component (or changing the parent value directly)? If so, that would be a pretty major breaking change.
There was a problem hiding this comment.
Yes, I think it would. But I would make the argument that mutably interacting with those is broken anyway. It only works for a narrow - if, perhaps, "typical" - set of use cases. Is this something we want to support if we cannot do it correctly?
For example, it is impossible to maintain a PastParent and correctly remove entities from Children collections if the user of this API is simply adding/altering the Parent component instead of calling commands.push_children(..). The old Parent would be gone and, if the entity was previously childed to another Parent, we would have no idea what list to remove it from. If we had #1655 it might be possible to support.
I do recognize that it would be a breaking change in that the use case that works correctly is taking two entities that haven't been part of parent/child relationships and just doing commands.insert(child, Parent(parent)).
There was a problem hiding this comment.
Yup theres no question that the whole system needs an overhaul. Currently it is possible (and easy) to mutably change Parent (either via commands or &mut Parent queries). Using that to change parents has probably been done (and regardless its possible and easy to do). Breaking that behavior silently feels about as problematic as the bug being fixed in this pr. If we can do it in a way that completely prevents manual Parent mutations (both via queries and direct world access), thats worth considering.
I'm also starting to consider moving forward on merging my hierarchy branch, or if that ends up not being ready / too disruptive for 0.5, maybe just leaving this behavior as-is for 0.5.
Let me know if you have any other suggestions.
There was a problem hiding this comment.
My preference would be to leave this behavior as-is and then do a full RFC on how parent-child stuff should work immediately after 0.5 lands. It's very central to UI, and @BoxyUwU's relations are also threatening to disrupt the space and need consideration.
There was a problem hiding this comment.
Cool, yeah there's several sets of changes in motion that will contribute to a better solution for this problem space. I don't have any good ideas about how to fix it in a way that works for everyone with what we have now, I think we need either Boxy's changes or something like #1655. I'm good closing this then, I'll keep running a fork until we get a proper solution.
|
Closing this for now in favor of a future overhaul. |
The only API to add a parent/child relationship between existing entities is through commands, there is no easy way to do it from `World`. Manually inserting the components is not completely possible since `PreviousParent` has no public constructor. This PR adds two methods to set entities as children of an `EntityMut`: `insert_children` and `push_children`. ~~The API is similar to the one on `Commands`, except that the parent is the `EntityMut`.~~ The API is the same as in #1703. However, the `Parent` and `Children` components are defined in `bevy_transform` which depends on `bevy_ecs`, while `EntityMut` is defined in `bevy_ecs`, so the methods are added to the `BuildWorldChildren` trait instead. If #1545 is merged this should be fixed too. I'm aware cart was experimenting with entity hierarchies, but unless it's a coming soon this PR would be useful to have meanwhile. Co-authored-by: Carter Anderson <[email protected]>
The PushChildren command was always setting the
ParentandPreviousParentto the same value which meant that a break (see ~line 31 ofparent_update_system) preventingPreviousParentremoval from executing would go off. This meant that if an entity was re-parented, it wasn't removed from its old parent'sChildrenlist.If this is resolved we are not immediately breaking out of
parent_update_systemeach loop. Thedebug_assert!begins going off - we've already performed the additions (see deleted code).This change does the following:
PushChildrento properly take theParentand set it as thePreviousParent, if it exists. Otherwise, it follows the previous behavior.Childrenupdate behavior inhierarchy_maintenance_system.I'm curious, also, is this the wrong direction? It seems like we might want to limit
PushChildrento solely update theParentandPreviousParentcomponents, leaving management ofChildrentohierarchy_maintenance_system. I did try this approach, but it did not function correctly and I wasn't making much progress debugging it. At the same time, re-parenting was completely broken andhierarchy_maintenance_systemwasn't doing much, so this seems at least like a step in the right direction if that's the goal.