Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 targeted 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 M-Needs-Migration-Guide 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
# for free to join this conversation on GitHub. Already have an account? # 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants