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 state private and only accessible through getter for State resource #8009

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Mar 9, 2023

Objective

State requires a kind of awkward state.0 to get the current state and exposes the field directly to manipulation.

Solution

Make it accessible through a getter method as well as privatize the field to make sure false assumptions about setting the state aren't made.

Migration Guide

  • Use State::get instead of accessing the tuple field directly.

@Aceeri Aceeri added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 9, 2023
@alice-i-cecile
Copy link
Member

The field in there should really be private too 🤔 Should we make that change here, or in a new PR?

@Aceeri
Copy link
Member Author

Aceeri commented Mar 9, 2023

The field in there should really be private too 🤔 Should we make that change here, or in a new PR?

I'll just make the change here because yeah, otherwise its a bit error prone for setting the next state.

@james7132 james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 9, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

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.

@cart
Copy link
Member

cart commented Mar 21, 2023

Idk if this is a win. It forces the "double star deref" for the State resource:

// before
fn system_1(state: Res<State<AppState>>) {
    if state.0 == AppState::InGame {
        info!("do thing");
    }
}

// after
fn system(state: Res<State<AppState>>) {
    if **state == AppState::InGame {
        info!("do thing");
    }
}

@alice-i-cecile
Copy link
Member

I think it's important to make the field private, but I would be fine with a getter instead.

@cart
Copy link
Member

cart commented Mar 23, 2023

Yeah I think a getter is preferable to double star

@Aceeri Aceeri changed the title impl Deref for State<S> Make state private and only accessible through getter for State resource Mar 23, 2023
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.

The new constructor is unfortunate, but I think at least there's docs there. I don't think we can remove it safely: it's way too hard to build compatible alternative state impls without it.

///
/// // Now that we are in `GameState::Pause`, `pause_system` will run
/// app.run(&mut world);
/// assert_eq!(world.resource::<Counter>().0, 0);
/// ```
pub fn in_state<S: States>(state: S) -> impl FnMut(Res<State<S>>) -> bool + Clone {
move |current_state: Res<State<S>>| current_state.0 == state
move |current_state: Res<State<S>>| *current_state.get() == state
Copy link
Member

@cart cart Mar 23, 2023

Choose a reason for hiding this comment

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

One more thought / proposal:

  1. Only implement .get() for states that implement copy/clone. Return by value. Then 9/10 times comparisons can be current_state.get() == state
  2. Also implement Deref::deref(), which returns the reference. Use double star (or manual deref() calls) in these cases.

Copy link
Member

Choose a reason for hiding this comment

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

States has a Clone bound already, so I think it should just return by value 100% of the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I think implicitly cloning for getting the state would be great, I'd be more comfortable with impling on just Copy + Clone ones though.

If we want to simplify this a bit we could just impl PartialEq/Eq on State<S> itself and it could just be
current_state == state

Copy link
Member

Choose a reason for hiding this comment

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

I like the impl on State quite a bit actually: that really feels like the idiomatic approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yup way better!

@cart cart added this pull request to the merge queue Apr 4, 2023
Merged via the queue into bevyengine:main with commit ed50c8b Apr 4, 2023
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants