Ambiguity checker is unaware of run criteria (including States)

[alice-i-cecile]

Bevy version

348e2a3d404bc61afe534371b100dc173f6d076f

Operating system & version

Irrelevant.

What you did

use bevy::ecs::schedule::ReportExecutionOrderAmbiguities;
use bevy::prelude::*;

#[derive(Clone, Eq, PartialEq)]
enum AppState {
    Menu,
    InGame,
}

#[derive(Default)]
struct Data;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_state(AppState::Menu)
        .init_resource::<Data>()
        .insert_resource(ReportExecutionOrderAmbiguities)
        .add_system_set(SystemSet::on_update(AppState::Menu).with_system(menu.system()))
        .add_system_set(SystemSet::on_update(AppState::InGame).with_system(game.system()))
        .run();
}

fn menu(mut _data: ResMut<Data>) {}
fn game(mut _data: ResMut<Data>) {}

What you expected to happen

No ambiguities should be found.

What actually happened

A system-order ambiguity between menu_system and gameplay_system was found.

 * Parallel systems:
 -- "&bevy_scratchpad::game" and "&bevy_scratchpad::menu"
    conflicts: ["bevy_scratchpad::Data"]

Additional information

First reported here.

[alice-i-cecile]

Obviously, the state case is the most important example of this. But it also occurs with other run criteria:

use bevy::ecs::schedule::{ReportExecutionOrderAmbiguities, ShouldRun};
use bevy::prelude::*;
#[derive(Default)]
struct Data;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .init_resource::<Data>()
        .insert_resource(ReportExecutionOrderAmbiguities)
        .add_system_set(
            SystemSet::new()
                .with_system(never.system())
                .with_run_criteria(never_run.system()),
        )
        .add_system_set(SystemSet::new().with_system(always.system()))
        .run();
}

fn never_run() -> ShouldRun {
    ShouldRun::No
}

fn never(mut _data: ResMut<Data>) {}
fn always(mut _data: ResMut<Data>) {}

 * Parallel systems:
 -- "&bevy_scratchpad::always" and "&bevy_scratchpad::never"
    conflicts: ["bevy_scratchpad::Data"]

Obviously, this isn't the most useful code snippet imaginable, but one could imagine a world where we investigate run criteria a bit more thoroughly and see which ones can co-occur in a static fashion.

[alice-i-cecile]

As @cart said, I think the correct answer for the short and probably medium term is to just hard-code the systems associated with each enum variant of a state / each "step" (e.g. on_enter, on_update) as being disjoint.

That said, this might automatically be fixed by a stageless approach (#1375), where we determine ambiguities via hard sync points instead (of which you should get between each state / step within a state).

[Ratysz]

That said, this might automatically be fixed by a stageless approach (#1375), where we determine ambiguities via hard sync points instead (of which you should get between each state / step within a state).

No, sync points have nothing to do with states. Sync points (dependencies, ambiguities...) are about execution order, states are about execution fact.

We could slightly-less-hard-code a fix for states: automatically define explicit execution order between systems with the appropriate run criteria. This is not that different, in the end, but it makes sure these relationships are reflected in the chached data instead of being simply hard-coded in the ambiguity checker. This won't reduce parallelization potential: systems in question are by definition not supposed to be executable at the same time.

This is solvable for states because we actually know the relationships systems with those run criteria will have. A general solution is impossible if we don't want to overcomplicate the API by requiring users to provide relationship tables of some kind.

A form of "define stuff by doing things to a label" API (lets call that "label properties"?) could make this more elegant, but we shouldn't be considering that as a thing-that-could-happen yet. (Stageless and label properties are orthogonal.)

[alice-i-cecile]

We could slightly-less-hard-code a fix for states: automatically define explicit execution order between systems with the appropriate run criteria. This is not that different, in the end, but it makes sure these relationships are reflected in the chached data instead of being simply hard-coded in the ambiguity checker. This won't reduce parallelization potential: systems in question are by definition not supposed to be executable at the same time.

I like this: it solves the problem and provides more useful information in other places we may want to display / work off of execution order.

[JoJoJet]

What's the status of this now that we have Schedule v3? OnEnter/OnExit systems now live in separate schedules so there should be no ambiguities. And of course, OnUpdate no longer exists.

[alice-i-cecile]

This is still technically true, but no longer a practical or fixable concern.

[alice-i-cecile]

Hmm, actually this still matters for systems running in different mutually exclusive states. Maybe we could detect that particular edge case?

[JoJoJet]

True. If we had some way of adding metadata to run conditions, we could hard-code in_state (and state_exists_and_equals) to mark systems as mutually exclusive with one another. We could add metadata by changing fn in_state() -> impl FnMut() to return an impl Condition wrapper type that includes the extra metadata.

[hymm]

We could maybe add a implicit set somehow for each in_state value and add ambiguous with for all those sets with each other. Might be tricky now that we removed the iterator over all the states.