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

Incorrect WARN when using sub states #13854

Closed
jonathandw743 opened this issue Jun 14, 2024 · 5 comments · Fixed by #13862
Closed

Incorrect WARN when using sub states #13854

jonathandw743 opened this issue Jun 14, 2024 · 5 comments · Fixed by #13862
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@jonathandw743
Copy link

jonathandw743 commented Jun 14, 2024

Bevy version

0.14.0-rc.2

use bevy::prelude::*;

fn main() {
    let mut app = App::new();
    app.add_plugins(DefaultPlugins);
    app.insert_state(RootState);
    app.add_sub_state::<AppState>();
    app.add_sub_state::<PauseScreen>();
    app.add_systems(Update, foo.run_if(in_state(PauseScreen::On)));
    app.run();
}

// I want everything to be substates for copacetiousness
#[derive(States, Debug, Clone, PartialEq, Eq, Hash)]
pub struct RootState;

#[derive(SubStates, Debug, Clone, PartialEq, Eq, Hash, Default)]
#[source(RootState = RootState)]
pub enum AppState {
    #[default]
    MainMenu,
    Game,
}

#[derive(SubStates, Debug, Clone, PartialEq, Eq, Hash, Default)]
#[source(AppState = AppState::Game)]
pub enum PauseScreen {
    On,
    #[default]
    Off,
}

fn foo() {
    info!("in pause screen");
}

This program gives the warning:

2024-06-14T21:06:10.480930Z  WARN bevy_state::condition: No state matching the type for On exists - did you forget to `init_state` when initializing the app?

This doesn't seem to happen with only one layer of sub-states.
If it you do need to init_state, this is not suggested in the docs.

@jonathandw743 jonathandw743 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jun 14, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Jun 14, 2024
@alice-i-cecile
Copy link
Member

FYI @MiniaczQ @lee-orr

@MiniaczQ
Copy link
Contributor

Oh, that's an interesting issue.
So in_state uses the existence of State<S> to decide whether a state "exists", but computed/sub states work on optional resources.

We can either use a different resource to verify that a state is installed (e.g. Events<StateTransitionEvent<S>> is something I use), but then we need to add it as an argument to in_state 😛

Alternatively we can just remove the warning, it warns about something pretty obvious

@jonathandw743
Copy link
Author

jonathandw743 commented Jun 14, 2024

Oh, that's an interesting issue. So in_state uses the existence of State<S> to decide whether a state "exists", but computed/sub states work on optional resources.

We can either use a different resource to verify that a state is installed (e.g. Events<StateTransitionEvent<S>> is something I use), but then we need to add it as an argument to in_state 😛

Alternatively we can just remove the warning, it warns about something pretty obvious

The warning makes sense for States because it is assumed that they are initialised (idk how appropriate this assumption is). But it doesn't make sense for sub-states as they are inherently optional.

As an outsider, I will cautiously make the suggestion that sub states can be generalised so that all states are sub states. I don't think there needs to be two classes of states in this way (which makes the interface more complex). A regular state could then just be a sub state of some built-in unary root state.

Then if add_sub_state was not called for a state, then any in_state calls for that would only return false which is how sub-states behave.

@lee-orr
Copy link
Contributor

lee-orr commented Jun 15, 2024

The warning makes sense for States because it is assumed that they are initialised (idk how appropriate this assumption is). But it doesn't make sense for sub-states as they are inherently optional.

I'm uncertain what you mean by "it is assumed that they are initialized"? Can you elaborate?
I think my confusion is that all state types need to be registered with the app to work, so "initialized" in that sense applies to everything - but if you mean they can or can't be None that's a different distinction.

@lee-orr
Copy link
Contributor

lee-orr commented Jun 15, 2024

@alice-i-cecile - can you mark me as assigned here? I'm working on a solution RN

github-merge-queue bot pushed a commit that referenced this issue Jun 16, 2024
# Objective
Fixes #13854

## Solution
Removed the inaccurate warning. This was done for a few reasons:

- States not existing is now a valid "state" (for lack of a better term)
- Other run conditions don't provide an equivalent warning
mockersf pushed a commit that referenced this issue Jun 19, 2024
# Objective
Fixes #13854

## Solution
Removed the inaccurate warning. This was done for a few reasons:

- States not existing is now a valid "state" (for lack of a better term)
- Other run conditions don't provide an equivalent warning
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants