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

Make standard commands more ergonomic (in niche cases) #8249

Merged
merged 5 commits into from
Mar 29, 2023

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Mar 29, 2023

Objective

I ran into a case where I need to create a CommandQueue and push standard Command actions like Insert or Remove to it manually. I saw that Remove looked as follows:

struct Remove<T> {
  entity: Entity,
  phantom: PhantomData<T>
}

so naturally, I tried to use Remove::<Foo>::from(entity) but it didn't exist. We need to specify the PhantomData explicitly when creating this command action. The same goes for RemoveResource and InitResource

Solution

This PR implements the following:

  • From<Entity> for Remove<T>
  • Default for RemoveResource and InitResource
  • use these traits in the implementation of methods of Commands
  • rename phantom field on the structs above to _phantom to have a more uniform field naming scheme for the command actions

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • Added: implemented From<Entity> for Remove<T> and Default for RemoveResource and InitResource for ergonomics

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use C-Code-Quality A section of code that is hard to understand or change labels Mar 29, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 29, 2023

This is marginally nicer, but definitely a niche bit of the API. On the fence if the complexity here is worth it.

Could we make constructor methods instead? I think that's a better spot on the complexity <-> ergonomics curve for a niche API.

@RobWalt RobWalt changed the title Make standard commands more ergonomic Make standard commands more ergonomic (in niche cases) Mar 29, 2023
@RobWalt
Copy link
Contributor Author

RobWalt commented Mar 29, 2023

Not a big fan of From impls in generals, so I'm really glad you asked!

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@RobWalt
Copy link
Contributor Author

RobWalt commented Mar 29, 2023

I was a bit too hasty there 😅 Good catches! Thanks!

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 29, 2023
@alice-i-cecile
Copy link
Member

Oh right: this is also technically breaking because you renamed the phantom data field. I'd probably just revert that? I don't think it's appreciably better.

@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 29, 2023
@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 Mar 29, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 29, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 29, 2023
Merged via the queue into bevyengine:main with commit 5f0abbf Mar 29, 2023
@RobWalt RobWalt deleted the feat/command-ergonomics branch March 29, 2023 19:18
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted 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.

3 participants