-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Merged by Bors] - Remove EntityCommands::add_children
#6942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with removing this. I'm neutral on the rename: I think it's a better name and should be done eventually, but migrating directly may lead to confusing compiler messages.
Agreed that the API duplication here is definitely undesirable. The recent API additions definitely obviates the need for such an API. |
bors r+ |
# 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>
EntityCommands::add_children
EntityCommands::add_children
# Objective Align the hierarchy API between `EntityCommands` and `EntityMut`. Added missing methods to `EntityMut`. Replaced the duplicate `Command` implementations with the ones on `EntityMut` (e.g. The `AddChild` command is now just `world.entity_mut(..).add_child(..)`) Fixed `update_old_parents` not sending `ChildAdded` events. This PR does not add `add_children` to `EntityMut` as I would like to remove it from `EntityCommands` instead in #6942. ## Changelog * Added `add_child`, `set_parent` and `remove_parent` to `EntityMut` * Fixed missing `ChildAdded` events Co-authored-by: devil-ira <justthecooldude@gmail.com>
# Objective Align the hierarchy API between `EntityCommands` and `EntityMut`. Added missing methods to `EntityMut`. Replaced the duplicate `Command` implementations with the ones on `EntityMut` (e.g. The `AddChild` command is now just `world.entity_mut(..).add_child(..)`) Fixed `update_old_parents` not sending `ChildAdded` events. This PR does not add `add_children` to `EntityMut` as I would like to remove it from `EntityCommands` instead in #6942. ## Changelog * Added `add_child`, `set_parent` and `remove_parent` to `EntityMut` * Fixed missing `ChildAdded` events Co-authored-by: devil-ira <justthecooldude@gmail.com>
# Objective Remove a method with an unfortunate name and questionable usefulness. Added in bevyengine#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 bevyengine#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>
…yengine#6926) # Objective Align the hierarchy API between `EntityCommands` and `EntityMut`. Added missing methods to `EntityMut`. Replaced the duplicate `Command` implementations with the ones on `EntityMut` (e.g. The `AddChild` command is now just `world.entity_mut(..).add_child(..)`) Fixed `update_old_parents` not sending `ChildAdded` events. This PR does not add `add_children` to `EntityMut` as I would like to remove it from `EntityCommands` instead in bevyengine#6942. ## Changelog * Added `add_child`, `set_parent` and `remove_parent` to `EntityMut` * Fixed missing `ChildAdded` events Co-authored-by: devil-ira <justthecooldude@gmail.com>
# Objective Remove a method with an unfortunate name and questionable usefulness. Added in bevyengine#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 bevyengine#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>
…yengine#6926) # Objective Align the hierarchy API between `EntityCommands` and `EntityMut`. Added missing methods to `EntityMut`. Replaced the duplicate `Command` implementations with the ones on `EntityMut` (e.g. The `AddChild` command is now just `world.entity_mut(..).add_child(..)`) Fixed `update_old_parents` not sending `ChildAdded` events. This PR does not add `add_children` to `EntityMut` as I would like to remove it from `EntityCommands` instead in bevyengine#6942. ## Changelog * Added `add_child`, `set_parent` and `remove_parent` to `EntityMut` * Fixed missing `ChildAdded` events Co-authored-by: devil-ira <justthecooldude@gmail.com>
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:
The docs for
add_children
suggest the following:I would instead suggest using the following snippet.
Using
add_children
gets more unwieldy when you also want theparent_id
.The name
I see why
add_children
is named that way, it's the non-builder variant ofwith_children
so it kinda makes sense,but now the method name situation for
add_child
,add_children
andpush_children
is rather unfortunate.Removing
add_children
and renamingpush_children
toadd_children
in one go is kinda bleh, but that way we end up with the matching methodsadd_child
andadd_children
.Another reason to rename
push_children
is that it's trying to mimick theVec
api naming but fails becausepush
is for single elements. I guess it should have beenextend_children_from_slice
, but lets not name it that :)Questions
Shouldpush_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
onEntityCommands
was removed.If you were using
add_children
overwith_children
to return data out of the closure you can useset_parent
oradd_child
to avoid the closure instead.