Impl system condition combinators for `Result`-ful run conditions #19580

gwafotapa · 2025-06-11T10:29:15Z

Objective

This is a follow-up of #19553.
I missed something. Right now the combinators (and, or, ...) only work with conditions returning bool. The same goes for condition_changed, condition_changed_to and not. The objective is to fix that to allow conditions returning Result<(), BevyError> and Result<bool, BevyError> to use these methods as well.

Solution

It's essentially just about calling into_condition_system() in the methods. I just have a problem for not because it's implemented differently than the others. Currently not can accept any system whose output implement Not. That's a problem for systems returning Result<(), BevyError> or Result<bool, BevyError>. I see mostly two options:

Leave things as they are and the new conditions cannot be used with not.
Limit not to accept only run conditions. This way not could accept the new run conditions but not any system whose output implements Not.

I would say the second solution has more upsides than downsides because

Use cases of not with Not feels more niche than use cases of not with the new conditions
It is more coherent (being able to use the new run conditions with run_if but not not is confusing)
To invert a system whose output implements Not you can still do system.map(|x| !x)

but I may be missing something. What do you think ?

Testing

I also added a simple test for all the missing methods.

…d Result<bool, BevyError>

alice-i-cecile · 2025-06-11T22:29:51Z

@urben1680 want to review this one too?

urben1680 · 2025-06-12T07:05:48Z

I think into_condition_system is the right approach, it aligns into how bevy turns fallible and infallible systems into the same output (Result<(), BevyError>) via the IntoScheduleConfigs impl.

Regarding the Not case I also favor the second solution. It should be consistent.

When this leaves Draft I do a proper review. 👍

…ypes

gwafotapa · 2025-06-13T08:01:06Z

@urben1680 PR is ready for review.

urben1680

I would only like to see a change about the not return type becoming anonymous, the other things do not bother me enough. 😄

urben1680 · 2025-06-13T13:12:06Z

crates/bevy_ecs/src/schedule/condition.rs

+    pub fn not<Marker, In, Out, C>(
+        condition: C,
+    ) -> AdapterSystem<fn(bool) -> bool, C::ConditionSystem>
    where
