forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove EntityCommands::add_children (#6942)

Browse Source 
# Objective
Remove a method with an unfortunate name and questionable usefulness.
Added in #4708

It doesn't make sense to me for us to provide a method to work around a limitation of closures when we can simply, *not* use a closure.
The limitation in this case is not being able to initialize a variable from inside a closure:

```rust
let child_id;
commands.spawn_empty().with_children(|parent| {
    // Error: passing uninitalized variable to a closure.
    child_id = parent.spawn_empty().id();
});

// Do something with child_id
```
The docs for `add_children` suggest the following:
```rust
let child_id = commands
    .spawn_empty()
    .add_children(|parent| parent.spawn_empty().id());
```
I would instead suggest using the following snippet.
```rust
let parent_id = commands.spawn_empty().id();
let child_id = commands.spawn_empty().set_parent(parent_id).id();

// To be fair, at the time of #4708 this would have been a bit more cumbersome since `set_parent` did not exist.
```

Using `add_children` gets more unwieldy when you also want the `parent_id`.
```rust
let parent_commands = commands.spawn_empty();
let parent_id = parent_commands.id();
let child_id = parent_commands.add_children(|parent| parent.spawn_empty().id());
```
### The name
I see why `add_children` is named that way, it's the non-builder variant of `with_children` so it kinda makes sense,
but now the method name situation for `add_child`, `add_children` and `push_children` is *rather* unfortunate.

Removing `add_children` and renaming `push_children` to `add_children` in one go is kinda bleh, but that way we end up with the matching methods `add_child` and `add_children`. 

Another reason to rename `push_children` is that it's trying to mimick the `Vec` api naming but fails because `push` is for single elements. I guess it should have been `extend_children_from_slice`, but lets not name it that :)

### Questions
~~Should `push_children` be renamed in this PR? This would make the migration guide easier to deal with.~~
Let's do that later.

Does anyone know of a way to do a simple text/regex search through all the github repos for usage of `add_children`?
That way we can have a better idea of how this will affect users. My guess is that usage of `add_children` is quite rare.

## Migration Guide
The method `add_children` on `EntityCommands` was removed.
If you were using `add_children` over `with_children` to return data out of the closure you can use `set_parent` or `add_child` to avoid the closure instead.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
This commit is contained in:
ira 2022-12-16 19:53:23 +00:00
parent 38d567d2c5
commit 0d606030a2
1 changed files with 6 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
crates/bevy_hierarchy/src/child_builder.rs
View File

@ -248,40 +248,7 @@ impl<'w, 's, 'a> ChildBuilder<'w, 's, 'a> {
/// Trait defining how to build children
pub trait BuildChildren {
/// Creates a [`ChildBuilder`] with the given children built in the given closure
///
/// Compared to [`add_children`][BuildChildren::add_children], this method returns self
/// to allow chaining.
fn with_children(&mut self, f: impl FnOnce(&mut ChildBuilder)) -> &mut Self;
/// Creates a [`ChildBuilder`] with the given children built in the given closure
///
/// Compared to [`with_children`][BuildChildren::with_children], this method returns the
/// the value returned from the closure, but doesn't allow chaining.
///
/// ## Example
///
/// ```no_run
/// # use bevy_ecs::prelude::*;
/// # use bevy_hierarchy::*;
/// #
/// # #[derive(Component)]
/// # struct SomethingElse;
/// #
/// # #[derive(Component)]
/// # struct MoreStuff;
/// #
/// # fn foo(mut commands: Commands) {
/// let mut parent_commands = commands.spawn_empty();
/// let child_id = parent_commands.add_children(|parent| {
/// parent.spawn_empty().id()
/// });
///
/// parent_commands.insert(SomethingElse);
/// commands.entity(child_id).with_children(|parent| {
/// parent.spawn(MoreStuff);
/// });
/// # }
/// ```
fn add_children<T>(&mut self, f: impl FnOnce(&mut ChildBuilder) -> T) -> T;
/// Pushes children to the back of the builder's children. For any entities that are
/// already a child of this one, this method does nothing.
///
@ -313,11 +280,6 @@ pub trait BuildChildren {
impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> {
fn with_children(&mut self, spawn_children: impl FnOnce(&mut ChildBuilder)) -> &mut Self {
self.add_children(spawn_children);
self
}
fn add_children<T>(&mut self, spawn_children: impl FnOnce(&mut ChildBuilder) -> T) -> T {
let parent = self.id();
let mut builder = ChildBuilder {
commands: self.commands(),
@ -327,11 +289,10 @@ impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> {
},
};
let result = spawn_children(&mut builder);
spawn_children(&mut builder);
let children = builder.push_children;
self.commands().add(children);
result
self
}
fn push_children(&mut self, children: &[Entity]) -> &mut Self {
@ -506,12 +467,13 @@ mod tests {
let mut commands = Commands::new(&mut queue, &world);
let parent = commands.spawn(C(1)).id();
let children = commands.entity(parent).add_children(|parent| {
[
let mut children = Vec::new();
commands.entity(parent).with_children(|parent| {
children.extend([
parent.spawn(C(2)).id(),
parent.spawn(C(3)).id(),
parent.spawn(C(4)).id(),
]
]);
});
queue.apply(&mut world);