OnEnter triggers twice and OnExit is not triggered

[jonathandw743]

Bevy version

0.13.2

[Optional] Relevant system information

Ubuntu 22.04.4 LTS 64-bit

Minimal project with the issue

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .insert_state(AppState::A)
        .insert_state(AppState::B)
        .add_systems(OnEnter(AppState::A), setup_a)
        .add_systems(OnEnter(AppState::B), setup_b)
        .add_systems(OnExit(AppState::A), cleanup_a)
        .add_systems(OnExit(AppState::B), cleanup_b)
        .run();
}

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

fn setup_a() {
    info!("setting up A");
}

fn setup_b() {
    info!("setting up B");
}

fn cleanup_a() {
    info!("cleaning up A");
}

fn cleanup_b() {
    info!("cleaning up B");
}

The Output

2024-06-14T14:30:43.872719Z  INFO minimalstateissue: setting up B
2024-06-14T14:30:43.872759Z  INFO minimalstateissue: setting up B

Expected Output

2024-06-14T14:30:43.872719Z  INFO minimalstateissue: setting up A
2024-06-14T14:30:43.872719Z  INFO minimalstateissue: cleaning up A
2024-06-14T14:30:43.872759Z  INFO minimalstateissue: setting up B

OR

2024-06-14T14:30:43.872759Z  INFO minimalstateissue: setting up B

depending on the intended behaviour (I can say that what happens now is not intended behaviour).

[rparrett]

States have been significantly reworked for 0.14, so it would be worth testing with 0.14.0.rc-2.

I am seeing something entirely different in 0.14.0.rc-2 (I also added the OnExit systems):

2024-06-14T14:38:11.676864Z  INFO i13844: setting up A

I think this could possibly be a bug with insert_state -- it seems to be behaving like init_state, not doing anything if the state is already present

[rparrett]

Pinging @MiniaczQ

[rparrett]

This patch results in the behavior I personally expect:

2024-06-14T15:08:05.639731Z  INFO i13844: setting up B

But folks more familiar with states should weigh in. I am not sure that the behavior of StateTransitionEvent is correct in the patch.

diff --git a/crates/bevy_state/src/app.rs b/crates/bevy_state/src/app.rs
index 2ff97f6ee..5fc7e8f49 100644
--- a/crates/bevy_state/src/app.rs
+++ b/crates/bevy_state/src/app.rs
@@ -76,19 +76,24 @@ impl AppExtStates for SubApp {
     }
 
     fn insert_state<S: FreelyMutableState>(&mut self, state: S) -> &mut Self {
-        if !self.world().contains_resource::<State<S>>() {
+        let already_initialized = self.world().contains_resource::<State<S>>();
+
+        self.insert_resource::<State<S>>(State::new(state.clone()));
+
+        if !already_initialized {
             setup_state_transitions_in_world(self.world_mut(), Some(Startup.intern()));
-            self.insert_resource::<State<S>>(State::new(state.clone()))
-                .init_resource::<NextState<S>>()
+
+            self.init_resource::<NextState<S>>()
                 .add_event::<StateTransitionEvent<S>>();
             let schedule = self.get_schedule_mut(StateTransition).unwrap();
             S::register_state(schedule);
-            self.world_mut().send_event(StateTransitionEvent {
-                exited: None,
-                entered: Some(state),
-            });
         }
 
+        self.world_mut().send_event(StateTransitionEvent {
+            exited: None,
+            entered: Some(state),
+        });
+
         self
     }

[MiniaczQ]

What's the use case behind initializing the same state twice?
If we don't have one I don't see why we'd support this if we want to fully overwrite the first call anyways.
(Just remove the first call)

Using the first call and ignoring subsequent ones seems reasonable to me.
A warning for initializing it multiple times is something we should do.

This is the run result for main branch btw:

2024-06-14T15:47:38.355810Z  INFO states: setting up A

Only first call runs, initialization with B is noop.

[rparrett]

It looks like insert_state exists to support initializing a state with a particular state value, and overwriting states wasn't considered.

#10731

So I was making an assumption based on the name insert and how other parts of the engine function.

A warning for initializing it multiple times is something we should do.

I think I agree with this.

[MiniaczQ]

I can imagine state overwriting being useful when using external modules, which initialize state by themselves.
But that can be considered an architectural issue instead.

Unless we have a proper use-case I don't think we need the overwrite support.

[rparrett]

Okay, so my assumption also seems in line with what the docs actually say about how that function behaves:

https://dev-docs.bevyengine.org/bevy/prelude/trait.AppExtStates.html#tymethod.insert_state

If we are going to officially stop supporting overwriting, we should make sure to update the docs.

[jonathandw743]

What's the use case behind initializing the same state twice? If we don't have one I don't see why we'd support this if we want to fully overwrite the first call anyways. (Just remove the first call)

I have a main menu which takes you to a game. There is state AppState{Game, MainMenu} and state PauseScreen(bool) for when the app is in AppState::Game. By default, the App goes into state AppState::MainMenu and PauseScreen(false) by default. I was making a plugin which would take me to the AppState::Game straight away. In doing this, the state was initialised twice.

[MiniaczQ]

If we already promise we support this then we should probably keep it that way, otherwise it's a breaking change :/
The use case seems pretty reasonable, I didn't consider debug tools :)

I'll see if we can get overwrite running, biggest problem is ensuring we get only one event after all setups