Implement SystemCondition for systems returning Result<bool, BevyError> and Result<(), BevyError> (#19553)

# Objective

Fixes #19403
As described in the issue, the objective is to support the use of
systems returning `Result<(), BevyError>` and
`Result<bool, BevyError>` as run conditions. In these cases, the run
condition would hold on `Ok(())` and `Ok(true)` respectively.


## Solution

`IntoSystem<In, bool, M>` cannot be implemented for systems returning
`Result<(), BevyError>` and `Result<bool, BevyError>` as that would
conflict with their trivial implementation of the trait. That led me to
add a method to the sealed trait `SystemCondition` that does the
conversion. In the original case of a system returning `bool`, the
system is returned as is. With the new types, the system is combined
with `map()` to obtain a `bool`.

By the way, I'm confused as to why `SystemCondition` has a generic `In`
parameter as it is only ever used with `In = ()` as far as I can tell.

## Testing

I added a simple test for both type of system. That's minimal but it
felt enough. I could not picture the more complicated tests passing for
a run condition returning `bool` and failing for the new types.

## Doc

I documenting the change on the page of the trait. I had trouble wording
it right but I'm not sure how to improve it. The phrasing "the condition
returns `true`" which reads naturally is now technically incorrect as
the new types return a `Result`. However, the underlying condition
system that the implementing system turns into does indeed return
`bool`. But talking about the implementation details felt too much.
Another possibility is to use another turn of phrase like "the condition
holds" or "the condition checks out". I've left "the condition returns
`true`" in the documentation of `run_if` and the provided methods for
now.

I'm perplexed about the examples. In the first one, why not implement
the condition directly instead of having a system returning it? Is it
from a time of Bevy where you had to implement your conditions that way?
In that case maybe that should be updated. And in the second example I'm
missing the point entirely. As I stated above, I've only seen conditions
used in contexts where they have no input parameter. Here we create a
condition with an input parameter (cannot be used by `run_if`) and we
are using it with `pipe()` which actually doesn't need our system to
implement `SystemCondition`. Both examples are also calling
`IntoSystem::into_system` which should not be encouraged. What am I
missing?

Author: gwafotapa

crates/bevy_ecs/src/schedule/condition.rs

@@ -11,8 +11,18 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
 
 /// A system that determines if one or more scheduled systems should run.
 ///
-/// Implemented for functions and closures that convert into [`System<Out=bool>`](System)
-/// with [read-only](crate::system::ReadOnlySystemParam) parameters.
+/// `SystemCondition` is sealed and implemented for functions and closures with
+/// [read-only](crate::system::ReadOnlySystemParam) parameters that convert into
+/// [`System<Out = bool>`](System), [`System<Out = Result<(), BevyError>>`](System) or
+/// [`System<Out = Result<bool, BevyError>>`](System).
+///
+/// `SystemCondition` offers a private method
+/// (called by [`run_if`](crate::schedule::IntoScheduleConfigs::run_if) and the provided methods)
+/// that converts the implementing system into a condition (system) returning a bool.
+/// Depending on the output type of the implementing system:
+/// - `bool`: the implementing system is used as the condition;
+/// - `Result<(), BevyError>`: the condition returns `true` if and only if the implementing system returns `Ok(())`;
+/// - `Result<bool, BevyError>`: the condition returns `true` if and only if the implementing system returns `Ok(true)`.
 ///
 /// # Marker type parameter
 ///
@@ -31,7 +41,7 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
 /// ```
 ///
 /// # Examples
-/// A condition that returns true every other time it's called.
+/// A condition that returns `true` every other time it's called.
 /// ```
 /// # use bevy_ecs::prelude::*;
 /// fn every_other_time() -> impl SystemCondition<()> {
@@ -54,7 +64,7 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
 /// # assert!(!world.resource::<DidRun>().0);
 /// ```
 ///
-/// A condition that takes a bool as an input and returns it unchanged.
+/// A condition that takes a `bool` as an input and returns it unchanged.
 ///
 /// ```
 /// # use bevy_ecs::prelude::*;
@@ -71,8 +81,30 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
 /// # world.insert_resource(DidRun(false));
 /// # app.run(&mut world);
 /// # assert!(world.resource::<DidRun>().0);
-pub trait SystemCondition<Marker, In: SystemInput = ()>:
-    sealed::SystemCondition<Marker, In>
+/// ```
+///
+/// A condition returning a `Result<(), BevyError>`
+///
+/// ```
+/// # use bevy_ecs::prelude::*;
+/// # #[derive(Component)] struct Player;
+/// fn player_exists(q_player: Query<(), With<Player>>) -> Result {
+///     Ok(q_player.single()?)
+/// }
+///
+/// # let mut app = Schedule::default();
+/// # #[derive(Resource)] struct DidRun(bool);
+/// # fn my_system(mut did_run: ResMut<DidRun>) { did_run.0 = true; }
+/// app.add_systems(my_system.run_if(player_exists));
+/// # let mut world = World::new();
+/// # world.insert_resource(DidRun(false));
+/// # app.run(&mut world);
+/// # assert!(!world.resource::<DidRun>().0);
+/// # world.spawn(Player);
+/// # app.run(&mut world);
+/// # assert!(world.resource::<DidRun>().0);
+pub trait SystemCondition<Marker, In: SystemInput = (), Out = bool>:
+    sealed::SystemCondition<Marker, In, Out>
 {
     /// Returns a new run condition that only returns `true`
     /// if both this one and the passed `and` return `true`.
@@ -371,28 +403,61 @@ pub trait SystemCondition<Marker, In: SystemInput = ()>:
     }
 }
 
