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.

Sign up for GitHub

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

Jump to bottom

Respect SystemParam::validate_param for observers and other non-executor system runners #15394

Closed
MiniaczQ opened this issue Sep 23, 2024 · 2 comments · Fixed by #15526
Closed

Respect SystemParam::validate_param for observers and other non-executor system runners #15394

MiniaczQ opened this issue Sep 23, 2024 · 2 comments · Fixed by #15526
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible 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
Milestone
0.15

Comments

@MiniaczQ
Copy link
Contributor

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

Currently only schedule executors respect the SystemParam::validate_param to prevent panics when running systems with unavailable resources.
As a follow up to #15276 this should be implemented for other places that run systems like:

  • observers,
  • manually executed systems through &mut World

What solution would you like?

Check validate_param directly before running the systems and if it fails, don't run them.
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events M-Needs-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! and removed S-Needs-Triage This issue needs to be labelled M-Needs-Release-Note Work that should be called out in the blog due to impact labels Sep 23, 2024
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed X-Contentious There are nontrivial implications that should be thought through labels Sep 23, 2024
@MiniaczQ MiniaczQ mentioned this issue Sep 28, 2024
Merged
@MiniaczQ
Copy link
Contributor Author

Trying to implement this, few topics to resolve:

  1. Where to store the warning mechanism (Logging for system parameter validation should be configurable #15391). Right now it's a macro in the executors, but we need it in observers and run_once world methods. Should I put it in system_param.rs?
  2. There is a lot of validate -> run patterns. The only reason this is separated is for multithreaded executor, where we want to save on task spawning. This would also help us centralize the warnings.
  3. Observers don't return values, so validate support is trivial. The problem is in run_once methods, which can return a value. We can either:
    a) Add Option to all return values.
    b) Add a new family of methods with try_ prefix.
    The latter is less intrusive, but in the long run it prevents us from requiring validate_param before running the systems.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 28, 2024
@alice-i-cecile
Copy link
Member

Now that we're actually shipping Single and friends, this is a blocker to the 0.15 release: https://discord.com/channels/691052431525675048/749335865876021248/1289685595676868661

This was referenced Sep 28, 2024
Open
Merged
github-merge-queue bot pushed a commit that referenced this issue Sep 30, 2024
@MiniaczQ @alice-i-cecile @bushrat011899
System param validation for observers, system registry and run once (#…
5289e18 
…15526)

# Objective

Fixes #15394

## Solution

Observers now validate params.

System registry has a new error variant for when system running fails
due to invalid parameters.

Run once now returns a `Result<Out, RunOnceError>` instead of `Out`.
This is more inline with system registry, which also returns a result.

I'll address warning messages in #15500.

## Testing

Added one test for each case.

---

## Migration Guide

- `RunSystemOnce::run_system_once` and
`RunSystemOnce::run_system_once_with` now return a `Result<Out>` instead
of just `Out`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
robtfm pushed a commit to robtfm/bevy that referenced this issue Oct 4, 2024
@MiniaczQ @alice-i-cecile
@bushrat011899 @robtfm
System param validation for observers, system registry and run once (b…
3ea9803 
…evyengine#15526)

# Objective

Fixes bevyengine#15394

## Solution

Observers now validate params.

System registry has a new error variant for when system running fails
due to invalid parameters.

Run once now returns a `Result<Out, RunOnceError>` instead of `Out`.
This is more inline with system registry, which also returns a result.

I'll address warning messages in bevyengine#15500.

## Testing

Added one test for each case.

---

## Migration Guide

- `RunSystemOnce::run_system_once` and
`RunSystemOnce::run_system_once_with` now return a `Result<Out>` instead
of just `Out`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible 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
Milestone
0.15
Development

Successfully merging a pull request may close this issue.

System param validation for observers, system registry and run once MiniaczQ/bevy
2 participants
@alice-i-cecile @MiniaczQ