Skip to content
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

Closed

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Dec 13, 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:

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:

let child_id = commands
    .spawn_empty()
    .add_children(|parent| parent.spawn_empty().id());

I would instead suggest using the following snippet.

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.

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.

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Hierarchy Parent-child entity hierarchies labels Dec 13, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a 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.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 14, 2022
@james7132
Copy link
Member

Agreed that the API duplication here is definitely undesirable. The recent API additions definitely obviates the need for such an API.

@james7132 james7132 added this to the 0.10 milestone Dec 14, 2022
@cart
Copy link
Member

cart commented Dec 16, 2022

bors r+

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>
@bors bors bot changed the title Remove EntityCommands::add_children [Merged by Bors] - Remove EntityCommands::add_children Dec 16, 2022
@bors bors bot closed this Dec 16, 2022
@tim-blackbird tim-blackbird deleted the remove-add_child branch December 16, 2022 20:21
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# 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>
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# 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>
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>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…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>
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
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants