From 9fc9e88bb2fbcd58dce3cfba54bdd92254526a35 Mon Sep 17 00:00:00 2001 From: ira Date: Fri, 16 Dec 2022 19:53:23 +0000 Subject: [PATCH] Remove `EntityCommands::add_children` (#6942) # 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 --- crates/bevy_hierarchy/src/child_builder.rs | 50 +++------------------- 1 file changed, 6 insertions(+), 44 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index fb092cee0b241f..0bb8b29f627533 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -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(&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(&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);