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

Add set_state extension method to Commands #15083

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Sep 7, 2024

Objective

  • Improve the ergonomics of managing states.

Solution

  • Add set_state extension method to Commands so you don't need to type out ResMut<NextState<S>> to update a state. It also reduces system parameter list size when you already have Commands.
  • I only updated a couple examples to showcase how it can be used. There is a potential perf cost to introducing Commands so this method shouldn't necessarily be used everywhere.

Testing

  • Tested the updated examples: game_menu and alien_cake_addict.

Showcase

Add Commands::set_state method for easily updating states.

Set directly:

fn go_to_game(mut game_state: ResMut<NextState<GameState>>) {
    game_state.set(GameState::Play);
}

Set with commands (NEW):

fn go_to_game(mut commands: Commands) {
    commands.set_state(GameState::Play);
}

@TrialDragon TrialDragon added C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-States App-level states machines labels Sep 7, 2024
Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

I've tested the examples that were changed, they still work. I feel it is adequately documented, and I personally like the API ergonomics of being able to just use commands to set state, whilst still having the direct setting for performance use cases. LGTM.

@TrialDragon TrialDragon added C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples labels Sep 7, 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'm on board with this as a convenience API (and the flexible nature of it is genuinely useful), but I'd like to revert the changes to the examples. We should teach users a single, consistent way to use our APIs, and default to the clearer and more performant existing API.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible C-Examples An addition or correction to our examples labels Sep 8, 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 X-Contentious There are nontrivial implications that should be thought through and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

I don't think we need to worry about command overhead, state switching is a pretty heavy operation just because of the transitions involved.

Merged via the queue into bevyengine:main with commit ce32b5c Sep 9, 2024
28 checks passed
@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 9, 2024

Second thoughts, I'd like this to be the standard way of updating states.
Using commands:

  • implies the deferred nature of this operation
  • reduces clutter associated with NextState, especially when many are used
  • does not make problems with system ambiguities worse
  • allows for falliable operations with a warning, which we will need when the traits get unified and not everything will use NextState
  • with a different command you can force the transition to happen immediately without writing an exclusive system

@alice-i-cecile what do you think?
I don't think this change has to happen now, but it looks like a better API overall to me

@UkoeHB UkoeHB deleted the set_state branch September 9, 2024 17:37
@Shatur
Copy link
Contributor

Shatur commented Sep 9, 2024

I'm on board with @MiniaczQ, I would even remove the NextState method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-States App-level states machines C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

5 participants