State-dependent system runs twice on wrong add_state call order

[vkaverin]

Bevy version

main branch on commit 284889c.

Operating system & version

MacOS Catalina 10.15.7 (19H2)

What you did

Consider following snippet:

use bevy::prelude::*;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_system_set(
            SystemSet::on_update(GameState::Running)
                .with_system(running_input.system()),
        )
        .add_state(GameState::Running)
        .run();
}

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

fn running_input(time: Res<Time>, ) {
    println!("{:?}", time.last_update())
}

What you expected to happen

System running_input is called once per frame.

What actually happened

System running_input is called twice per frame:

/Users/v.kaverin/.cargo/bin/cargo run --color=always --package rust-sandbox --bin rust-sandbox
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/rust-sandbox`
Some(Instant { t: 1625610921114726 })
Some(Instant { t: 1625610999077967 })
Some(Instant { t: 1625610999077967 })
Some(Instant { t: 1625611006604616 })
Some(Instant { t: 1625611006604616 })
Some(Instant { t: 1625611086654412 })
Some(Instant { t: 1625611086654412 })
Some(Instant { t: 1625611092062196 })
Some(Instant { t: 1625611092062196 })
Some(Instant { t: 1625611099839161 })
Some(Instant { t: 1625611099839161 })

Additional information

The result depend of when add_state was called.

After swapping add_state and add_system_set like

... 
        .add_state(GameState::Running)
        .add_system_set(
            SystemSet::on_update(GameState::Running)
                .with_system(running_input.system()),
        )
...

everything works just fine:

/Users/v.kaverin/.cargo/bin/cargo run --color=always --package rust-sandbox --bin rust-sandbox
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s
     Running `target/debug/rust-sandbox`
Some(Instant { t: 1626150136729694 })
Some(Instant { t: 1626150227376989 })
Some(Instant { t: 1626150233594279 })
Some(Instant { t: 1626150302349265 })
Some(Instant { t: 1626150307412494 })
Some(Instant { t: 1626150312973065 })
Some(Instant { t: 1626150331934881 })
Some(Instant { t: 1626150360073601 })

[alice-i-cecile]

Another sub-bug of #1120 :(

[mockersf]

This... works as expected.
add_state is adding the make_driver system set:

bevy/crates/bevy_app/src/app_builder.rs

Lines 180 to 183 in 284889c

	pub fn add_state<T: Component + Clone + Eq>(&mut self, initial: T) -> &mut Self {
	self.insert_resource(State::new(initial))
	.add_system_set(State::<T>::make_driver())
	}

which must be added first, as stated by its doc:

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

Lines 189 to 192 in 284889c

	/// Creates a driver set for the State.
	///
	/// Important note: this set must be inserted **before** all other state-dependant sets to work properly!
	pub fn make_driver() -> SystemSet {

[rparrett]

And also seemingly unavoidable: #1424 (comment)

[vkaverin]

make_driver is not used directly and looks like implementation details, so it's quite unobvious that it's necessary to check its docs while add_state's one tells nothing about this implicit dependency. It worth to add the doc to add_state method as more high level API as well at least.

But running the system twice is still not expected behavior regardless the docs.

[cart]

This is a product of SystemSet run criteria evaluation order. I think providing some configuration for SystemSet insertion position is probably a nice quick solution to this. All we really need is for the driver system set to be inserted at the front by default and normal sets to be inserted at the back by default.

This is slightly complicated by the fact that index 0 is reserved for the "default" system set (which always exists). But we can make "front" insertion just happen at index 1 instead.

We could even make this configuration private (for now) so users can't accidentally insert a SystemSet at the front with state run criteria. This would also buy us time to design the "final" system set order api.

I'm making these changes now, but feel free to discuss alternatives / pick this apart.

[cart]

This isn't quite as simple as I made it out to be because of the way we convert SystemSets (the "configuration") to virtual_system_set, parallel_systems, and exclusive_system lists (the "runtime state"). Upon insertion we store an index in each system container to the virtual_system_set index.

This creates a situation where re-ordering virtual_system_sets invalidates all stored indices (which could happen at any point during app building).

We'll either need to "fix" these indices when re-ordering occurs (or at some point before startup), use non-index ids to correlate, or change the lifecycle to ensure that when we populate the system container lists, the virtual system set indices are already stable.

@Ratysz would any of the above changes mess with your planned ecs work? Do you have an opinion here?

[Ratysz]

#1675 (the aforementioned "planned ECS work") does some of these changes itself and will include a way to specify run criteria evaluation order, so it should resolve this issue.