Setting Run Criteria for State-based System Set Overwrites System Set State Run Critera

[zicklag]

Bevy version

97d8e4e

Operating system & version

Pop!_OS ( Ubuntu ) 20.04

What you did

Here's a full example with working and not working:

use bevy::{app::AppExit, core::FixedTimestep, prelude::*};

#[derive(PartialEq, Eq, Hash, Clone, Debug)]
enum GameState {
    Loading,
    Running,
}

fn main() {
    // working();
    not_working()
}

fn working() {
    println!("Working example:");
    App::build()
        .add_plugins(DefaultPlugins)
        .add_state(GameState::Loading)
        .add_system_set(
            SystemSet::on_update(GameState::Loading)
                // Wait for game to load
                .with_system(
                    (|mut counter: Local<usize>, mut state: ResMut<State<GameState>>| {
                        println!("Loading: {}/100", *counter);
                        if *counter == 100 {
                            state.push(GameState::Running).unwrap();
                        }

                        *counter += 1;
                    })
                    .system(),
                ),
        )
        .add_system_set(
            SystemSet::on_update(GameState::Running)
                // Run the game ( which just exits the program )
                .with_system(
                    (|mut exit_events: EventWriter<AppExit>| {
                        println!("running");
                        exit_events.send(AppExit);
                    })
                    .system(),
                ),
        )
        .run();
}

fn not_working() {
    println!("Not working example:");
    App::build()
        .add_plugins(DefaultPlugins)
        .add_state(GameState::Loading)
        .add_system_set(
            SystemSet::on_update(GameState::Loading)
                // Wait for game to load
                .with_system(
                    (|mut counter: Local<usize>, mut state: ResMut<State<GameState>>| {
                        println!("Loading: {}/100", *counter);
                        if *counter == 100 {
                            state.push(GameState::Running).unwrap();
                        }

                        *counter += 1;
                    })
                    .system(),
                ),
        )
        .add_system_set(
            SystemSet::on_update(GameState::Running)
                .with_run_criteria(FixedTimestep::step(0.001))
                // Run the game ( which just exits the program )
                .with_system(
                    (|mut exit_events: EventWriter<AppExit>| {
                        println!("running");
                        exit_events.send(AppExit);
                    })
                    .system(),
                ),
        )
        .run();
}

It appears that the with_run_criteria is overwriting the run criteria of the SystemSet that is supposed to run it only when the GameState::Running state is active.

What you expected to happen

Both the working and the not working examples should run the same, with all the assets being loaded before the program exits.

What actually happened

The working output is as expected, all "assets" are loaded before game goes into running state and send the exit event:

Working example:
Loading: 0/100
Loading: 1/100
Loading: 2/100
Loading: 3/100
Loading: 4/100
...
Loading: 97/100
Loading: 98/100
Loading: 99/100
Loading: 100/100
running

The not working output though will run the game system that exits the program before all the assets have finished loading and the state transition is made:

Not working example:
Loading: 0/100
Loading: 1/100
Loading: 2/100
Loading: 3/100
running

Extra Context

Here's my current workaround to combine the fixed timestep run criteria with the game state run criteria:

.add_system_set(
            SystemSet::new()
                .with_run_criteria(
                    FixedTimestep::step(0.001).chain(
                        (|In(input): In<ShouldRun>, state: Res<State<GameState>>| {
                            if state.current() == &GameState::Running {
                                input
                            } else {
                                ShouldRun::No
                            }
                        })
                        .system(),
                    ),
                )
                // Wait for game to load
                .with_system(
                    (|mut exit_events: EventWriter<AppExit>| {
                        println!("running");
                        exit_events.send(AppExit);
                    })
                    .system(),
                ),
        )

[TheRawMeatball]

This is underdocumented, but expected behavior. This is a result of the fact that there is no obviously correct truth table for merging 2 run criteria. However, you should be able to counteract this issue by using piped run criteria.

[zicklag]

