`State::is_changed` is true in every frame, even if the value hasn't changed since last time the system was run.

[CGMossa]

Bevy version

main-branch

Operating system & version

Windows 10

What you did

Added a state and wanted to print out how the state after it is changed.

What you expected to happen

I expect the printing not to happen on each frame, and only when the state has been changed/set.

What actually happened

Additional information

This is very similar to #2275.

[NathanSWard]

Ahh this seems to be a problem due to the state_cleaner system which gets run for each state.
The problem seems to be that while the inner state may not change, there are other fields which may change.

I think the solution for the problem is going to be to add a way to access a ResMut mutably that does NOT trigger change detection.

[mockersf]

Here change detection is working properly: the resource State changed, even though the state didn't.

A proper fix would probably be:

• rename State to StateHandler. This State isn't the actual state
• add a state_changed(&self) -> bool to StateHandler

[NathanSWard]

add a state_changed(&self) -> bool to StateHandler

I'm not a big fan of this.
If possible, we should probably use the change detection pattern here and not introduce a new change detection only for states.

I see two paths:

1. Have a State<T> and a StateHandler<T>
2. have this work with Mut/ResMut mutable access without triggering change detection #2348

I'm slightly in favor of the second choice, however the first one I don't completely hate either. However, it should be noted that the first one adds more complexity in dealing with states.

[mockersf]

and

3. rename current State resource to StateHandler. This makes it obvious that change detection on it won't give you state change detection, and it's semantically correct as the State isn't the resource here

I think that the question "did I change state?" is a domain question that need to be answered by a method provided from the state handler, not a platform question provided by change detection on the resource holding it

[NathanSWard]

rename current State resource to StateHandler. This makes it obvious that change detection on it won't give you state change detection, and it's semantically correct as the State isn't the resource here

I don't dislike this solution, I'm just bothered that when you wrap it in a ResMut for example, you still have is_changed set_changed exposed.
Regardless of the name of the StateBikeshed struct, it's easy for a user to assume that is_changed refers to the state in it's entirety.
Even if you exposed an is_state_changed function, this is still ultimately a user trap that we should avoid.

I'm curious to know @alice-i-cecile or even perhaps @cart views on this.
For all I know, I'm completely wrong haha and StateHandler is the best approach 😄

I think the simplicity of State<T> is best is it make the most semantical sense that State<T>::is_changed refers the the changing of the held state. (Since that is the only thing the a user is able to observe from the structure.

[Ratysz]

I'm in favor of renaming State to StateHandler (or something to that tune). The current name, as is evident from this issue, is a bit misleading wrt change detection; renaming would completely disambiguate what's happening without a substantial API change, exposing more guts, requiring a new abstraction, or requiring a subversion of API contract suggested in #2348.

[alice-i-cecile]

I'm in agreement with Ratysz above.

[BGR360]

Chiming in as a new Bevy user. This bit me in the butt today.

Here's a minimal example if that's at all helpful:

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_state(AppState::A)
        .add_system(log_state_change)
        .run();
}

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
enum AppState {
    A,
    B,
}

fn log_state_change(app_state: Res<State<AppState>>) {
    if app_state.is_changed() {
        info!("App state changed: {:?}", app_state.current());
    }
}

This produces log spam on every frame, when the expected behavior would be no log spam.

On a related note, I found myself wanting a previous() method on the State struct, in addition to current().