Fixed criteria-less systems being re-ran unnecessarily (#1754)

Fixes #1753.

The problem was introduced while reworking the logic around stages' own criteria. Before #1675 they used to be stored and processed inline with the systems' criteria, and systems without criteria used that of their stage. After, criteria-less systems think they should run, always. This PR more or less restores previous behavior; a less cludge solution can wait until after 0.5 - ideally, until stageless.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>

crates/bevy_ecs/src/schedule/stage.rs

@@ -747,12 +747,6 @@ impl Stage for SystemStage {
             self.world_id = Some(world.id());
         }
 
-        if let ShouldRun::No | ShouldRun::NoAndCheckAgain =
-            self.stage_run_criteria.should_run(world)
-        {
-            return;
-        }
-
         if self.systems_modified {
             self.initialize_systems(world);
             self.rebuild_orders_and_dependencies();
@@ -767,101 +761,127 @@ impl Stage for SystemStage {
             self.executor_modified = false;
         }
 
-        // Evaluate run criteria.
+        let mut run_stage_loop = true;
-        for index in 0..self.run_criteria.len() {
+        while run_stage_loop {
-            let (run_criteria, tail) = self.run_criteria.split_at_mut(index);
+            let should_run = self.stage_run_criteria.should_run(world);
-            let mut criteria = &mut tail[0];
+            match should_run {
-            match &mut criteria.inner {
+                ShouldRun::No => return,
-                RunCriteriaInner::Single(system) => criteria.should_run = system.run((), world),
+                ShouldRun::NoAndCheckAgain => continue,
-                RunCriteriaInner::Piped {
+                ShouldRun::YesAndCheckAgain => (),
-                    input: parent,
+                ShouldRun::Yes => {
-                    system,
+                    run_stage_loop = false;
-                    ..
+                }
-                } => criteria.should_run = system.run(run_criteria[*parent].should_run, world),
+            };
-            }
-        }
 
-        let mut has_work = true;
+            // Evaluate system run criteria.
-        while has_work {
+            for index in 0..self.run_criteria.len() {
-            fn should_run(
+                let (run_criteria, tail) = self.run_criteria.split_at_mut(index);
-                container: &impl SystemContainer,
+                let mut criteria = &mut tail[0];
-                run_criteria: &[RunCriteriaContainer],
+                match &mut criteria.inner {
-            ) -> bool {
+                    RunCriteriaInner::Single(system) => criteria.should_run = system.run((), world),
-                matches!(
+                    RunCriteriaInner::Piped {
-                    container
+                        input: parent,
-                        .run_criteria()
+                        system,
-                        .map(|index| run_criteria[index].should_run),
+                        ..
-                    None | Some(ShouldRun::Yes) | Some(ShouldRun::YesAndCheckAgain)
+                    } => criteria.should_run = system.run(run_criteria[*parent].should_run, world),
-                )
-            }
-
-            // Run systems that want to be at the start of stage.
-            for container in &mut self.exclusive_at_start {
-                if should_run(container, &self.run_criteria) {
-                    container.system_mut().run(world);
                 }
             }
 
-            // Run parallel systems using the executor.
+            let mut run_system_loop = true;
-            // TODO: hard dependencies, nested sets, whatever... should be evaluated here.
+            let mut default_should_run = ShouldRun::Yes;
-            for container in &mut self.parallel {
+            while run_system_loop {
-                container.should_run = should_run(container, &self.run_criteria);
+                run_system_loop = false;
-            }
-            self.executor.run_systems(&mut self.parallel, world);
 
-            // Run systems that want to be between parallel systems and their command buffers.
+                fn should_run(
-            for container in &mut self.exclusive_before_commands {
+                    container: &impl SystemContainer,
-                if should_run(container, &self.run_criteria) {
+                    run_criteria: &[RunCriteriaContainer],
-                    container.system_mut().run(world);
+                    default: ShouldRun,
+                ) -> bool {
+                    matches!(
+                        container
+                            .run_criteria()
+                            .map(|index| run_criteria[index].should_run)
+                            .unwrap_or(default),
+                        ShouldRun::Yes | ShouldRun::YesAndCheckAgain
+                    )
                 }
-            }
 
-            // Apply parallel systems' buffers.
+                // Run systems that want to be at the start of stage.
-            for container in &mut self.parallel {
+                for container in &mut self.exclusive_at_start {
-                if container.should_run {
+                    if should_run(container, &self.run_criteria, default_should_run) {
-                    container.system_mut().apply_buffers(world);
+                        container.system_mut().run(world);
+                    }
                 }
-            }
 
-            // Run systems that want to be at the end of stage.
+                // Run parallel systems using the executor.
-            for container in &mut self.exclusive_at_end {
+                // TODO: hard dependencies, nested sets, whatever... should be evaluated here.
-                if should_run(container, &self.run_criteria) {
+                for container in &mut self.parallel {
-                    container.system_mut().run(world);
+                    container.should_run =
+                        should_run(container, &self.run_criteria, default_should_run);
                 }
-            }
+                self.executor.run_systems(&mut self.parallel, world);
 
-            // Check for old component and system change ticks
+                // Run systems that want to be between parallel systems and their command buffers.
-            self.check_change_ticks(world);
+                for container in &mut self.exclusive_before_commands {
+                    if should_run(container, &self.run_criteria, default_should_run) {
+                        container.system_mut().run(world);
+                    }
+                }
 
-            // Reevaluate run criteria.
+                // Apply parallel systems' buffers.
-            has_work = false;
+                for container in &mut self.parallel {
-            let run_criteria = &mut self.run_criteria;
+                    if container.should_run {
-            for index in 0..run_criteria.len() {
+                        container.system_mut().apply_buffers(world);
-                let (run_criteria, tail) = run_criteria.split_at_mut(index);
+                    }
-                let criteria = &mut tail[0];
+                }
-                match criteria.should_run {
+
-                    ShouldRun::No => (),
+                // Run systems that want to be at the end of stage.
-                    ShouldRun::Yes => criteria.should_run = ShouldRun::No,
+                for container in &mut self.exclusive_at_end {
-                    ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
+                    if should_run(container, &self.run_criteria, default_should_run) {
-                        match &mut criteria.inner {
+                        container.system_mut().run(world);
-                            RunCriteriaInner::Single(system) => {
+                    }
-                                criteria.should_run = system.run((), world)
+                }
+
+                // Check for old component and system change ticks
+                self.check_change_ticks(world);
+
+                // Evaluate run criteria.
+                let run_criteria = &mut self.run_criteria;
+                for index in 0..run_criteria.len() {
+                    let (run_criteria, tail) = run_criteria.split_at_mut(index);
+                    let criteria = &mut tail[0];
+                    match criteria.should_run {
+                        ShouldRun::No => (),
+                        ShouldRun::Yes => criteria.should_run = ShouldRun::No,
+                        ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
+                            match &mut criteria.inner {
+                                RunCriteriaInner::Single(system) => {
+                                    criteria.should_run = system.run((), world)
+                                }
+                                RunCriteriaInner::Piped {
+                                    input: parent,
+                                    system,
+                                    ..
+                                } => {
+                                    criteria.should_run =
+                                        system.run(run_criteria[*parent].should_run, world)
+                                }
                             }
-                            RunCriteriaInner::Piped {
+                            match criteria.should_run {
-                                input: parent,
+                                ShouldRun::Yes => {
-                                system,
+                                    run_system_loop = true;
-                                ..
+                                }
-                            } => {
+                                ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
-                                criteria.should_run =
+                                    run_system_loop = true;
-                                    system.run(run_criteria[*parent].should_run, world)
+                                }
+                                ShouldRun::No => (),
                             }
                         }
-                        match criteria.should_run {
-                            ShouldRun::Yes | ShouldRun::YesAndCheckAgain => has_work = true,
-                            ShouldRun::No | ShouldRun::NoAndCheckAgain => (),
-                        }
                     }
                 }
+
+                // after the first loop, default to not running systems without run criteria
+                default_should_run = ShouldRun::No;
             }
         }
     }

crates/bevy_ecs/src/schedule/state.rs

@@ -652,4 +652,33 @@ mod test {
             &MyState::Final
         );
     }
+
+    #[test]
+    fn issue_1753() {
+        #[derive(Clone, PartialEq, Eq, Debug, Hash)]
+        enum AppState {
+            Main,
+        }
+
+        fn should_run_once(mut flag: ResMut<bool>, test_name: Res<&'static str>) {
+            assert!(!*flag, "{:?}", *test_name);
+            *flag = true;
+        }
+
+        let mut world = World::new();
+        world.insert_resource(State::new(AppState::Main));
+        world.insert_resource(false);
+        world.insert_resource("control");
+        let mut stage = SystemStage::parallel().with_system(should_run_once.system());
+        stage.run(&mut world);
+        assert!(*world.get_resource::<bool>().unwrap(), "after control");
+
+        world.insert_resource(false);
+        world.insert_resource("test");
+        let mut stage = SystemStage::parallel()
+            .with_system_set(State::<AppState>::get_driver())
+            .with_system(should_run_once.system());
+        stage.run(&mut world);
+        assert!(*world.get_resource::<bool>().unwrap(), "after test");
+    }
 }