-impl<Marker, In: SystemInput, F> SystemCondition<Marker, In> for F where
-    F: sealed::SystemCondition<Marker, In>
+impl<Marker, In: SystemInput, Out, F> SystemCondition<Marker, In, Out> for F where
+    F: sealed::SystemCondition<Marker, In, Out>
 {
 }
 
 mod sealed {
-    use crate::system::{IntoSystem, ReadOnlySystem, SystemInput};
+    use crate::{
+        error::BevyError,
+        system::{IntoSystem, ReadOnlySystem, SystemInput},
+    };
 
-    pub trait SystemCondition<Marker, In: SystemInput>:
-        IntoSystem<In, bool, Marker, System = Self::ReadOnlySystem>
+    pub trait SystemCondition<Marker, In: SystemInput, Out>:
+        IntoSystem<In, Out, Marker, System = Self::ReadOnlySystem>
     {
         // This associated type is necessary to let the compiler
         // know that `Self::System` is `ReadOnlySystem`.
-        type ReadOnlySystem: ReadOnlySystem<In = In, Out = bool>;
+        type ReadOnlySystem: ReadOnlySystem<In = In, Out = Out>;
+
+        fn into_condition_system(self) -> impl ReadOnlySystem<In = In, Out = bool>;
     }
 
-    impl<Marker, In: SystemInput, F> SystemCondition<Marker, In> for F
+    impl<Marker, In: SystemInput, F> SystemCondition<Marker, In, bool> for F
     where
         F: IntoSystem<In, bool, Marker>,
         F::System: ReadOnlySystem,
     {
         type ReadOnlySystem = F::System;
+
+        fn into_condition_system(self) -> impl ReadOnlySystem<In = In, Out = bool> {
+            IntoSystem::into_system(self)
+        }
+    }
+
+    impl<Marker, In: SystemInput, F> SystemCondition<Marker, In, Result<(), BevyError>> for F
+    where
+        F: IntoSystem<In, Result<(), BevyError>, Marker>,
+        F::System: ReadOnlySystem,
+    {
+        type ReadOnlySystem = F::System;
+
+        fn into_condition_system(self) -> impl ReadOnlySystem<In = In, Out = bool> {
+            IntoSystem::into_system(self.map(|result| result.is_ok()))
+        }
+    }
+
+    impl<Marker, In: SystemInput, F> SystemCondition<Marker, In, Result<bool, BevyError>> for F
+    where
+        F: IntoSystem<In, Result<bool, BevyError>, Marker>,
+        F::System: ReadOnlySystem,
+    {
+        type ReadOnlySystem = F::System;
+
+        fn into_condition_system(self) -> impl ReadOnlySystem<In = In, Out = bool> {
+            IntoSystem::into_system(self.map(|result| matches!(result, Ok(true))))
+        }
     }
 }
 

crates/bevy_ecs/src/schedule/config.rs

@@ -14,8 +14,8 @@ use crate::{
     system::{BoxedSystem, InfallibleSystemWrapper, IntoSystem, ScheduleSystem, System},
 };
 
