diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index c6a5c99b8cc68..f9963a5200bc7 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -1133,7 +1133,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(a(input, data)? && b(input, data)?) + Ok(a(input, data).unwrap_or(false) && b(input, data).unwrap_or(false)) } } @@ -1155,7 +1155,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(!(a(input, data)? && b(input, data)?)) + Ok(!(a(input, data).unwrap_or(false) && b(input, data).unwrap_or(false))) } } @@ -1177,7 +1177,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(!(a(input, data)? || b(input, data)?)) + Ok(!(a(input, data).unwrap_or(false) || b(input, data).unwrap_or(false))) } } @@ -1199,7 +1199,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(a(input, data)? || b(input, data)?) + Ok(a(input, data).unwrap_or(false) || b(input, data).unwrap_or(false)) } } @@ -1221,7 +1221,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(!(a(input, data)? ^ b(input, data)?)) + Ok(!(a(input, data).unwrap_or(false) ^ b(input, data).unwrap_or(false))) } } @@ -1243,7 +1243,7 @@ where a: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, b: impl FnOnce(SystemIn<'_, A>, &mut T) -> Result, ) -> Result { - Ok(a(input, data)? ^ b(input, data)?) + Ok(a(input, data).unwrap_or(false) ^ b(input, data).unwrap_or(false)) } } @@ -1252,12 +1252,12 @@ mod tests { use super::{common_conditions::*, SystemCondition}; use crate::error::{BevyError, DefaultErrorHandler, ErrorContext}; use crate::{ - change_detection::ResMut, + change_detection::{Res, ResMut}, component::Component, message::Message, query::With, schedule::{IntoScheduleConfigs, Schedule}, - system::{IntoSystem, Local, Res, System}, + system::{IntoSystem, Local, System}, world::World, }; use bevy_ecs_macros::{Resource, SystemSet}; @@ -1305,6 +1305,252 @@ mod tests { assert_eq!(world.resource::().0, 6); } + #[test] + fn combinators_with_maybe_failing_condition() { + #![allow( + clippy::nonminimal_bool, + clippy::overly_complex_bool_expr, + reason = "Trailing `|| false` and `&& true` are used in this test to visually remain consistent with the combinators" + )] + + use crate::system::RunSystemOnce; + use alloc::sync::Arc; + use std::sync::Mutex; + + // Things that should be tested: + // - the final result of the combinator is correct + // - the systems that are expected to run do run + // - the systems that are expected to not run do not run + + #[derive(Component)] + struct Vacant; + + // SystemConditions don't have mutable access to the world, so we use a + // `Res` to count invocations. + #[derive(Resource, Default)] + struct Counter(Arc>); + + // The following constants are used to represent a system having run. + // both are prime so that when multiplied they give a unique value for any TRUE^n*FALSE^m + const FALSE: usize = 2; + const TRUE: usize = 3; + + // this is a system, but has the same side effect as `test_true` + fn is_true_inc(counter: Res) -> bool { + test_true(&counter) + } + + // this is a system, but has the same side effect as `test_false` + fn is_false_inc(counter: Res) -> bool { + test_false(&counter) + } + + // This condition will always yield `false`, because `Vacant` is never present. + fn vacant(_: crate::system::Single<&Vacant>) -> bool { + true + } + + fn test_true(counter: &Counter) -> bool { + *counter.0.lock().unwrap() *= TRUE; + true + } + + fn test_false(counter: &Counter) -> bool { + *counter.0.lock().unwrap() *= FALSE; + false + } + + // Helper function that runs a logic call and returns the result, as + // well as the composite number of the calls. + fn logic_call_result(f: impl FnOnce(&Counter) -> bool) -> (usize, bool) { + let counter = Counter(Arc::new(Mutex::new(1))); + let result = f(&counter); + (*counter.0.lock().unwrap(), result) + } + + // `test_true` and `test_false` can't fail like the systems can, and so + // we use them to model the short circuiting behavior of rust's logical + // operators. The goal is to end up with a composite number that + // describes rust's behavior and compare that to the result of the + // combinators. + + // we expect `true() || false()` to yield `true`, and short circuit + // after `true()` + assert_eq!( + logic_call_result(|c| test_true(c) || test_false(c)), + (TRUE.pow(1) * FALSE.pow(0), true) + ); + + let mut world = World::new(); + world.init_resource::(); + + // ensure there are no `Vacant` entities + assert!(world.query::<&Vacant>().iter(&world).next().is_none()); + assert!(matches!( + world.run_system_once((|| true).or(vacant)), + Ok(true) + )); + + // This system should fail + assert!(RunSystemOnce::run_system_once(&mut world, vacant).is_err()); + + #[track_caller] + fn assert_system( + world: &mut World, + system: impl IntoSystem<(), bool, Marker>, + equivalent_to: impl FnOnce(&Counter) -> bool, + ) { + use crate::system::System; + + *world.resource::().0.lock().unwrap() = 1; + + let system = IntoSystem::into_system(system); + let name = system.name(); + + let out = RunSystemOnce::run_system_once(&mut *world, system).unwrap_or(false); + + let (expected_counter, expected) = logic_call_result(equivalent_to); + let caller = core::panic::Location::caller(); + let counter = *world.resource::().0.lock().unwrap(); + + assert_eq!( + out, + expected, + "At {}:{} System `{name}` yielded unexpected value `{out}`, expected `{expected}`", + caller.file(), + caller.line(), + ); + + assert_eq!( + counter, expected_counter, + "At {}:{} System `{name}` did not increment counter as expected: expected `{expected_counter}`, got `{counter}`", + caller.file(), + caller.line(), + ); + } + + assert_system(&mut world, is_true_inc.or(vacant), |c| { + test_true(c) || false + }); + assert_system(&mut world, is_true_inc.nor(vacant), |c| { + !(test_true(c) || false) + }); + assert_system(&mut world, is_true_inc.xor(vacant), |c| { + test_true(c) ^ false + }); + assert_system(&mut world, is_true_inc.xnor(vacant), |c| { + !(test_true(c) ^ false) + }); + assert_system(&mut world, is_true_inc.and(vacant), |c| { + test_true(c) && false + }); + assert_system(&mut world, is_true_inc.nand(vacant), |c| { + !(test_true(c) && false) + }); + + // even if `vacant` fails as the first condition, where applicable (or, + // xor), `is_true_inc` should still be called. `and` and `nand` short + // circuit on an initial `false`. + assert_system(&mut world, vacant.or(is_true_inc), |c| { + false || test_true(c) + }); + assert_system(&mut world, vacant.nor(is_true_inc), |c| { + !(false || test_true(c)) + }); + assert_system(&mut world, vacant.xor(is_true_inc), |c| { + false ^ test_true(c) + }); + assert_system(&mut world, vacant.xnor(is_true_inc), |c| { + !(false ^ test_true(c)) + }); + assert_system(&mut world, vacant.and(is_true_inc), |c| { + false && test_true(c) + }); + assert_system(&mut world, vacant.nand(is_true_inc), |c| { + !(false && test_true(c)) + }); + + // the same logic ought to be the case with a condition that runs, but yields `false`: + assert_system(&mut world, is_true_inc.or(is_false_inc), |c| { + test_true(c) || test_false(c) + }); + assert_system(&mut world, is_true_inc.nor(is_false_inc), |c| { + !(test_true(c) || test_false(c)) + }); + assert_system(&mut world, is_true_inc.xor(is_false_inc), |c| { + test_true(c) ^ test_false(c) + }); + assert_system(&mut world, is_true_inc.xnor(is_false_inc), |c| { + !(test_true(c) ^ test_false(c)) + }); + assert_system(&mut world, is_true_inc.and(is_false_inc), |c| { + test_true(c) && test_false(c) + }); + assert_system(&mut world, is_true_inc.nand(is_false_inc), |c| { + !(test_true(c) && test_false(c)) + }); + + // and where one condition yields `false` and the other fails: + assert_system(&mut world, is_false_inc.or(vacant), |c| { + test_false(c) || false + }); + assert_system(&mut world, is_false_inc.nor(vacant), |c| { + !(test_false(c) || false) + }); + assert_system(&mut world, is_false_inc.xor(vacant), |c| { + test_false(c) ^ false + }); + assert_system(&mut world, is_false_inc.xnor(vacant), |c| { + !(test_false(c) ^ false) + }); + assert_system(&mut world, is_false_inc.and(vacant), |c| { + test_false(c) && false + }); + assert_system(&mut world, is_false_inc.nand(vacant), |c| { + !(test_false(c) && false) + }); + + // and where both conditions yield `true`: + assert_system(&mut world, is_true_inc.or(is_true_inc), |c| { + test_true(c) || test_true(c) + }); + assert_system(&mut world, is_true_inc.nor(is_true_inc), |c| { + !(test_true(c) || test_true(c)) + }); + assert_system(&mut world, is_true_inc.xor(is_true_inc), |c| { + test_true(c) ^ test_true(c) + }); + assert_system(&mut world, is_true_inc.xnor(is_true_inc), |c| { + !(test_true(c) ^ test_true(c)) + }); + assert_system(&mut world, is_true_inc.and(is_true_inc), |c| { + test_true(c) && test_true(c) + }); + assert_system(&mut world, is_true_inc.nand(is_true_inc), |c| { + !(test_true(c) && test_true(c)) + }); + + // and where both conditions yield `false`: + assert_system(&mut world, is_false_inc.or(is_false_inc), |c| { + test_false(c) || test_false(c) + }); + assert_system(&mut world, is_false_inc.nor(is_false_inc), |c| { + !(test_false(c) || test_false(c)) + }); + assert_system(&mut world, is_false_inc.xor(is_false_inc), |c| { + test_false(c) ^ test_false(c) + }); + assert_system(&mut world, is_false_inc.xnor(is_false_inc), |c| { + !(test_false(c) ^ test_false(c)) + }); + assert_system(&mut world, is_false_inc.and(is_false_inc), |c| { + test_false(c) && test_false(c) + }); + assert_system(&mut world, is_false_inc.nand(is_false_inc), |c| { + !(test_false(c) && test_false(c)) + }); + } + #[test] fn run_condition_combinators() { let mut world = World::new(); diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index a12b3469a4651..b05711569ea0b 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -4,6 +4,7 @@ use core::marker::PhantomData; use crate::{ change_detection::{CheckChangeTicks, Tick}, + error::ErrorContext, prelude::World, query::FilteredAccessSet, schedule::InternedSystemSet, @@ -157,6 +158,42 @@ where ) -> Result { struct PrivateUnsafeWorldCell<'w>(UnsafeWorldCell<'w>); + // Since control over handling system run errors is passed on to the + // implementation of `Func::combine`, which may run the two closures + // however it wants, errors must be intercepted here if they should be + // handled by the world's error handler. + unsafe fn run_system( + system: &mut S, + input: SystemIn, + world: &mut PrivateUnsafeWorldCell, + ) -> Result { + // SAFETY: see comment on `Func::combine` call + match (|| unsafe { + system.validate_param_unsafe(world.0)?; + system.run_unsafe(input, world.0) + })() { + Err(RunSystemError::Failed(err)) => { + // let the world's default error handler handle the error if `Failed(_)` + (world.0.default_error_handler())( + err, + ErrorContext::System { + name: system.name(), + last_run: system.get_last_run(), + }, + ); + + // Since the error handler takes the error by value, create a new error: + // The original error has already been handled, including + // the reason for the failure here isn't important. + Err(format!("System `{}` failed", system.name()).into()) + } + // `Skipped(_)` and `Ok(_)` are passed through: + // system skipping is not an error, and isn't passed to the + // world's error handler by the executors. + result @ (Ok(_) | Err(RunSystemError::Skipped(_))) => result, + } + } + Func::combine( input, &mut PrivateUnsafeWorldCell(world), @@ -167,14 +204,9 @@ where // passed to a function as an unbound non-'static generic argument, they can never be called in parallel // or re-entrantly because that would require forging another instance of `PrivateUnsafeWorldCell`. // This means that the world accesses in the two closures will not conflict with each other. - |input, world| unsafe { self.a.run_unsafe(input, world.0) }, - // `Self::validate_param_unsafe` already validated the first system, - // but we still need to validate the second system once the first one runs. + |input, world| unsafe { run_system(&mut self.a, input, world) }, // SAFETY: See the comment above. - |input, world| unsafe { - self.b.validate_param_unsafe(world.0)?; - self.b.run_unsafe(input, world.0) - }, + |input, world| unsafe { run_system(&mut self.b, input, world) }, ) } @@ -200,14 +232,12 @@ where #[inline] unsafe fn validate_param_unsafe( &mut self, - world: UnsafeWorldCell, + _world: UnsafeWorldCell, ) -> Result<(), SystemParamValidationError> { - // We only validate parameters for the first system, - // since it may make changes to the world that affect - // whether the second system has valid parameters. - // The second system will be validated in `Self::run_unsafe`. - // SAFETY: Delegate to other `System` implementations. - unsafe { self.a.validate_param_unsafe(world) } + // Both systems are validated in `Self::run_unsafe`, so that we get the + // chance to run the second system even if the first one fails to + // validate. + Ok(()) } fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { diff --git a/release-content/migration-guides/combinator_system.md b/release-content/migration-guides/combinator_system.md new file mode 100644 index 0000000000000..7af01c344e41c --- /dev/null +++ b/release-content/migration-guides/combinator_system.md @@ -0,0 +1,38 @@ +--- +title: System Combinators +pull_requests: [20671] +--- + +The `CombinatorSystem`s can be used to combine multiple `SystemCondition`s with logical operators. Previously, the conditions would short circuit if the system failed to run, for example because it's query could not be filled by the world. + +Now, the `CombinatorSystem`s will work as expected, following the semantics of rust's logical operators. +Namely, if a `SystemCondition` fails, it will be considered to have returned `false` and in combinators that don't short circuit the other condition will now be run. + +Specifically, the combinators act as follows: + +| Combinator | Rust Equivalent | +|:----------:|:---------------:| +| `and` | `a && b` | +| `or` | `a \|\| b` | +| `xor` | `a ^ b` | +| `nand` | `!(a && b)` | +| `nor` | `!(a \|\| b)` | +| `xnor` | `!(a ^ b)` | + +```rust +fn vacant(_: crate::system::Single<&Vacant>) -> bool { + true +} + +fn is_true() -> bool { + true +} + +assert!(world.query::<&Vacant>().iter(&world).next().is_none()); + +// previously: +assert!(world.run_system_once(is_true.or(vacant)).is_err()); + +// now: +assert!(matches!(world.run_system_once(is_true.or(vacant)), Ok(true))); +```