Skip to content

Create a When system param wrapper for skipping systems that fail validation #18516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alice-i-cecile opened this issue Mar 24, 2025 · 8 comments · Fixed by #18765
Closed

Create a When system param wrapper for skipping systems that fail validation #18516

alice-i-cecile opened this issue Mar 24, 2025 · 8 comments · Fixed by #18765
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR

Comments

@alice-i-cecile
Copy link
Member

I really like the optional system param thing, I was wondering about that myself.

I'm less sold on having a single skipping wrapper; however if we did decide to go with it my vote would be to call it When<T> (eg. When<Res<MyResource>> or When<Single<Window>> or When<Populated<MyComponent>>) to indicate that this system runs when the parameter is valid.

Just having a few separate wrappers with this functionality built in seems fine. Having a dedicated When<T> wrapper does increase flexibility somewhat... There are situations where users may want their app to panic if they don't have exactly one entity with a single value, I think I've had situations where I'd have liked to enforce that invariant before.


Just so we are all on the same page, your proposed semantics would basically be:

  1. Every system parameter errors by default when invalid.
  2. The Option<T> wrapper turns an error into a successful None parameter.
  3. The When<T> wrapper skips the system when it encounters an error.
  4. The fallback error handler lets users further customize how unwrapped errors are handled.

I don't hate that. One slightly awkward result is the behavior of Option<When<T>> and When<Option<T>>. Should the former skip?

Originally posted by @NthTensor in #18504 (comment)

@chescock
Copy link
Contributor

We may also want a Result<T, ???> param that's like Option but lets you distinguish Skipped from Invalid.


One use case I had in mind for generic Option parameters is the safe PipeSystem example.

The issue there is that regular PipeSystem will only validate the parameters to the first system, since the first system might change whether the second system should run. But the example uses ParamSet, which currently validates all sub-parameters.

I wanted to be able to write that using ParamSet<A::Param, Option<B::Param>> so that the ParamSet only validates the first parameter, and then the pipe system can bail out early if the second parameter is None.

... but Option isn't quite enough, since we want to return an error on Invalid but success on Skipped. So I think we'll eventually want a Result wrapper to support higher-order systems.

@homebrewmellow
Copy link

Single, Option, Populated already have that behavior, no? it says so:
"Those parameters will prevent systems from running if their requirements aren’t met."

@NthTensor
Copy link
Contributor

Single, Option, Populated already have that behavior, no?

This proposal would change the semantics so that they error (see point 1) unless combined with a When<T> wrapper.

@alice-i-cecile
Copy link
Member Author

The Option<Single> behavior doesn't seem particularly intuitive or useful to me. I opted not to change it, but I would prefer a consistent blanket implementation.

@NthTensor
Copy link
Contributor

Agreed

@tbillington
Copy link
Contributor

So When<Res<T>> would be kind of equivalent to sys_fn.run_if(resource_exists::<T>) ?

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR labels Mar 25, 2025
@chescock
Copy link
Contributor

I think the intention of the original behavior of Option<Single> is a case where an entity may or may not exist, but should never have duplicates. Like, you might have a "current target" or not, but you never have two. I think that's more common then a case where you might have any number, but want to run special code when there is exactly one.

Although then the ideal behavior for Single might be 0 => Skip, 1 => Valid, N => Invalid.

(I do prefer having a blanket Option impl! But I believe the original behavior of Option<Single> was actually useful in practice.)

@NthTensor
Copy link
Contributor

I'm fine with any of this but, for the record, I don't think we should be shipping any other changes to this system in 0.16 now that Alice has basically fixed it.

github-merge-queue bot pushed a commit that referenced this issue May 4, 2025
…alidation (#18765)

# Objective

Create a `When` system param wrapper for skipping systems that fail
validation.

Currently, the `Single` and `Populated` parameters cause systems to skip
when they fail validation, while the `Res` family causes systems to
error. Generalize this so that any fallible parameter can be used either
to skip a system or to raise an error. A parameter used directly will
always raise an error, and a parameter wrapped in `When<P>` will always
cause the system to be silently skipped.

~~Note that this changes the behavior for `Single` and `Populated`. The
current behavior will be available using `When<Single>` and
`When<Populated>`.~~

Fixes #18516

## Solution

Create a `When` system param wrapper that wraps an inner parameter and
converts all validation errors to `skipped`.

~~Change the behavior of `Single` and `Populated` to fail by default.~~

~~Replace in-engine use of `Single` with `When<Single>`. I updated the
`fallible_systems` example, but not all of the others. The other
examples I looked at appeared to always have one matching entity, and it
seemed more clear to use the simpler type in those cases.~~

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Zachary Harrold <[email protected]>
Co-authored-by: François Mockers <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue May 7, 2025
# Objective

Provide a generic `impl SystemParam for Option<P>` that uses system
parameter validation. This immediately gives useful impls for params
like `EventReader` and `GizmosState` that are defined in terms of `Res`.
It also allows third-party system parameters to be usable with `Option`,
which was previously impossible due to orphan rules.

Note that this is a behavior change for `Option<Single>`. It currently
fails validation if there are multiple matching entities, but with this
change it will pass validation and produce `None`.

Also provide an impl for `Result<P, SystemParamValidationError>`. This
allows systems to inspect the error if necessary, either for bubbling it
up or for checking the `skipped` flag.

Fixes #12634
Fixes #14949
Related to #18516

## Solution

Add generic `SystemParam` impls for `Option` and `Result`, and remove
the impls for specific types.

Update documentation and `fallible_params` example with the new
semantics for `Option<Single>`.
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this issue May 17, 2025
…alidation (bevyengine#18765)

# Objective

Create a `When` system param wrapper for skipping systems that fail
validation.

Currently, the `Single` and `Populated` parameters cause systems to skip
when they fail validation, while the `Res` family causes systems to
error. Generalize this so that any fallible parameter can be used either
to skip a system or to raise an error. A parameter used directly will
always raise an error, and a parameter wrapped in `When<P>` will always
cause the system to be silently skipped.

~~Note that this changes the behavior for `Single` and `Populated`. The
current behavior will be available using `When<Single>` and
`When<Populated>`.~~

Fixes bevyengine#18516

## Solution

Create a `When` system param wrapper that wraps an inner parameter and
converts all validation errors to `skipped`.

~~Change the behavior of `Single` and `Populated` to fail by default.~~

~~Replace in-engine use of `Single` with `When<Single>`. I updated the
`fallible_systems` example, but not all of the others. The other
examples I looked at appeared to always have one matching entity, and it
seemed more clear to use the simpler type in those cases.~~

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Zachary Harrold <[email protected]>
Co-authored-by: François Mockers <[email protected]>
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this issue May 17, 2025
# Objective

Provide a generic `impl SystemParam for Option<P>` that uses system
parameter validation. This immediately gives useful impls for params
like `EventReader` and `GizmosState` that are defined in terms of `Res`.
It also allows third-party system parameters to be usable with `Option`,
which was previously impossible due to orphan rules.

Note that this is a behavior change for `Option<Single>`. It currently
fails validation if there are multiple matching entities, but with this
change it will pass validation and produce `None`.

Also provide an impl for `Result<P, SystemParamValidationError>`. This
allows systems to inspect the error if necessary, either for bubbling it
up or for checking the `skipped` flag.

Fixes bevyengine#12634
Fixes bevyengine#14949
Related to bevyengine#18516

## Solution

Add generic `SystemParam` impls for `Option` and `Result`, and remove
the impls for specific types.

Update documentation and `fallible_params` example with the new
semantics for `Option<Single>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants