Skip to content
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

Logging for system parameter validation should be configurable #15391

Closed
alice-i-cecile opened this issue Sep 23, 2024 · 7 comments · Fixed by #15500
Closed

Logging for system parameter validation should be configurable #15391

alice-i-cecile opened this issue Sep 23, 2024 · 7 comments · Fixed by #15500
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Following #15276, systems are now skipped when they cannot run due to missing data, rather than panicking and crashing their app.

While we should 100% warn by default when this happens, some of those logs will be spurious. This behavior might be expected, and users should be able to turn them off.

What solution would you like?

Allow users to disable this logging by disabling it at the system config level, which should work on both systems and system sets.

It would be nice to be able to change the log level to any of the standard levels: none, debug, info, warn, error, panic, but warn, and ignore are the most important.

What alternative(s) have you considered?

We could also expose a way to turn off these warnings globally, but this is likely to be a footgun: your systems silently not running is very bad!

We could also add resource / component-level log disabling, but that's more complex. My preferred solution there now is to add opt-in support for reads/writes system sets (based on #5388), and then use the mechanism above to configure them.

Additional context

Previously discussed in #15265.

@alice-i-cecile 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-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Sep 23, 2024
@MiniaczQ
Copy link
Contributor

Hmm, I'm not sure whether we need all the log levels, system skipping sounds like it should emit a single type
Is there a use case for more?

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Sep 23, 2024

Hmm, that's fine. It can be useful for folks configuring their logging filtering extensively, but warn/panic/silent is fine.

@MiniaczQ
Copy link
Contributor

This needs more thought, observers and run_system also need to validate params

I think we need to do the HashSet solution first to prevent spam and discuss before doing more

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 28, 2024

What about something like

#[derive(Default)]
enum WarnStatus {
  Never,
  #[default]
  Once,
  Always,
}

impl WarnStatus {
  pub fn warn(&mut self) -> bool {
    let (next, should_warn) = match self {
      Never => (Never, false),
      Once => (Never, true),
      Always => (Always, true),
    }
    *self = next;
    should_warn
  }
}

we can store this in SystemMeta which we can always access mutably within the system functions.
Then configuration is a matter of exposing few additional configuration methods.

This means we don't store a hashset

@MiniaczQ
Copy link
Contributor

Something we might consider is track exactly which system parameter failed validation behind a feature flag.

github-merge-queue bot pushed a commit that referenced this issue Sep 28, 2024
# Objective

Add the following system params:
- `QuerySingle<D, F>` - Valid if only one matching entity exists,
- `Option<QuerySingle<D, F>>` - Valid if zero or one matching entity
exists.

As @chescock pointed out, we don't need `Mut` variants.

Fixes: #15264

## Solution

Implement the type and both variants of system params.
Also implement `ReadOnlySystemParam` for readonly queries.

Added a new ECS example `fallible_params` which showcases `SingleQuery`
usage.
In the future we might want to add `NonEmptyQuery`,
`NonEmptyEventReader` and `Res` to it (or maybe just stop at mentioning
it).

## Testing

Tested with the example.
There is a lot of warning spam so we might want to implement #15391.
@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 29, 2024

Okay, so #15500 adds the internals for warnings, but I couldn't figure out how to expose the configuration functions as following:

// Realistically we want shorthand methods: `.no_warns()`, etc.
app.add_systems(Update, my_system.with_validation_warning(ValidationWarning::Never));

closest I've got is

app.add_systems(Update, IntoSystem::into_system(my_system).with_validation_warning(ValidationWarning::Never));

Note that only function systems make sense here.
Combined systems store multiple SystemMetas (just configure one of them) and exclusive systems always have valid params (world).

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 29, 2024

Also, about tracing exactly which param is failing, we could do right now if we move warnings into validate_param.
Which I'm quite in favor of, I'll look into it.

We'll lose system/condition/observer information, but the name will be there so this is redundant, I'd rather know the exact param.

github-merge-queue bot pushed a commit that referenced this issue Oct 3, 2024
# 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.

---------

Co-authored-by: SpecificProtagonist <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Oct 4, 2024
# Objective

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

Fixes: bevyengine#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.

---------

Co-authored-by: SpecificProtagonist <[email protected]>
robtfm pushed a commit to robtfm/bevy that referenced this issue Oct 4, 2024
# Objective

Add the following system params:
- `QuerySingle<D, F>` - Valid if only one matching entity exists,
- `Option<QuerySingle<D, F>>` - Valid if zero or one matching entity
exists.

As @chescock pointed out, we don't need `Mut` variants.

Fixes: bevyengine#15264

## Solution

Implement the type and both variants of system params.
Also implement `ReadOnlySystemParam` for readonly queries.

Added a new ECS example `fallible_params` which showcases `SingleQuery`
usage.
In the future we might want to add `NonEmptyQuery`,
`NonEmptyEventReader` and `Res` to it (or maybe just stop at mentioning
it).

## Testing

Tested with the example.
There is a lot of warning spam so we might want to implement bevyengine#15391.
robtfm pushed a commit to robtfm/bevy that referenced this issue Oct 4, 2024
# Objective

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

Fixes: bevyengine#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.

---------

Co-authored-by: SpecificProtagonist <[email protected]>
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants