From 0e805eb49c89cf7ff8ff047fe5e3bff2f2352b8b Mon Sep 17 00:00:00 2001 From: Guillaume Wafo-Tapa <43912646+gwafotapa@users.noreply.github.com> Date: Tue, 10 Jun 2025 02:04:10 +0200 Subject: [PATCH] Implement SystemCondition for systems returning Result 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` as run conditions. In these cases, the run condition would hold on `Ok(())` and `Ok(true)` respectively. ## Solution `IntoSystem` cannot be implemented for systems returning `Result<(), BevyError>` and `Result` 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? --- crates/bevy_ecs/src/schedule/condition.rs | 91 +++++++++++++++++++---- crates/bevy_ecs/src/schedule/config.rs | 8 +- crates/bevy_ecs/src/schedule/mod.rs | 49 +++++++++++- 3 files changed, 128 insertions(+), 20 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 2b31ad50c7..5d331f64f2 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -11,8 +11,18 @@ pub type BoxedCondition = Box>; /// A system that determines if one or more scheduled systems should run. /// -/// Implemented for functions and closures that convert into [`System`](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`](System), [`System>`](System) or +/// [`System>`](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`: the condition returns `true` if and only if the implementing system returns `Ok(true)`. /// /// # Marker type parameter /// @@ -31,7 +41,7 @@ pub type BoxedCondition = Box>; /// ``` /// /// # 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 = Box>; /// # assert!(!world.resource::().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 = Box>; /// # world.insert_resource(DidRun(false)); /// # app.run(&mut world); /// # assert!(world.resource::().0); -pub trait SystemCondition: - sealed::SystemCondition +/// ``` +/// +/// A condition returning a `Result<(), BevyError>` +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// # #[derive(Component)] struct Player; +/// fn player_exists(q_player: Query<(), With>) -> Result { +/// Ok(q_player.single()?) +/// } +/// +/// # let mut app = Schedule::default(); +/// # #[derive(Resource)] struct DidRun(bool); +/// # fn my_system(mut did_run: ResMut) { 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::().0); +/// # world.spawn(Player); +/// # app.run(&mut world); +/// # assert!(world.resource::().0); +pub trait SystemCondition: + sealed::SystemCondition { /// 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: } } -impl SystemCondition for F where - F: sealed::SystemCondition +impl SystemCondition for F where + F: sealed::SystemCondition { } mod sealed { - use crate::system::{IntoSystem, ReadOnlySystem, SystemInput}; + use crate::{ + error::BevyError, + system::{IntoSystem, ReadOnlySystem, SystemInput}, + }; - pub trait SystemCondition: - IntoSystem + pub trait SystemCondition: + IntoSystem { // This associated type is necessary to let the compiler // know that `Self::System` is `ReadOnlySystem`. - type ReadOnlySystem: ReadOnlySystem; + type ReadOnlySystem: ReadOnlySystem; + + fn into_condition_system(self) -> impl ReadOnlySystem; } - impl SystemCondition for F + impl SystemCondition for F where F: IntoSystem, F::System: ReadOnlySystem, { type ReadOnlySystem = F::System; + + fn into_condition_system(self) -> impl ReadOnlySystem { + IntoSystem::into_system(self) + } + } + + impl SystemCondition> for F + where + F: IntoSystem, Marker>, + F::System: ReadOnlySystem, + { + type ReadOnlySystem = F::System; + + fn into_condition_system(self) -> impl ReadOnlySystem { + IntoSystem::into_system(self.map(|result| result.is_ok())) + } + } + + impl SystemCondition> for F + where + F: IntoSystem, Marker>, + F::System: ReadOnlySystem, + { + type ReadOnlySystem = F::System; + + fn into_condition_system(self) -> impl ReadOnlySystem { + IntoSystem::into_system(self.map(|result| matches!(result, Ok(true)))) + } } } diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index f1a48e432b..4826d0a66d 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -14,8 +14,8 @@ use crate::{ system::{BoxedSystem, InfallibleSystemWrapper, IntoSystem, ScheduleSystem, System}, }; -fn new_condition(condition: impl SystemCondition) -> BoxedCondition { - let condition_system = IntoSystem::into_system(condition); +fn new_condition(condition: impl SystemCondition) -> 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(self, condition: impl SystemCondition) -> ScheduleConfigs { + fn run_if(self, condition: impl SystemCondition) -> ScheduleConfigs { self.into_configs().run_if(condition) } @@ -535,7 +535,7 @@ impl> IntoScheduleCo self } - fn run_if(mut self, condition: impl SystemCondition) -> ScheduleConfigs { + fn run_if(mut self, condition: impl SystemCondition) -> ScheduleConfigs { self.run_if_dyn(new_condition(condition)); self } diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index 807112f438..8c5aa1d6fb 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/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); #[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::().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::().0, vec![0]); } + #[test] + fn system_with_condition_result_unit() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_systems( + make_function_system(0).run_if(|| Err::<(), BevyError>(core::fmt::Error.into())), + ); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![]); + + schedule.add_systems(make_function_system(1).run_if(|| Ok(()))); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![1]); + } + + #[test] + fn system_with_condition_result_bool() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_systems(( + make_function_system(0).run_if(|| Err::(core::fmt::Error.into())), + make_function_system(1).run_if(|| Ok(false)), + )); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![]); + + schedule.add_systems(make_function_system(2).run_if(|| Ok(true))); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![2]); + } + #[test] fn systems_with_distributive_condition() { let mut world = World::default();