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

System param validation for observers, system registry and run once #15526

Merged
merged 10 commits into from
Sep 30, 2024
Merged

System param validation for observers, system registry and run once #15526

merged 10 commits into from
Sep 30, 2024

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Sep 29, 2024 •
edited
Loading

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

@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through labels Sep 29, 2024
@MiniaczQ MiniaczQ marked this pull request as ready for review September 29, 2024 20:54
@MiniaczQ MiniaczQ added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 29, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 29, 2024
alice-i-cecile
alice-i-cecile reviewed Sep 29, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple and effective; I'm pleased that adapting these to the new system param validation framework was so easy.

I have some feedback on docs / error messages that I'd like to resolve before we merge if possible.

MiniaczQ and others added 2 commits September 30, 2024 01:10
@MiniaczQ @alice-i-cecile
Apply suggestions from code review
7234133 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@MiniaczQ
pr suggestions
6f713b1
@alice-i-cecile alice-i-cecile added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Sep 29, 2024
bushrat011899
bushrat011899 approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! I've suggested a handful of comments to help with understanding why the new tests should fail (resource not available in the world). On first glance I missed that was the cause of the failure and was quite confused!

@bushrat011899 bushrat011899 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 30, 2024
@MiniaczQ @bushrat011899
Apply suggestions from code review
064a83c 
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
Merged via the queue into bevyengine:main with commit 5289e18 Sep 30, 2024
26 checks passed
robtfm pushed a commit to robtfm/bevy that referenced this pull request 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
Reviewers

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@chescock chescock Awaiting requested review from chescock

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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.15
Development

Successfully merging this pull request may close these issues.

Respect SystemParam::validate_param for observers and other non-executor system runners
3 participants
@MiniaczQ @bushrat011899 @alice-i-cecile