Skip to content

When & WhenMut: skip system if Res is missing. #18927

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mrchantey
Copy link
Contributor

Objective

ParamWarnPolicy has been removed in 0.16, deprecating never_param_warn(). Like Single and Populated, we need a Res type that will skip the system if the resource is not present.

Solution

Implement two new system params: When & WhenMut, identical to Res & ResMut but with a validate_param that will skip instead of panic.

These types can also be extended to 'wrap' other types beyond Res & ResMut.

Testing

Tested both should skip and should not skip cases, verify by running cargo test -p bevy_ecs system::when


Showcase

Res & ResMut now have two alternatives When and WhenMut for when a system should skip instead of panic.

// existing options
fn panics_if_not_present(res: Res<Foo>){}
fn runs_even_if_not_present(res: Option<Res<Foo>>){}

// new - skip the system if the resource is missing
fn skips_if_not_present(res: When<Foo>){}

@mrchantey mrchantey requested a review from NthTensor April 25, 2025 03:20
@mrchantey mrchantey added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Apr 25, 2025
@mrchantey mrchantey added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 25, 2025
Copy link
Contributor

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

@bushrat011899
Copy link
Contributor

I like the idea, but When feels very general. To me, it feels like something you could use with any SystemParam and doesn't communicate its link to Resource well. Bike shedding some other names:

  • ResIf<R>
  • When<Res<R>> (this would be nice but might require more work than this PR covers to be generic)

@mrchantey
Copy link
Contributor Author

Good point, I think the name was chosen so that the same convention can be implemented for more types beyond Res & ResMut in the future, more context in discord. Definitely possible to wrap Res, the first version did use fn my_system(res: When<Res<Foo>>) but maybe this brings it more in line with the other utility types like Single<T>?

@MrGVSV
Copy link
Member

MrGVSV commented Apr 25, 2025

Definitely possible to wrap Res, the first version did use fn my_system(res: When<Res<Foo>>) but maybe this brings it more in line with the other utility types like Single<T>?

The cools thing about making When a wrapper is that we can possibly make Single and friends panic again. I like that it can skip systems but sometimes there are times where you want to fail fast if the expected single-ness is ever invalidated.

So I think When<Single<T>> actually makes a lot of sense.

Alternatively, we could do the inverse if skipping systems is a more commonly-desired scenario. Maybe we make Res skip by default but then allow users to require its existence with Require<Res<T>> or something.

@NthTensor
Copy link
Contributor

@alice-i-cecile May need to make a call on M-Needs-Release-Note. This is typically a maintainer-level publishing choice.

I'm really not sure what the right usage of When would be -- do we want it to work for all system params, do we want it to be just for resources -- so I'm also tagging this with Needs-Design.

The cools thing about making When a wrapper is that we can possibly make Single and friends panic again.

Sort of don't want to break Single again personally, that's a lot of churn over several releases.

@NthTensor NthTensor added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Apr 25, 2025
@alice-i-cecile
Copy link
Member

Sort of don't want to break Single again personally, that's a lot of churn over several releases.

Strongly agree here.

@alice-i-cecile May need to make a call on M-Needs-Release-Note. This is typically a maintainer-level publishing choice.

Yes, worth having.

I'm really not sure what the right usage of When would be -- do we want it to work for all system params, do we want it to be just for resources -- so I'm also tagging this with Needs-Design.

All system params please!

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Apr 25, 2025
@chescock
Copy link
Contributor

Note that there is also an existing PR for a generic When: #18765

@MrGVSV
Copy link
Member

MrGVSV commented Apr 25, 2025

Sort of don't want to break Single again personally, that's a lot of churn over several releases.

Fair but I think it's going to benefit readability if skipping (or not skipping) is explicit. And I do think there's benefit to having a fail fast variant for these sorts of parameters. If I expect a system to run, it might not be helpful for it to silently skip (of course, you can panic manually with Query::single so that's at least an option).

This could also be confusing for new users who see that When causes a system to be skipped, but find their Single systems randomly being skipped.

So imo adding a generic When puts Single as is in an unfinished/inconsistent state. And it's probably better to deal with another bit of churn now than decide to do it further down the road.

If it's even desired behavior haha I could be alone in this thought process.

@mrchantey
Copy link
Contributor Author

mrchantey commented Apr 25, 2025

it's probably better to deal with another bit of churn now than decide to do it further down the road.

I agree, this feels like one thats worth thinking deeply about and getting right. In terms of design it sounds like there are two paths forward:

Option A: Single and friends Panic

  • Res and friends still panic
  • Single and friends should panic again
fn system(
  res: When<Res<Foo>>,
  single: When<Single<Bar>>,
)

Option B: Only Resources Panic

  • Res and friends still panic
  • Single and friends still skip

In this case we'd need to make some implementation decisions:

// b.1 - No wrapper for Res & ResMut
fn system(
  res: When<Foo>,
  res_mut: WhenMut<Foo>,
  reader: WhenMut<EventReader<Bar>>,
)
// b.2 - Blanket Wrapper
fn system(
  res: When<Res<Foo>>,
  res_mut: When<ResMut<Foo>>,
  reader: When<EventReader<Bar>>,
)
// b.3 Required Single panics
fn system(
  query: Required<Single<Foo>>,
)

In my opinion Option B better reflects the nature of Components vs Resources. Looking at my codebase I'd like most Populated<Window> systems to skip, but Res<Time> systems to panic by default.

As for the implementation I think we should close this pr in favor of the blanket wrapper in #18765, and When<Single<T>> is valid but redundant. This closes the door on When<MyResource> but thats controversial and a wrapper is far simpler to implement and maintain.

@bushrat011899
Copy link
Contributor

Personally, I'm in favour of When being a generic wrapper for any system parameter. For one thing, it means we don't need to do WhenMut, WhenNonSend, WhenNonSendMut, etc. etc., making the API surface area smaller but more powerful.

I also like that this opens the door for a form of per-parameter run conditions as a general API, expanding on Option and ParamSet.

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 M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants