Skip to content

Added with_query to EntityCommands#9544

Closed
bushrat011899 wants to merge 7 commits intobevyengine:mainfrom
bushrat011899:EntityCommandsWithQuery
Closed

Added with_query to EntityCommands#9544
bushrat011899 wants to merge 7 commits intobevyengine:mainfrom
bushrat011899:EntityCommandsWithQuery

Conversation

@bushrat011899
Copy link
Copy Markdown
Contributor

Objective

Solution

  • Added a convenience method to EntityCommands, with_query which allows a mutable query to be run for a specific Entity.
  • Added documentation and unit tests as appropriate.

Example

commands
    .entity(player.entity)
    .insert(ReallyComplexPlayerBundle::default()) // This adds Health to the Entity
    .with_query::<&mut Health>(|mut health| {
        health.0 = 100;
    });

@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 labels Aug 23, 2023
Comment on lines +861 to +868
/// commands
/// .entity(player.entity)
/// .insert((Health(100), LuckyStart))
/// /* ... */
/// .with_query::<(&mut Health, &LuckyStart)>(|(mut health, _)| {
/// // Players with the LuckyStart component start with double health.
/// health.0 *= 2;
/// });
Copy link
Copy Markdown
Contributor

@ickshonpe ickshonpe Aug 24, 2023

Choose a reason for hiding this comment

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

This doesn't seem like an improvement compared to:

let player_commands = commands.entity(player.entity);
let base_health = 100;
if give_lucky_start {
    player_commands.insert((Health(2 * base_health), LuckyStart));
} else {
    player_commands.insert(Health(health));
}

Or a system that applies the health bonus when an entity with a LuckyStart is spawned. Having the rule applied in a system would make it more discoverable and it would apply universally to anything that gets a LuckyStart such as enemies etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added an alternate resolution to #4917 here, which basically moves this PR into a user-defined trait. The goal of this PR was just to close #4917, which is over a year old now. If this functionality isn't desired, I think it would be sensible to close #4917 and this PR.

Comment on lines +890 to +892
/// .with_query::<&mut Transform>(|mut transform| {
/// // Players should spawn slightly above their randomly chosen spawn point.
/// transform.translation += 0.1 * transform.up();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, seems like this would be better implemented as a system, instead of adding a closure for every entity where you want to adjust its translation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do think there's a material difference tho. Adding it as a system permanently adds it to the system schedule, adding whatever overhead (however small) that entails.

I will say that I don't necessarily believe my PR is an absolutely necessary change to Bevy, but it is the minimum reasonable solution (IMO) to the issue raised that this PR is attempting to close. If this PR does get rejected because the feature itself isn't worth adding, I do hope the issue does as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that this is an important distinction, and really what justifies this feature to me.

Copy link
Copy Markdown
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 think this is reasonably useful, but it needs a bit of polish before merging.

If others feel this is particularly useful or useless, I'm happy to follow your opinions there.

@joseph-gio
Copy link
Copy Markdown
Member

I see the utility, but I'll note that this feels like a more specialized form of one-shot systems (which I believe we will eventually support). Thinking holistically, would we want to support both this and one-shot systems? (We may -- I haven't formed a full opinion on this yet).

Also, I think the inclusion of a with_query_filtered variant is a bit odd, given that this only runs for a single entity. If users want to add conditions I think it would make more sense to just put an if statement inside of the closure.

Also, I think there should be some kind of error or warning if the command gets applied on an entity that doesn't match the query.

@joseph-gio
Copy link
Copy Markdown
Member

Something that would achieve a similar effect is to implement EntityCommand for FnOnce(EntityMut). That way you could do:

commands
    .spawn()
    .add(|mut entity: EntityMut| {
        entity.get_mut::<Health>().0 *= 2;
    });

@alice-i-cecile
Copy link
Copy Markdown
Member

Something that would achieve a similar effect is to implement EntityCommand for FnOnce(EntityMut). That way you could do:

I like this much better!

@alice-i-cecile
Copy link
Copy Markdown
Member

Closing this out; let's do that instead in a new PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EntityCommands get mutable reference to added components

4 participants