-fn new_condition<M>(condition: impl SystemCondition<M>) -> BoxedCondition {
-    let condition_system = IntoSystem::into_system(condition);
+fn new_condition<M, Out>(condition: impl SystemCondition<M, (), Out>) -> BoxedCondition {
+    let condition_system = condition.into_condition_system();
     assert!(
         condition_system.is_send(),
         "SystemCondition `{}` accesses `NonSend` resources. This is not currently supported.",
@@ -447,7 +447,7 @@ pub trait IntoScheduleConfigs<T: Schedulable<Metadata = GraphInfo, GroupMetadata
     ///
     /// Use [`distributive_run_if`](IntoScheduleConfigs::distributive_run_if) if you want the
     /// condition to be evaluated for each individual system, right before one is run.
-    fn run_if<M>(self, condition: impl SystemCondition<M>) -> ScheduleConfigs<T> {
+    fn run_if<M, Out>(self, condition: impl SystemCondition<M, (), Out>) -> ScheduleConfigs<T> {
         self.into_configs().run_if(condition)
     }
 
@@ -535,7 +535,7 @@ impl<T: Schedulable<Metadata = GraphInfo, GroupMetadata = Chain>> IntoScheduleCo
         self
     }
 
-    fn run_if<M>(mut self, condition: impl SystemCondition<M>) -> ScheduleConfigs<T> {
+    fn run_if<M, Out>(mut self, condition: impl SystemCondition<M, (), Out>) -> ScheduleConfigs<T> {
         self.run_if_dyn(new_condition(condition));
         self
     }

crates/bevy_ecs/src/schedule/mod.rs

@@ -29,6 +29,7 @@ mod tests {
     use alloc::{string::ToString, vec, vec::Vec};
     use core::sync::atomic::{AtomicU32, Ordering};
 
+    use crate::error::BevyError;
     pub use crate::{
         prelude::World,
         resource::Resource,
@@ -49,10 +50,10 @@ mod tests {
     struct SystemOrder(Vec<u32>);
 
     #[derive(Resource, Default)]
-    struct RunConditionBool(pub bool);
+    struct RunConditionBool(bool);
 
     #[derive(Resource, Default)]
-    struct Counter(pub AtomicU32);
+    struct Counter(AtomicU32);
 
     fn make_exclusive_system(tag: u32) -> impl FnMut(&mut World) {
         move |world| world.resource_mut::<SystemOrder>().0.push(tag)
@@ -252,12 +253,13 @@ mod tests {
     }
 
     mod conditions {
+
         use crate::change_detection::DetectChanges;
 
         use super::*;
 
         #[test]
-        fn system_with_condition() {
+        fn system_with_condition_bool() {
             let mut world = World::default();
             let mut schedule = Schedule::default();
 
@@ -276,6 +278,47 @@ mod tests {
             assert_eq!(world.resource::<SystemOrder>().0, vec![0]);
         }
 
+        #[test]
+        fn system_with_condition_result_unit() {
+            let mut world = World::default();
+            let mut schedule = Schedule::default();
+
+            world.init_resource::<SystemOrder>();
+
+            schedule.add_systems(
+                make_function_system(0).run_if(|| Err::<(), BevyError>(core::fmt::Error.into())),
+            );
+
+            schedule.run(&mut world);
+            assert_eq!(world.resource::<SystemOrder>().0, vec![]);
+
+            schedule.add_systems(make_function_system(1).run_if(|| Ok(())));
+
+            schedule.run(&mut world);
+            assert_eq!(world.resource::<SystemOrder>().0, vec![1]);
+        }
+
+        #[test]
+        fn system_with_condition_result_bool() {
+            let mut world = World::default();
+            let mut schedule = Schedule::default();
+
+            world.init_resource::<SystemOrder>();
+
+            schedule.add_systems((
+                make_function_system(0).run_if(|| Err::<bool, BevyError>(core::fmt::Error.into())),
+                make_function_system(1).run_if(|| Ok(false)),
+            ));
+
+            schedule.run(&mut world);
+            assert_eq!(world.resource::<SystemOrder>().0, vec![]);
+
+            schedule.add_systems(make_function_system(2).run_if(|| Ok(true)));
+
+            schedule.run(&mut world);
+            assert_eq!(world.resource::<SystemOrder>().0, vec![2]);
+        }
+
         #[test]
         fn systems_with_distributive_condition() {
             let mut world = World::default();