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

Systems set to run on state resume do not respect .after() constraints #6130

Closed
Neurrone opened this issue Sep 30, 2022 · 10 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@Neurrone
Copy link

Bevy version

0.8.1

What you did

Describe how you arrived at the problem. If you can, consider providing a code snippet or link.

I have my game menus each have a corresponding AppState. For example, there is a state for options menu, main menu etc.

I have two systems:

  • on_previous_menu_event which I need ran before
  • on_resume, which gets ran when I resume a state.

I tried to achieve this by doing the following:

  .add_system(on_previous_menu_event.before(on_menu_spawn))
  .add_system_set(SystemSet::on_resume(AppState::MainMenu).with_system(on_resume.after(on_previous_menu_event)))

The ordering is required for the logic to work correctly.

What went wrong

By inserting print statements into both systems, I see that about half the time, the systems are ran in the opposite order in which I declared above.

Additional information

Other information that can be used to further reproduce or isolate the problem.
This commonly includes:

  • screenshots
  • logs
  • theories about what might be going wrong
  • workarounds that you used
  • links to related bugs, PRs or discussions
@Neurrone Neurrone added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Sep 30, 2022
@tim-blackbird
Copy link
Contributor

tim-blackbird commented Sep 30, 2022

I don't think it's possible to order a system relative to a system in a different set, currently. I think this is supposed to throw a warning.
You can instead order the entire SystemSet to run after on_previous_menu_event.

Both seem to work.

@Neurrone
Copy link
Author

You can instead order the entire SystemSet to run after on_previous_menu_event.

I didn't know that was possible, how do I do that?

@Neurrone
Copy link
Author

I tried this which doesn't seem to work either.

.add_system_set(SystemSet::on_resume(AppState::MainMenu).after(on_previous_menu_event).with_system(on_resume))

@tim-blackbird
Copy link
Contributor

I tried reproducing with the code below, but it behaves as i would expect.

Spoiler
use std::time::Duration;

use bevy::prelude::*;

#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
enum AppState {
    One,
    Two,
}

fn system_a(mut state: ResMut<State<AppState>>) {
    match state.current() {
        AppState::One => {
            println!("push two");
            let _ = state.push(AppState::Two);
        }
        AppState::Two => {
            println!("resuming one");
            let _ = state.pop();
        }
    }
    println!("a");
    std::thread::sleep(Duration::from_millis(500));
}

fn system_b() {
    println!("b");
    std::thread::sleep(Duration::from_millis(500));
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_state(AppState::One)
        .add_system(system_a)
        .add_system_set(SystemSet::on_resume(AppState::One).with_system(system_b.after(system_a)))
        .run();
}

@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 Sep 30, 2022
@Neurrone
Copy link
Author

Neurrone commented Oct 1, 2022

I'll try to create a minimal reproduceable example, thanks a lot for that code sample. I'll build off on that to get something that better matches my code.

@Neurrone
Copy link
Author

Neurrone commented Oct 1, 2022

In the process of trying to emonstrate this bug, I believe I may have found another one.

use std::time::Duration;

use bevy::prelude::*;

#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
enum AppState {
    MainMenu,
    Options,
}

fn display_options_menu(mut state: ResMut<State<AppState>>) {
    std::thread::sleep(Duration::from_millis(500));
    println!("Switching to options menu");
    state.push(AppState::Options).unwrap();
}

fn resume_main_menu() {
    println!("Resuming main menu state");
}

struct PreviousMenuEvent;

fn return_to_main_menu(
    mut state: ResMut<State<AppState>>,
    mut prev_menu_evt: EventWriter<PreviousMenuEvent>,
) {
    // std::thread::sleep(Duration::from_millis(500));
    println!("Returning to main menu");
    prev_menu_evt.send(PreviousMenuEvent);
    state.pop().unwrap();
}

fn on_previous_menu_event(mut prev_menu_evt: EventReader<PreviousMenuEvent>) {
    println!("Running on_previous_menu_event system");
    for _ev in prev_menu_evt.iter() {
        println!("Got previous menu event");
    }
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_event::<PreviousMenuEvent>()
        .add_system(on_previous_menu_event)
        .add_state(AppState::MainMenu)
        .add_system_set(SystemSet::on_update(AppState::MainMenu).with_system(display_options_menu))
        .add_system_set(
            SystemSet::on_update(AppState::Options)
                .with_system(return_to_main_menu.before(on_previous_menu_event)),
        )
        .add_system_set(
            SystemSet::on_resume(AppState::MainMenu)
                .with_system(resume_main_menu.after(on_previous_menu_event)),
        )
        .run();
}

In this example, the bug I intended to demonstrate was resume_main_menu doesn't run properly after on_previous_menu_event.

However when I run this example, the on_previous_menu_event system is only ever run once and never gets run subsequently, which is a separate bug that I haven't encountered before.

@tim-blackbird
Copy link
Contributor

When a state transition happens it re-runs on_update so the above code is entering an endless loop, it never even reaches the PostUpdate stage.

Jeez. The state stuff is super prone to footguns and nothing is documented :/
I'm glad we're going to yeet this implementation

@Neurrone
Copy link
Author

Neurrone commented Oct 1, 2022

I'm very confused that on_update gets run a second time during a state transition.

I'll refactor it so that it doesn't run during a state update and see if that fixes it.

@Neurrone
Copy link
Author

Neurrone commented Oct 1, 2022

I've reproduced the problem, where the resume code runs before the event is received. Although now I suspect that the event being received only after returning to the main menu is just due to the order that on_previous_menu_event runs.

on_previous_menu_event is probably run before switch_menus, which would explain why I only receive the event when returning to the main menu.

However, this doesn't explain why the .after constraint on on_resume doesn't seem to work.

use bevy::prelude::*;

#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
enum AppState {
    MainMenu,
    Options,
}

fn switch_menus(
    mut state: ResMut<State<AppState>>,
    mut input: ResMut<Input<KeyCode>>,
    mut prev_menu_evt: EventWriter<PreviousMenuEvent>,
) {
    if !input.just_pressed(KeyCode::Return) {
        return;
    }
    input.clear();
    match state.current() {
        AppState::MainMenu => {
            println!("Switching to options menu");
            state.push(AppState::Options);
        }
        AppState::Options => {
            println!("Returning to main menu");
            prev_menu_evt.send(PreviousMenuEvent);
            state.pop().unwrap();
        }
    }
}

fn resume_main_menu() {
    println!("Resuming main menu state");
}

struct PreviousMenuEvent;

fn on_previous_menu_event(mut prev_menu_evt: EventReader<PreviousMenuEvent>) {
    // println!("Running on_previous_menu_event system");
    for _ev in prev_menu_evt.iter() {
        println!("Got previous menu event");
    }
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_event::<PreviousMenuEvent>()
        .add_system(on_previous_menu_event)
        .add_state(AppState::MainMenu)
        .add_system_set(SystemSet::on_update(AppState::MainMenu).with_system(switch_menus))
        .add_system_set(SystemSet::on_update(AppState::Options).with_system(switch_menus))
        .add_system_set(
            SystemSet::on_resume(AppState::MainMenu)
                .with_system(resume_main_menu.after(on_previous_menu_event)),
        )
        .run();
}

I can work around this issue, e.g by using .add_system(on_previous_menu_event.after(switch_menus)) to get the desired effect.

@alice-i-cecile
Copy link
Member

Obsoleted by #7267. On enter / on exit systems are now found their own schedules.

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
Archived in project
Development

No branches or pull requests

3 participants