-        TOut: core::ops::Not,
-        T: IntoSystem<(), TOut, Marker>,
+        In: SystemInput,
+        C: SystemCondition<Marker, In, Out>,
    {
-        let condition = IntoSystem::into_system(condition);
-        let name = format!("!{}", condition.name());
-        NotSystem::new(super::NotMarker, condition, name.into())
+        let f: fn(bool) -> bool = |x| !x;
+        IntoSystem::into_system(condition.into_condition_system().map(f))


I think we can return an impl SystemCondition<(), In> here so you don't need to turn that mapping function into a function pointer which requires slower dynamic dispatch.

Returning impl SystemCondition<(), In> caused the compiler to complain about a missing Clone bound. I finally went another way. Using the approach in NotSystem of defining a helper type, I removed the use of a function pointer. I also reworked on the implementations of SystemCondition to remove function pointers there as well.

urben1680 · 2025-06-13T13:16:24Z

crates/bevy_ecs/src/schedule/condition.rs

-/// Invokes [`Not`] with the output of another system.
-///
-/// See [`common_conditions::not`] for examples.
-pub type NotSystem<S> = AdapterSystem<NotMarker, S>;


I wonder why this explicit type was needed and why it needed to be public when all other methods returned impl ....

I think there are two alternatives here:

Write a migration guide that this type alias is gone.

Stick to the explicit type returned by not and change this to this: (maybe with a #[deprecated] as it is unused in the engine now)

type NotSystem<T, In = (), Out = bool> = AdapterSystem<fn(bool) -> bool, <T as SystemCondition<(), In, Out>>::ConditionSystem>;

I strongly prefer 1, also because I think 2 does not technically resemble the original NotSystem alias depending on how exotic the system output were with that Not impl. I am not even sure 2. works.

But I am fine with not do anything here as I assume nobody used this alias.

I don't know why this was needed either. I wrote the migration guide. I also removed a line in the documentation example of trait Adapt.

urben1680 · 2025-06-13T13:17:07Z

crates/bevy_ecs/src/schedule/condition.rs

@@ -1214,7 +1232,7 @@ where
    fn combine(
        input: <Self::In as SystemInput>::Inner<'_>,
        a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
-        b: impl FnOnce(SystemIn<'_, A>) -> B::Out,


Huh, was this ever working? If not, was this ever used?

As far as I can tell, this was only ever used in conjunction with run_if which only accepts a system with In = (). So this never came into play as this is always called with In = () for both systems A and B. But this might change in the future so we may as well correct it now.

urben1680 · 2025-06-13T13:23:28Z

crates/bevy_ecs/src/schedule/condition.rs

+        counter *= 3 * 7 * 13 * 17 * 23 * 29;
+        assert_eq!(world.resource::<Counter>().0, counter);


These tests are ridiculous because who reads them must first understand that these primes can only multiply to the correct value if the expected conditions ran. But all previous tests here look like that so I guess it is fine. I just wanted to say it.

Noted. I thought the intent was clear and it is concise this way.

urben1680 · 2025-06-13T13:47:06Z

crates/bevy_ecs/src/schedule/condition.rs

+        // This associated type is necessary to let the compiler
+        // know that `Self::System` is `ReadOnlySystem`.


I don't think this comment needs to be added on every impl. You can also make it a proper doc of this associated type.

You're right. That's better. I'll make a proper doc and add one as well for the other associated type.

urben1680 · 2025-06-13T14:24:23Z

This PR seems to fix #19527

…led::SystemCondition

…on' into impl_system_condition

gwafotapa · 2025-06-14T10:29:49Z

This PR seems to fix #19527

Indeed it does. Assuming @lotus128 was using it with a system returning a bool, not whose output required Not.

gwafotapa · 2025-06-14T10:32:13Z

crates/bevy_ecs/src/system/adapter_system.rs

@@ -16,7 +16,6 @@ use crate::{
 /// use bevy_ecs::system::{Adapt, AdapterSystem};
 ///
 /// // A system adapter that inverts the result of a system.
-/// // NOTE: Instead of manually implementing this, you can just use `bevy_ecs::schedule::common_conditions::not`.


not() cannot accept any system whose output is Not anymore so this is not a manual implementation of it anymore.

gwafotapa · 2025-06-14T10:37:49Z

crates/bevy_ecs/src/schedule/condition.rs

+    /// Used with [`AdapterSystem`] to inverse the `bool` returned by the system.
+    #[doc(hidden)]
+    #[derive(Clone, Copy)]
+    pub struct InverseBoolAdapter;


This used to be NotMarker. It's now outputting a bool instead of a Not. Thought the name bad.

release-content/migration-guides/remove_not_system.md

Co-authored-by: urben1680 <[email protected]>

chescock · 2025-06-23T17:42:05Z

Note that #19145 would address the same issue with an incompatible approach, by making all systems return Result.

gwafotapa added 5 commits June 8, 2025 19:19

impl SystemCondition for systems with output Result<(), BevyError> an…

4da9b5a

…d Result<bool, BevyError>

Add simple tests for these implementations

25205f6

Update documentation

6fe4cd5

Update doc and doc test

08bd016

Add support for the boolean combinators with a simple test

7c3c766

alice-i-cecile changed the title ~~Impl system condition~~ Impl system condition combinators for Result-ful run conditions Jun 11, 2025

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 11, 2025

gwafotapa and others added 2 commits June 12, 2025 16:20

Remove the Not bound on not(). Add a corresponding test for the new t…

257f215

…ypes

Merge branch 'main' into impl_system_condition

c3bd9a8

gwafotapa marked this pull request as ready for review June 12, 2025 14:33

gwafotapa and others added 2 commits June 12, 2025 18:27

Fix broken doc link

486cc4d

Merge branch 'main' into impl_system_condition

d7a8aef

urben1680 suggested changes Jun 13, 2025

View reviewed changes

gwafotapa added 5 commits June 13, 2025 16:45

Add doc and remove redundant comments for the associated types of sea…

93037ab

…led::SystemCondition

Avoid using function pointers

9ff7f67

Update doc of trait Adapt

c62b34d

Add migration guide for NotSystem

c1f2eb1

Merge remote-tracking branch 'refs/remotes/origin/impl_system_conditi…

0b6143c

…on' into impl_system_condition

gwafotapa commented Jun 14, 2025

View reviewed changes

urben1680 approved these changes Jun 14, 2025

View reviewed changes

release-content/migration-guides/remove_not_system.md Outdated Show resolved Hide resolved

Update release-content/migration-guides/remove_not_system.md

83b6267

Co-authored-by: urben1680 <[email protected]>

alice-i-cecile added this to the 0.17 milestone Jun 14, 2025

Merge branch 'main' into impl_system_condition

0ffe6f8

Resolve conflicts from missing imports

0ecbec8

		counter = 3 7 * 13 * 17 * 23 * 29;
		assert_eq!(world.resource::<Counter>().0, counter);

		// This associated type is necessary to let the compiler
		// know that `Self::System` is `ReadOnlySystem`.

Uh oh!

Impl system condition combinators for Result-ful run conditions #19580

Are you sure you want to change the base?

Impl system condition combinators for Result-ful run conditions #19580

Conversation

gwafotapa commented Jun 11, 2025

Objective

Solution

Testing

Uh oh!

alice-i-cecile commented Jun 11, 2025

Uh oh!

urben1680 commented Jun 12, 2025

Uh oh!

gwafotapa commented Jun 13, 2025

Uh oh!

urben1680 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwafotapa Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urben1680 commented Jun 13, 2025

Uh oh!

gwafotapa commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chescock commented Jun 23, 2025

Uh oh!

Uh oh!

Impl system condition combinators for `Result`-ful run conditions #19580

Impl system condition combinators for `Result`-ful run conditions #19580

gwafotapa Jun 14, 2025 •

edited

Loading

gwafotapa commented Jun 14, 2025 •

edited

Loading