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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add despawn_children #2903

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Add despawn_children #2903

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23334eb
Add despawn_children_recursive
TheRawMeatball Oct 2, 2021
cb518cf
rename to despawn_children
TheRawMeatball Oct 5, 2021
cd5fe43
Tweak docs and names
TheRawMeatball Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions crates/bevy_transform/src/hierarchy/hierarchy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ pub struct DespawnRecursive {
entity: Entity,
}

#[derive(Debug)]
pub struct DespawnChildrenRecursive {
entity: Entity,
}

pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) {
// first, make the entity's own parent forget about it
if let Some(parent) = world.get::<Parent>(entity).map(|parent| parent.0) {
Expand All @@ -36,15 +41,32 @@ fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity) {
}
}

fn despawn_children(world: &mut World, entity: Entity) {
if let Some(mut children) = world.get_mut::<Children>(entity) {
for e in std::mem::take(&mut children.0) {
despawn_with_children_recursive_inner(world, e);
}
}
}

impl Command for DespawnRecursive {
fn write(self, world: &mut World) {
despawn_with_children_recursive(world, self.entity);
}
}

impl Command for DespawnChildrenRecursive {
fn write(self, world: &mut World) {
despawn_children(world, self.entity);
}
}

pub trait DespawnRecursiveExt {
/// Despawns the provided entity and its children.
fn despawn_recursive(self);

/// Despawns the provided entity's children.
fn despawn_children(&mut self);
Copy link
Member

@cart cart Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the terseness of this name, it goes against the pattern we've already established:

  1. despawn: only despawns the entity
  2. despawn_recursive: despawns the entity and its descendants

If we apply the "despawn" action to children, that should only remove the children, not their descendants. According to the current naming conventions, this should be something like despawn_children_recursive.

That being said, there are already some issues prior to this pr, such as the docs for despawn_recursive being wrong: "despawns the provided entity and its children" is not the actual behavior of despawn_recursive. It despawns the entity and its descendants.

Happy to discuss the best way to resolve this situation. Some random thoughts:

  • If we somehow make "despawn recursive" the default behavior for despawn, then we can use the terser name here. Recursive despawns are the default in godot, which sort of makes sense to me, especially given that "scenes" are spawned as node trees in a hierarchy in godot, and we do the same thing in bevy. I expect that generally people despawning an entity would want the descendants to go with it. I'm not actually convinced yet that this is a good idea for bevy, but its worth discussing. It would require adding implicit per-component behaviors to the normal despawn action (either directly or as a "reaction").
  • We could rename despawn_children to despawn_children_recursive.
  • We could accept the inconsistent naming and move on. Not a huge fan of this. I'd prefer to at least have a general idea of what this api should (ideally) look like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like making recursive the default, it's probably what people want to use anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think option 1 is the ideal option, it's definitely not actionable for this PR. As for between option two or three, I'd personally prefer to keep the PR as is, but any clear cut answer would be great. The fourth implied option would be to not add this functionality until we have the ideal api, but that's not very appealing either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. No sense in blocking a useful feature on a big overhaul. I would prefer consistency here in the short term. Can you change this to despawn_children_recursive and fix the documentation to properly describe the recursive behavior for both despawn_recursive and despawn_children_recursive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about despawn_descendants?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too cryptic IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but it forces the user to contend with the realities of the situation, so I think I still prefer it. despawn_children_recursive also requires a bit of brain work to understand exactly what it does. despawn_children has the aforementioned consistency issues. despawn_descendants does what it says on the tin and uses standard tree terminology. If we ever change the despawn semantics, we can move back to the less cryptic despawn_children naming.

}

impl<'w, 's, 'a> DespawnRecursiveExt for EntityCommands<'w, 's, 'a> {
Expand All @@ -53,6 +75,11 @@ impl<'w, 's, 'a> DespawnRecursiveExt for EntityCommands<'w, 's, 'a> {
let entity = self.id();
self.commands().add(DespawnRecursive { entity });
}

fn despawn_children(&mut self) {
let entity = self.id();
self.commands().add(DespawnChildrenRecursive { entity });
}
}

impl<'w> DespawnRecursiveExt for EntityMut<'w> {
Expand All @@ -65,6 +92,15 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> {
despawn_with_children_recursive(self.world_mut(), entity);
}
}

fn despawn_children(&mut self) {
let entity = self.id();
// SAFE: The location is updated.
unsafe {
despawn_children(self.world_mut(), entity);
self.update_location();
}
}
}

#[cfg(test)]
Expand Down