Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER #18454
Conversation
…for systems and run conditions
alice-i-cecile
added
C-Bug
| #[derive(Debug, PartialEq, Eq, Clone, Error)] | ||
| pub enum SystemParamValidationError { | ||
| /// A system failed to validate a parameter. | ||
| #[error("System failed to validate parameter")] | ||
| System, | ||
| /// A run condition failed to validate a parameter. | ||
| #[error("Run condition failed to validate parameter")] | ||
| RunCondition, | ||
| /// An observer failed to validate a parameter. | ||
| #[error("Observer failed to validate parameter")] | ||
| Observer, | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add name of the param here?
There was a problem hiding this comment.
This should probably get piped through to RegisteredSystemError::InvalidParams. That's a nit, as the run_system api probably needs to be refactored a tad now that we can return results from systems. I think right now it will give you a Result<Result<O, E>, RegisteredSystemError> when it really should be more monadic.
Let's not worry about that now.
There was a problem hiding this comment.
I would really like to add the name of the param here, but doing so will be substantially more complex. So, follow-up ideally :)
There was a problem hiding this comment.
Overall great work!
This is way simpler than the previous mess, but that still leaves some cleanup to be done.
The examples (comments) mention we can mix and match certain behaviors, but that is no longer the case!
Using params as "run conditions" should now rely on param: Option<Param> followed by let param = param?.
No need for "warn once" here, I don't think "warn once" was a good mechanic, it was a middle ground between never warn and always warn. What we actually want is "warn once whenever a system starts skipping, then warn again if it ran in the meantime". (and we only need to track run conditions and systems, since observers don't run on schedule)
We should be able to do that as long as the error handler is stateful (I'm very out of date on the features), just store a map of system -> last invalid param tick.
|
A2: A3: |
|
Re: Warn once. I agreed that a stateful error handler would be smart. But currently the default error handler is just panicking, and if users want more complex behavior they have to write it themselves. We could wait to see what people do with the feature, or we could provide some other pre-made handlers. I'd be fine with either.
I'm personally ok with not having a system-level override, just the param and then the default handler. Up to you. |
|
Alright, thanks for the feedback! I'm going to do minimal cleanup on this on Sunday, get it merged, and then see if I can't get per |
|
A possible strategy for per-param configuration are generic wrappers which change the error type to something that handlers are less sensitive to. |
alice-i-cecile
removed
the
X-Controversial
alice-i-cecile
added
the
X-Contentious
Co-authored-by: MiniaczQ <[email protected]>
|
I don't understand what's going on with https://github.com/bevyengine/bevy/actions/runs/14024499144/job/39260883882?pr=18454, which seems to be failing due to #18301. I also don't know why this is only failing on this PR. It seems to be Mac-specific, so I can't reproduce it locally. |
|
Cloned and ran |
alice-i-cecile
added
S-Ready-For-Final-Review
# Objective
When introduced, `Single` was intended to simply be silently skipped,
allowing for graceful and efficient handling of systems during invalid
game states (such as when the player is dead).
However, this also caused missing resources to *also* be silently
skipped, leading to confusing and very hard to debug failures. In
0.15.1, this behavior was reverted to a panic, making missing resources
easier to debug, but largely making `Single` (and `Populated`)
worthless, as they would panic during expected game states.
Ultimately, the consensus is that this behavior should differ on a
per-system-param basis. However, there was no sensible way to *do* that
before this PR.
## Solution
Swap `SystemParam::validate_param` from a `bool` to:
```rust
/// The outcome of system / system param validation,
/// used by system executors to determine what to do with a system.
pub enum ValidationOutcome {
/// All system parameters were validated successfully and the system can be run.
Valid,
/// At least one system parameter failed validation, and an error must be handled.
/// By default, this will result in1 a panic. See [crate::error] for more information.
///
/// This is the default behavior, and is suitable for system params that should *always* be valid,
/// either because sensible fallback behavior exists (like [`Query`] or because
/// failures in validation should be considered a bug in the user's logic that must be immediately addressed (like [`Res`]).
Invalid,
/// At least one system parameter failed validation, but the system should be skipped due to [`ValidationBehavior::Skip`].
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
Skipped,
}
```
Then, inside of each `SystemParam` implementation, return either Valid,
Invalid or Skipped.
Currently, only `Single`, `Option<Single>` and `Populated` use the
`Skipped` behavior. Other params (like resources) retain their current
failing
## Testing
Messed around with the fallible_params example. Added a pair of tests:
one for panicking when resources are missing, and another for properly
skipping `Single` and `Populated` system params.
## To do
- [x] get #18454 merged
- [x] fix the todo!() in the macro-powered tuple implementation (please
help 🥺)
- [x] test
- [x] write a migration guide
- [x] update the example comments
## Migration Guide
Various system and system parameter validation methods
(`SystemParam::validate_param`, `System::validate_param` and
`System::validate_param_unsafe`) now return and accept a
`ValidationOutcome` enum, rather than a `bool`. The previous `true`
values map to `ValidationOutcome::Valid`, while `false` maps to
`ValidationOutcome::Invalid`.
However, if you wrote a custom schedule executor, you should now respect
the new `ValidationOutcome::Skipped` parameter, skipping any systems
whose validation was skipped. By contrast, `ValidationOutcome::Invalid`
systems should also be skipped, but you should call the
`default_error_handler` on them first, which by default will result in a
panic.
If you are implementing a custom `SystemParam`, you should consider
whether failing system param validation is an error or an expected
state, and choose between `Invalid` and `Skipped` accordingly. In Bevy
itself, `Single` and `Populated` now once again skip the system when
their conditions are not met. This is the 0.15.0 behavior, but stands in
contrast to the 0.15.1 behavior, where they would panic.
---------
Co-authored-by: MiniaczQ <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: Chris Russell <[email protected]>
…ia the GLOBAL_ERROR_HANDLER (#18454) There are two related problems here: 1. Users should be able to change the fallback behavior of *all* ECS-based errors in their application by setting the `GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein. 2. The existing solution (#15500) for customizing this behavior is high on boilerplate, not global and adds a great deal of complexity. The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question: `Single` / `Populated` should silently skip the system, but `Res` should panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day). I've removed the existing `ParamWarnPolicy`-based configuration, and wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the various schedule executors to properly plumb through errors . Additionally, I've done a small cleanup pass on the corresponding example. I've run the `fallible_params` example, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲 1. Currently, failed system param validation will result in endless console spam. Do you want me to implement a solution for warn_once-style debouncing somehow? 2. Currently, the error reporting for failed system param validation is very limited: all we get is that a system param failed validation and the name of the system. Do you want me to implement improved error reporting by bubbling up errors in this PR? 3. There is broad consensus that the default behavior for failed system param validation should be set on a per-system param basis. Would you like me to implement that in this PR? My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart. `ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed completely. Failures during system param validation are now handled via the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs for more information. --------- Co-authored-by: MiniaczQ <[email protected]>
# Objective
When introduced, `Single` was intended to simply be silently skipped,
allowing for graceful and efficient handling of systems during invalid
game states (such as when the player is dead).
However, this also caused missing resources to *also* be silently
skipped, leading to confusing and very hard to debug failures. In
0.15.1, this behavior was reverted to a panic, making missing resources
easier to debug, but largely making `Single` (and `Populated`)
worthless, as they would panic during expected game states.
Ultimately, the consensus is that this behavior should differ on a
per-system-param basis. However, there was no sensible way to *do* that
before this PR.
## Solution
Swap `SystemParam::validate_param` from a `bool` to:
```rust
/// The outcome of system / system param validation,
/// used by system executors to determine what to do with a system.
pub enum ValidationOutcome {
/// All system parameters were validated successfully and the system can be run.
Valid,
/// At least one system parameter failed validation, and an error must be handled.
/// By default, this will result in1 a panic. See [crate::error] for more information.
///
/// This is the default behavior, and is suitable for system params that should *always* be valid,
/// either because sensible fallback behavior exists (like [`Query`] or because
/// failures in validation should be considered a bug in the user's logic that must be immediately addressed (like [`Res`]).
Invalid,
/// At least one system parameter failed validation, but the system should be skipped due to [`ValidationBehavior::Skip`].
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
Skipped,
}
```
Then, inside of each `SystemParam` implementation, return either Valid,
Invalid or Skipped.
Currently, only `Single`, `Option<Single>` and `Populated` use the
`Skipped` behavior. Other params (like resources) retain their current
failing
## Testing
Messed around with the fallible_params example. Added a pair of tests:
one for panicking when resources are missing, and another for properly
skipping `Single` and `Populated` system params.
## To do
- [x] get #18454 merged
- [x] fix the todo!() in the macro-powered tuple implementation (please
help 🥺)
- [x] test
- [x] write a migration guide
- [x] update the example comments
## Migration Guide
Various system and system parameter validation methods
(`SystemParam::validate_param`, `System::validate_param` and
`System::validate_param_unsafe`) now return and accept a
`ValidationOutcome` enum, rather than a `bool`. The previous `true`
values map to `ValidationOutcome::Valid`, while `false` maps to
`ValidationOutcome::Invalid`.
However, if you wrote a custom schedule executor, you should now respect
the new `ValidationOutcome::Skipped` parameter, skipping any systems
whose validation was skipped. By contrast, `ValidationOutcome::Invalid`
systems should also be skipped, but you should call the
`default_error_handler` on them first, which by default will result in a
panic.
If you are implementing a custom `SystemParam`, you should consider
whether failing system param validation is an error or an expected
state, and choose between `Invalid` and `Skipped` accordingly. In Bevy
itself, `Single` and `Populated` now once again skip the system when
their conditions are not met. This is the 0.15.0 behavior, but stands in
contrast to the 0.15.1 behavior, where they would panic.
---------
Co-authored-by: MiniaczQ <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: Chris Russell <[email protected]>
Objective
There are two related problems here:
GLOBAL_ERROR_HANDLER. See Unify and simplify command and system error handling #18351 for earlier work in this vein.The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question:
Single/Populatedshould silently skip the system, butResshould panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day).Solution
I've removed the existing
ParamWarnPolicy-based configuration, and wired up theGLOBAL_ERROR_HANDLER/default_error_handlerto the various schedule executors to properly plumb through errors .Additionally, I've done a small cleanup pass on the corresponding example.
Testing
I've run the
fallible_paramsexample, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲Questions for reviewers
My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart.
Migration Guide
ParamWarnPolicyand theWithParamWarnPolicyhave been removed completely. Failures during system param validation are now handled via theGLOBAL_ERROR_HANDLER: please see thebevy_ecs::errormodule docs for more information.