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

Rename EntityCommands::clone to clone_and_spawn #16696

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

JaySpruce
Copy link
Contributor

@JaySpruce JaySpruce commented Dec 6, 2024

Objective

Follow-up to #16672.

EntityCommands::clone looks the same as the Clone trait, which could be confusing. A discord discussion has made me realize that's probably a bigger problem than I thought. Oops :P

Solution

Renamed EntityCommands::clone to EntityCommands::clone_and_spawn, renamed EntityCommands::clone_with to EntityCommands::clone_and_spawn_with. Also added some docs explaining the commands' relation to Clone (components need to implement it (or Reflect)).

Showcase

// Create a new entity and keep its EntityCommands
let mut entity = commands.spawn((ComponentA(10), ComponentB(20)));

// Create a clone of the first entity
let mut entity_clone = entity.clone_and_spawn();

The Bikeshed

  • clone_and_spawn (Alice's suggestion)
  • spawn_clone (benfrankel's suggestion)
  • spawn_cloned (rparrett's suggestion)

@rparrett
Copy link
Contributor

rparrett commented Dec 6, 2024

Here's a link to the referenced conversation.

https://discord.com/channels/691052431525675048/749335865876021248/1314635145650638960

(btw, can't wait to use this feature)

@JaySpruce
Copy link
Contributor Author

Credit to @eugineerd for the neat feature, sorry for repeatedly nitpicking it :P

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Dec 8, 2024
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 definitely think we should rename this, but I find spawn_clone quite awkward and unclear. Are you okay with clone_and_spawn instead?

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 8, 2024
@rparrett
Copy link
Contributor

rparrett commented Dec 8, 2024

What about spawn_cloned?

Also cc @benfrankel who originally suggested spawn_clone.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Dec 8, 2024

That's also an improvement, but I think that the order of operations (clone the data out, then spawn a new entity with it) is clearest with my suggestion :)

@JaySpruce JaySpruce changed the title Rename EntityCommands::clone to spawn_clone Rename EntityCommands::clone to clone_and_spawn Dec 9, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 10, 2024
Merged via the queue into bevyengine:main with commit db4c468 Dec 10, 2024
29 checks passed
BD103 pushed a commit to BD103/bevy that referenced this pull request Dec 10, 2024
## Objective

Follow-up to bevyengine#16672.

`EntityCommands::clone` looks the same as the `Clone` trait, which could
be confusing. A discord discussion has made me realize that's probably a
bigger problem than I thought. Oops :P

## Solution

Renamed `EntityCommands::clone` to `EntityCommands::clone_and_spawn`,
renamed `EntityCommands::clone_with` to
`EntityCommands::clone_and_spawn_with`. Also added some docs explaining
the commands' relation to `Clone` (components need to implement it (or
`Reflect`)).

## Showcase

```
// Create a new entity and keep its EntityCommands
let mut entity = commands.spawn((ComponentA(10), ComponentB(20)));

// Create a clone of the first entity
let mut entity_clone = entity.clone_and_spawn();
```

## The Bikeshed

- `clone_and_spawn` (Alice's suggestion)
- `spawn_clone` (benfrankel's suggestion)
- `spawn_cloned` (rparrett's suggestion)
@JaySpruce JaySpruce deleted the rename_clone branch December 11, 2024 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants