[Merged by Bors] - Support returning data out of with_children#4708
Closed
SUPERCILEX wants to merge 9 commits intobevyengine:mainfrom
SUPERCILEX:with_children
Closed
[Merged by Bors] - Support returning data out of with_children#4708SUPERCILEX wants to merge 9 commits intobevyengine:mainfrom SUPERCILEX:with_children
SUPERCILEX wants to merge 9 commits intobevyengine:mainfrom
SUPERCILEX:with_children
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
alice-i-cecile
added
C-Usability
alice-i-cecile
requested changes
May 16, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
I like the direction here quite a bit. It needs a doc example somewhere though, and I'd like to get more reviews on the lifetime nonsense.
DJMcNab
reviewed
May 16, 2022
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Member
|
I'm on board with the new design, but the comment about a doc test still stands :) |
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Contributor
Author
|
Lol, yep. Done! |
DJMcNab
approved these changes
May 17, 2022
| }; | ||
| use smallvec::SmallVec; | ||
|
|
||
| use crate::prelude::{Children, Parent, PreviousParent}; |
Member
There was a problem hiding this comment.
We really need automated tooling for our imports. It's unclear to me why these have moved, but possibly it's our style?
Contributor
Author
There was a problem hiding this comment.
I think CLion tries to organize imports as external, internal, crate. I can revert if desired.
Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
DJMcNab
approved these changes
May 17, 2022
Member
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I haven't memorised our import style, so I'm not sure what we should do here. It might be easier just to revert that change, but it's not blocking either way.
Contributor
Author
|
Well the real answer is this lol: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=import#group_imports. But it isn't stable and messes up cfgs when I tried it. |
alice-i-cecile
requested changes
May 17, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
One doc suggestion, then I'm happy to merge this.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 17, 2022
# Objective Support returning data out of with_children to enable the use case of changing the parent commands with data created inside the child builder. ## Solution Change the with_children closure to return T. Closes #2817. --- ## Changelog `BuildChildren::add_children` was added with the ability to return data to use outside the closure (for spawning a new child builder on a returned entity for example).
Contributor
bors
bot
changed the title
exjam
pushed a commit
to exjam/bevy
that referenced
this pull request
May 22, 2022
# Objective Support returning data out of with_children to enable the use case of changing the parent commands with data created inside the child builder. ## Solution Change the with_children closure to return T. Closes bevyengine#2817. --- ## Changelog `BuildChildren::add_children` was added with the ability to return data to use outside the closure (for spawning a new child builder on a returned entity for example).
bors bot
pushed a commit
that referenced
this pull request
Dec 16, 2022
# 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>
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# 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>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# 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>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Support returning data out of with_children to enable the use case of changing the parent commands with data created inside the child builder. ## Solution Change the with_children closure to return T. Closes bevyengine#2817. --- ## Changelog `BuildChildren::add_children` was added with the ability to return data to use outside the closure (for spawning a new child builder on a returned entity for example).
alice-i-cecile
added
A-ECS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Support returning data out of with_children to enable the use case of changing the parent commands with data created inside the child builder.
Solution
Change the with_children closure to return T.
Closes #2817.
Changelog
BuildChildren::add_childrenwas added with the ability to return data to use outside the closure (for spawning a new child builder on a returned entity for example).