Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Nov 26, 2021

Objective

Solution

  • If a 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 its GlobalTransform updated ever, amongst other issues. Detecting this is not cheap without a way to know the previous value of a removed Children 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.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 26, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Nov 27, 2021
Copy link
Member

@alice-i-cecile alice-i-cecile left a 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.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 6, 2022
@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 6, 2022

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:

// Another way to create a hierarchy is to add a Parent component to an entity,
// which would be added automatically to parents with other methods.
// Similarly, adding a Parent component will automatically add a Children component to the
// parent.
commands
.spawn_bundle(SpriteBundle {
transform: Transform {
translation: Vec3::new(-250.0, 0.0, 0.0),
scale: Vec3::splat(0.75),
..Default::default()
},
texture: texture.clone(),
sprite: Sprite {
color: Color::RED,
..Default::default()
},
..Default::default()
})
// Using the entity from the previous section as the parent:
.insert(Parent(parent));

That should be fixed by this PR

Copy link

@RobertoSnap RobertoSnap left a 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.

CleanShot 2022-01-07 at 10 02 39

@alice-i-cecile alice-i-cecile added this to the Bevy 0.6.1 milestone Jan 15, 2022
.push(entity);
}
Err(bevy_ecs::query::QueryEntityError::NoSuchEntity) => {
// Our parent does not exist, so we should no longer exist.
Copy link
Member

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?

Copy link
Member Author

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.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.6.1 milestone Feb 10, 2022
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR A-Hierarchy Parent-child entity hierarchies and removed A-ECS Entities, components, systems, and events labels Apr 25, 2022
@DJMcNab
Copy link
Member Author

DJMcNab commented Feb 16, 2023

This probably isn't relevant anymore

@DJMcNab DJMcNab closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic on unwrap when calling despawn_recursive()
4 participants