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

Restore support for running fn EntityCommands on entities that might be despawned #11107

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Dec 27, 2023

Objective

In #9604 we removed the ability to define an EntityCommand as fn(Entity, &mut World). However I have since realized that fn(Entity, &mut World) is an incredibly expressive and powerful way to define a command for an entity that may or may not exist (fn(EntityWorldMut) only works on entities that are alive).

Solution

Support EntityCommands in the style of fn(Entity, &mut World), as well as fn(EntityWorldMut). Use a marker generic on the EntityCommand trait to allow multiple impls.

The second commit in this PR replaces all of the internal command definitions with ones using fn definitions. This is mostly just to show off how expressive this style of command is -- we can revert this commit if we'd rather avoid breaking changes.


Changelog

Re-added support for expressively defining an EntityCommand as a function that takes Entity, &mut World.

Migration Guide

All Command types in bevy_ecs, such as Spawn, SpawnBatch, Insert, etc., have been made private. Use the equivalent methods on Commands or EntityCommands instead.

If you were working with ChildBuilder, recreate these commands using a closure. For example, you might migrate a Command to insert components like:

// Before (0.12)
parent.add_command(Insert {
    entity: ent_text,
    bundle: Capitalizable,
});

// After (0.13)
parent.add_command(move |world: &mut World| {
    world.entity_mut(ent_text).insert(Capitalizable);
});

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 27, 2023
@JoJoJet JoJoJet added this to the 0.13 milestone Dec 27, 2023
@hymm hymm added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Dec 27, 2023
@hymm
Copy link
Contributor

hymm commented Dec 27, 2023

What effect does using a closure for commands have here? Each instance of a closure is a unique type, but I'm not sure I understand how that would effect the performance or the amount of code generated.

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 27, 2023

The closures shouldn't affect anything. They're just turning an explicit unique type into a hidden unique type.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ran some benches and seems like if there's any diff it's within the noise.

image

@hymm hymm removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jan 7, 2024
@JoJoJet JoJoJet added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 7, 2024
@JoJoJet JoJoJet requested a review from alice-i-cecile January 8, 2024 07:15
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 really like these changes. The reduced boilerplate is excellent.

IMO just completely removing things like the SpawnBatch struct from our public API is a net win: these were confusing and never used in practice.

@alice-i-cecile alice-i-cecile 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 Jan 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 8, 2024
Merged via the queue into bevyengine:main with commit df2ba09 Jan 8, 2024
25 checks passed
@JoJoJet JoJoJet deleted the command-fn branch January 9, 2024 00:17
github-merge-queue bot pushed a commit that referenced this pull request Mar 31, 2024
# Objective

- The `bundles` parameter in `insert_or_spawn_batch` method has
inconsistent naming with docs (e.g. `bundles_iter`) since #11107.

## Solution

- Replace `bundles` with `bundles_iter`, as `bundles_iter` is more
expressive to its type.
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

4 participants