Could we maybe add some kind of combinators then like ( maybe different name ) .with_or_run_criteria or with_and_run_criteria? Or maybe chaining is just better, but I couldn't figure out how to use a change in this case, do you have an example?

Also, I think that if this is expected behavior it would be much better to take out the convenience SystemSet::on_update functions in favor of a .with_run_criteria(SystemState::on_update(...)) pattern. That way it is clear that saying with_run_criteria is overwriting the SystemState criteria. Otherwise I have to understand the implementation detail that SystemSet::on_update is actually just sugar for a run criteria.

[Ratysz]

Yes, the states' sets are a bit of a holdover right now. A good chunk of API is due for a second pass to tighten inconsistencies like this.

"Piping" criteria is a little bit different from chaining, hence a dedicated verb. This test should serve as an example; the gist: label the criteria when first inserting it, and then use RunCriteria::pipe(SomeCriteriaLabel, some_pipeable_criteria.system()) or SomeCriteriaLabel.pipe(some_pipeable_criteria.system()) (where some_pipeable_criteria has a In<ShouldRun> argument) to construct a combinator that will reuse the result of the "parent" criteria.

[zicklag]

I'm a little confused with the piping though, because what if I don't have a system to add the run first run critiera to and I only have the piped version.

For instance, in the test you linked to you first add the "every other time" run criteria by itself to a system and label it, but what if I don't have a separate system to add it to and I only have one system. Then I can't add it to one system with a label and use the label later. That's why tried a chain instead, because it let me insert it in one step.

Also, I'm not able to pipe two built-in systems that aren't designed for chaining such as State::on_update and FixedTimestep::step because the FixedTimeStep::step doesn't have a In<ShouldRun> arg.

Maybe we need "proxy" criteria that can be used to chain two run criterias with .and and .or. 🤔

Anyway, thanks both for the help! I'm just kind of brainstorming ideas, but I get that there's still some work to be done here and I've got a decent work-around so I'm happy. 😃

[Ratysz]

There are methods to add criteria directly to stage, without having to attach them to a system. They do require you to label it, though, for obvious reasons.

[zicklag]

Ah, OK. Got it. 👍

[bayswaterpc]

Wanted to mention I ran into this same issue, and this thread helped immensly thanks everyone asking and starting the conversation

/// An implementation of the classic game "Breakout" with egui panels
const TIME_STEP: f32 = 1.0 / 60.0;
fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_plugin(EguiPlugin)
        .insert_resource(Scoreboard { score: 0 })
        .insert_resource(ClearColor(Color::rgb(0.9, 0.9, 0.9)))
        .add_state(AppState::CentralPanelState)
        .add_startup_system(setup.system())
        //Time Step overrides gamestate requirement
        // .add_system_set(
        //     SystemSet::on_update(AppState::GameState)
        //         .with_run_criteria(FixedTimestep::step(TIME_STEP as f64))
        //         .with_system(paddle_movement_system.system())
        //         .with_system(ball_collision_system.system())
        //         .with_system(ball_movement_system.system()),
        // )
        .add_system_set( //follows timestep & state requirements
            SystemSet::new()
                .with_run_criteria(
                    FixedTimestep::step(TIME_STEP as f64).chain(
                        (|In(input): In<ShouldRun>, state: Res<State<AppState>>| {
                            if state.current() == &AppState::GameState {
                                input
                            } else {
                                ShouldRun::No
                            }
                        })
                            .system(),
                    ),
                )
                // Wait for game to load
                .with_system(paddle_movement_system.system())
                .with_system(ball_collision_system.system())
                .with_system(ball_movement_system.system()),

        )
        .add_system_set(
            SystemSet::on_update(AppState::GameState)
                .with_system(scoreboard_system.system())
        )
        .add_system(ui_egui.system())
        .run();
}

You can find the running example here for reference for the curious https://github.com/bayswaterpc/bevy_egui/blob/main/examples/bevy_central_panel.rs

[alice-i-cecile]

Fixed by #7267.