[Merged by Bors] - Add despawn_children #2903
Conversation
TheRawMeatball
added
A-Transform
|
I like it but not much the name, I think naming it |
mockersf
added
S-Ready-For-Final-Review
| 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); |
There was a problem hiding this comment.
While I like the terseness of this name, it goes against the pattern we've already established:
despawn: only despawns the entitydespawn_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_childrentodespawn_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.
There was a problem hiding this comment.
I like making recursive the default, it's probably what people want to use anyway
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What about despawn_descendants?
There was a problem hiding this comment.
It's too cryptic IMO.
There was a problem hiding this comment.
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.
|
bors r+ |
Adds a convenience method for despawning all the children of the entity, but not the entity itself.
Adds a convenience method for despawning all the children of the entity, but not the entity itself.