Skip to content

Create a When system param wrapper for skipping systems that fail validation #18765

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

Merged
merged 9 commits into from
May 4, 2025

Conversation

chescock
Copy link
Contributor

@chescock chescock commented Apr 8, 2025

Objective

Create a When system param wrapper for skipping systems that fail validation.

Currently, the Single and Populated parameters cause systems to skip when they fail validation, while the Res family causes systems to error. Generalize this so that any fallible parameter can be used either to skip a system or to raise an error. A parameter used directly will always raise an error, and a parameter wrapped in When<P> will always cause the system to be silently skipped.

Note that this changes the behavior for Single and Populated. The current behavior will be available using When<Single> and When<Populated>.

Fixes #18516

Solution

Create a When system param wrapper that wraps an inner parameter and converts all validation errors to skipped.

Change the behavior of Single and Populated to fail by default.

Replace in-engine use of Single with When<Single>. I updated the fallible_systems example, but not all of the others. The other examples I looked at appeared to always have one matching entity, and it seemed more clear to use the simpler type in those cases.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Apr 8, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels Apr 8, 2025
@alice-i-cecile
Copy link
Member

Change the behavior of Single and Populated to fail by default.

I really don't think we should do this, and would prefer to spin this out into a separate PR. These are useful, terse helpers for a common pattern and I would really like to stop changing their semantics in breaking ways.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Apr 8, 2025
This reverts commit d872cea.

Revert "Fix build."

This reverts commit 64b65b8.

Revert "Have Single and Populated default to skipping instead of failing."

This reverts commit cea7adb.
@chescock
Copy link
Contributor Author

chescock commented Apr 8, 2025

I really don't think we should do this, and would prefer to spin this out into a separate PR. These are useful, terse helpers for a common pattern and I would really like to stop changing their semantics in breaking ways.

Sure, I'll remove that from this PR.

I do still think it's worth doing, though! The panicking version of Single is valuable for the same reason panicking Res is: to encode invariants that should always be true. I expect things like Single<D, With<Player>> to be especially common. And having a simple and consistent story for when systems panic vs skip is valuable, so that users only have to learn one thing, and it works for both Single and Res (and everything else).

But I will feel really bad for all the folks trying to use Single for skipping if we pull the run out from under them again :).

@chescock
Copy link
Contributor Author

chescock commented Apr 8, 2025

And having a simple and consistent story for when systems panic vs skip is valuable, so that users only have to learn one thing, and it works for both Single and Res (and everything else).

... the other way to get consistency is to make Res skip by default, and have a wrapper type to convert skips to panics, like Assert<Res<R>>. I'm not sure whether that would be more or less controversial :).

@alice-i-cecile
Copy link
Member

But I will feel really bad for all the folks trying to use Single for skipping if we pull the run out from under them again :).

Mhmm :) This is too late in the 0.16 cycle for this to land, so I'm reluctant to change the behavior yet again.

Thanks for splitting it out though; this is a much simpler and uncontroversial helper on its own.

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 8, 2025
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.

Love it, just a suggestion for documentation. Not blocking at all.

@JaySpruce JaySpruce 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 Apr 29, 2025
alice-i-cecile and others added 4 commits April 29, 2025 12:08
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
Split comment onto multiple lines to avoid horizontal scrolling in docs.
@mockersf mockersf enabled auto-merge May 4, 2025 08:22
@mockersf mockersf added this pull request to the merge queue May 4, 2025
Merged via the queue into bevyengine:main with commit d28e490 May 4, 2025
32 checks passed
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a When system param wrapper for skipping systems that fail validation
5 participants