Adding run criteria to SystemSet::on_exit overwrites existing one

[wilk10]

Bevy version

Checked on 0.5 and main.

Operating system & version

MacOS 11.4

What you did

use bevy::{ecs::schedule::ShouldRun, prelude::*};

fn main() {
    App::build()
        .add_plugins(MinimalPlugins)
        .add_state(MyState::S1)
        .add_system_set(
            SystemSet::on_enter(MyState::S1)
                .with_system(exit_state_1.system())
        )
        .add_system_set(
            SystemSet::on_exit(MyState::S1)
                .with_run_criteria(check.system())
                .with_system(success.system())
        )
        .run();
}

struct Foo;

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
enum MyState {
    S1,
    S2,
}

fn exit_state_1(mut commands: Commands, mut state: ResMut<State<MyState>>) {
    commands.spawn().insert(Foo);
    state.set(MyState::S2).unwrap();
}

fn check(foo: Query<&Foo>) -> ShouldRun {
    dbg!(foo.iter().count());
    match foo.single() {
        Ok(_) => {
            println!("running");
            ShouldRun::Yes
        },
        Err(_) => {
            println!("not running");
            ShouldRun::No
        },
    }
}

fn success(foo: Query<&Foo>) {
    if let Ok(_) = foo.single() {
        println!("hurray");
        panic!("terminate");
    }
}

What you expected to happen

To print

running
hurray
thread 'Compute Task Pool (1)' panicked at 'terminate', src/main.rs:48:9

What actually happened

It loops indefinitely, printing:

[src/main.rs:33] foo.iter().count() = 0
not running

Additional information

I think the crux of the issue is that when building the SystemSet, with_run_criteria is called:

bevy/crates/bevy_ecs/src/schedule/system_set.rs

Lines 68 to 73 in 43d99bb

	pub fn on_exit<T>(s: T) -> SystemSet
	where
	T: Component + Debug + Clone + Eq + Hash,
	{
	Self::new().with_run_criteria(State::<T>::on_exit(s))
	}

but then when i add my own with_run_criteria, it is overwritten here:

bevy/crates/bevy_ecs/src/schedule/system_set.rs

Lines 99 to 102 in 43d99bb

	pub fn with_run_criteria<Marker>(mut self, run_criteria: impl IntoRunCriteria<Marker>) -> Self {
	self.run_criteria = Some(run_criteria.into());
	self
	}

So it's not run on State exit, and so it's run before it can detect the entity with Foo(i think).

Maybe a solution could be to pipe the custom run criteria on the one added on SystemSet init? That would be my preferred, if possible. Or if that's not possible/intended, perhaps to remove with_run_criteria from the public API for SystemSets?

Hope this is useful.
There's a tiny bit more information here
Thanks a lot!

[Ratysz]

Systems are intended to only have one criteria at most, because there is no good universal way to logically join criteria (there's plenty of discussion on that in issues linked from #2446). So, this is not as much of a bug as it is lack of a coherent warning or error; adding criteria to individual systems in a set with criteria does correctly produce an error, at least.

I should also note hat it's not possible to pipe into a criteria that isn't set up to be piped into.

[wilk10]

Thanks a lot for the reply!
The error message when adding a criteria to individual systems within a set was particularly useful indeed and put me on the right track investigating this.

Looking at SystemSet static methods, SystemSet::new() is the only one that doesn't automatically call with_run_criteria, and all other methods receive a RunCriteriaDescriptor from State impl (eg: SystemSet::on_exit(state)).
I think perhaps 90% of the time sets are initialised using those methods and not SystemSet::new() without with_run_criteria (ie: in Bevy's own codebase that happens only twice, and i never used it in my decently sized project, while i use system sets all the time).
So perhaps, especially after #2446 lands, would it be possible to support adding with_run_criteria on sets initialised using the other static methods? Because i believe Bevy is in control of the logical piping/joining of run criteria in this case, meaning that the custom with_run_criteria added by users can just be evaluated after the one received on initialisation.

Until then, perhapswith_run_criteria shouldn't be public, since it overwrites the run criteria that a set already has from initialisation in 90% of cases, making it useless.
A warning/panic would be great too, but hopefully all the ingredients are there to support adding custom run criteria to system sets, which would be awesome.

[Ratysz]

That's not really how piping works: the system being piped into needs to be able to take the piped argument and react to it. This is user-specific logic, Bevy can't write that for them automatically.

Both criteria and states need reworking before this mess can be untangled for good; we've started the design groundwork for it, but the subset of contributors that are willing and able to champion it are preoccupied.

For the short term, my take is the opposite: the SystemSet::on_exit() and its friends should be removed instead, with run criteria like State::on_exit() remaining as the sole method of interacting with states. It will remove this issue's source of confusion entirely, and will highlight that states are built on run criteria, and combining them with other criteria works through (multi)piping.

[wilk10]

Duplicate of #1839