-
-
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
Slightly nicer handling of parent/child failure modes #3199
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.
The limitations are unfortunate, but I think this is an improvement over the previous behavior.
Code also LGTM.
This seems to be a 'duplicate' of #2410; that is, #2410 should also have fixed #3195. However, #2410 doesn't fix @RobertoSnap's new issue, which occured problem because of this example: bevy/examples/ecs/hierarchy.rs Lines 42 to 61 in 44f370a
That should be fixed by this PR |
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.
Ran this PR with latest changes on local repo and local reproduction repo.
The Components got despawned as expected.
Here is reproduction repo which now runs successfully. UI gets despawned.
.push(entity); | ||
} | ||
Err(bevy_ecs::query::QueryEntityError::NoSuchEntity) => { | ||
// Our parent does not exist, so we should no longer exist. |
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.
Is this actually true? A parent no longer existing doesn't necessarily imply that the descendants should be despawned? Maybe the intent was to move the children to the top of the 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.
Like I say in the PR body, we cannot reasonably check whether despawn
or despawn_recursive
was called on the parent entity.
In my mind, a parent with all its children is an atomic unit, so it's much more likely that the user wants to despawn this child
But in general, this messiness is a consequence of how messy our entire parent/child system is; that is, I doubt this will be the final state. However, this patch makes the current state nicer, fixing an issue which several users ran into.
This probably isn't relevant anymore |
Objective
Solution
Parent
's parent doesn't exist, despawn the entity containing it.I think that for the case of scene spawning, we should probably have handling for when the parent of the scene doesn't exist and stop the spawning. However, this change also fixes that, with significantly less work required (as the
SceneSpawner
data structures are sub-optimal for adding that check).Even with this fix, a 'general' case of this problem still remains. For example, if 'despawn' is called instead of
despawn_recursive
, we end up with a 'floating' entity. This entity would not have itsGlobalTransform
updated ever, amongst other issues. Detecting this is not cheap without a way to know the previous value of a removedChildren
component. (🎟️ ?) However, this version is still a better state of affairs.In theory, we could have different behaviour here if the parent was despawned explicitly non-recursively. That is, one could argue that panicking in that case would be 'better'. However, we don't store that information, and cannot reasonably do so; taking the most 'optimistic' approach here is probably best.