Define system param validation on a per-system parameter basis

[alice-i-cecile]

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:

/// 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

• get Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER #18454 merged
• fix the todo!() in the macro-powered tuple implementation (please help 🥺)
• test
• write a migration guide
• 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.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[alice-i-cecile]

Spun out the When discussion into #18516 (I really like it, but it's off-topic here). I'm going to add more tests and clean up the example then get this merged.

[mockersf]

this is the same as before, but why is it like that?
0 would give None
1 would give Some
n would give Skipped?

[alice-i-cecile]

Yeah, that was the intended design. I'm not sold on it being particularly useful.

[NthTensor]

I recommend we continue discussion about improving the semantics of the existing system either in #18516 or a new issue.

[mockersf]

this is not the same as the && that was used before to combine bools: && allows us to stop evaluating parameters as soon as an invalid one is found, this will need to evaluate them all

[alice-i-cecile]

I don't think we should be short-circuiting here: it will substantially worsen our error messages once #18515 is added.

[mockersf]

this has the potential to be a lot slower, but then it doesn't matter because right now we panic so we would be slower to report the error 😄

I think we should still short circuit. We would still be able to report the first failure encountered which would be OK for #18515. accumulating failures would need more changes and probably a way bigger hit on perfs as you would need something with a dynamic size to accumulate errors

[alice-i-cecile]

Okay fine, the dynamically sized error accumulation would suck. I've mostly fixed short-circuiting, but left a PERF note on the macro.