Better warnings about invalid parameters

[MiniaczQ]

Objective

System param validation warnings should be configurable and default to "warn once" (per system).

Fixes: #15391

Solution

SystemMeta is given a new ParamWarnPolicy field.
The policy decides whether warnings will be emitted by each system param when it fails validation.
The policy is updated by the system after param validation fails.

Example warning:

2024-09-30T18:10:04.740749Z  WARN bevy_ecs::system::function_system: System fallible_params::do_nothing_fail_validation will not run because it requested inaccessible system parameter Single<(), (With<Player>, With<Enemy>)>

Currently, only the first invalid parameter is displayed.

Warnings can be disabled on function systems using .param_never_warn().
(there is also .with_param_warn_policy(policy))

Testing

Ran fallible_params example.

[MiniaczQ]

I've noticed some problems with this approach, I'll be reworking it

[SpecificProtagonist]

This approach looks good.

Added a new bevy_ecs default feature flag bevy_warn_invalid_param which enables the warnings.
Disabling it can slightly improve performance due to reduced checks when running each system.

Have you benchmarked this? Two not taken (predictable) ifs per system invocation probably aren't measurable and not worth adding a system flag.

I'm not convinced we need WarnPolicy::Always, but I don't have much of an opinion on it.

I do however think it makes sense to rename WarnPolicy to ParamWarnPolicy, as it is about param validation and doesn't affect bevy::log::warn.

[MiniaczQ]

Have you benchmarked this? Two not taken (predictable) ifs per system invocation probably aren't measurable and not worth adding a system flag.

Not benchmarked, I'll see if next reviewer agrees or if we'll need benchmarks.

[SpecificProtagonist]

Trying to construct a worst-case benchmark, where the systems do no work and there are no parameters to validate or extract:

let mut world = World::new();
let mut schedule = Schedule::default();
for _ in 0..1000 {
    schedule.add_systems(|| ());
}
for _ in 0..1000000 {
    schedule.run(&mut world);
}

With bevy_param_warn:
  Time (mean ± σ):     13.639 s ±  0.147 s    [User: 13.590 s, System: 0.025 s]
  Range (min … max):   13.484 s … 13.982 s    10 runs

Without:
  Time (mean ± σ):     13.817 s ±  0.196 s    [User: 13.772 s, System: 0.025 s]
  Range (min … max):   13.539 s … 14.250 s    10 runs

So there's about a standard deviation difference. But if these numbers were precise, the feature flag would allow removing 0.178ns per system. I don't think that's worth it.

[MiniaczQ]

Thank you for your work, I'll remove the flag when im home :)

[chescock]

So the warning is now emitted from the param itself, which lets you include the param type in the message. And the warning state is stored on the system, which saves space and is easier to configure. As a consequence, if a system has multiple failing parameters, it only warns on the first one, right? That all seems reasonable, although the final result is a little surprising.

[MiniaczQ]

if a system has multiple failing parameters, it only warns on the first one, right?

The policy update of a (function) system happens after param validation fails.
But param validation of tuples, etc. currently short circuits, so yes
you will only see the first missing type.

While we can remove the short circuit, this won't patch a hole where missing parameters can change over time and we'll still only get warning about the first one (time-wise).

I guess we could store the warning per system param??
But then routing all the configuration will be hellish

[bushrat011899]

Looks good to me, love the idea of giving more meaningful feedback to the user without spamming them needlessly. I have a note about a possible future PR, but definitely doesn't need to be in this PR.

[bushrat011899]

Future PR: Seems reasonable to add an Always policy as well:

/// State machine for emitting warnings when [system params are invalid](System::validate_param).
#[derive(Clone, Copy)]
pub enum ParamWarnPolicy {
    /// No warning should ever be emitted.
    Never,
    /// The warning will be emitted once and status will update to [`Self::Never`].
    Once,
    /// The warning will be emitted each time the system is run.
    Always,
}

[MiniaczQ]

I had it but removed it as per previous feedback.
I think Always will be a bit too annoying, but we could certainly use more complex policies:

• system just stopped running (prev frame valid, this frame invalid)
• invalid params changed

If we get some higher level logging with editor, we can do stuff like:

• system X stopped running at frame N because of invalid param P and is still not running till frame M

[SpecificProtagonist]

Hm, I don't think we have a good way to test whether the right things get logged?

btw the PR description still mentions bevy_param_warn

Anyway, the implementation is looking good to me!