Set panic as default fallible system param behavior #16638

MiniaczQ · 2024-12-03T22:26:29Z

Objective

Solution

This is a patch fix, proper fix requires a breaking change.

Added Panic enum variant and using is as the system meta default.
Warn once behavior can be enabled same way disabling panic (originally disabling wans) is.

To fix an issue with the current architecture, where all combinator system params get checked together,
combinator systems only check params of the first system.
This will result in old, panicking behavior on subsequent systems and will be fixed in 0.16.

Testing

Ran unit tests and fallible_params example.

MiniaczQ · 2024-12-04T12:07:37Z

Naive approach comes with some issues, I'm not sure when I'll have time to truly sit down and work on this, so feel free to take over as needed

alice-i-cecile · 2024-12-08T15:36:08Z

Can you say a bit more about the issues you encountered?

MiniaczQ · 2024-12-08T15:51:11Z

The main issue is stuff that was fixed in the System::run() -> Option<S::Out> rework (didn't land in 0.15).
In a combination system, like a.and(b) or a.pipe(b) we're running param checks for all systems before running any of them.
This results in resource_exists::<MyRes>.and(do_stuff) failing params, because do_stuff doesn't have MyRes even though it wouldn't run because of failed condition.
When we change default fail behavior to panic, this causes crashes.

I've had time to think about it since this PR.
We didn't ship a complete feature, so I think we should be fine to downscope it.
Since combination systems are incomplete, let's just ensure the first system (or the system, if no combinations are used) works correctly with fallible params, do not check params for subsequent ones and just let app crash like it did in 0.14.
In 0.16 we can ship the correct param checking behavior, where we do it directly before running a system.

alice-i-cecile · 2024-12-08T16:09:11Z

I'm okay with that as the behavior for now :)

mockersf · 2024-12-24T01:28:46Z

MiniaczQ#1 should get the example working again

don't setup the skin/morph when there is no renderer

# Objective Fixes: bevyengine#16578 ## Solution This is a patch fix, proper fix requires a breaking change. Added `Panic` enum variant and using is as the system meta default. Warn once behavior can be enabled same way disabling panic (originally disabling wans) is. To fix an issue with the current architecture, where **all** combinator system params get checked together, combinator systems only check params of the first system. This will result in old, panicking behavior on subsequent systems and will be fixed in 0.16. ## Testing Ran unit tests and `fallible_params` example. --------- Co-authored-by: François Mockers <mockersf@gmail.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective - Fixes #16959 - The `pbr.rs` example in the 3d section panicked because of the changes in #16638, that was not supposed to happen ## Solution - For now it's sufficient to introduce a `never_param_warn` call when adding the fallible system into the app ## Testing - Tested on my machine via `cargo r --example pbr`, it built and ran successfully --------- Co-authored-by: Freya Pines <freya@Freyas-MacBook-Air.local> Co-authored-by: François Mockers <francois.mockers@vleue.com>

Fixes: #16578 This is a patch fix, proper fix requires a breaking change. Added `Panic` enum variant and using is as the system meta default. Warn once behavior can be enabled same way disabling panic (originally disabling wans) is. To fix an issue with the current architecture, where **all** combinator system params get checked together, combinator systems only check params of the first system. This will result in old, panicking behavior on subsequent systems and will be fixed in 0.16. Ran unit tests and `fallible_params` example. --------- Co-authored-by: François Mockers <mockersf@gmail.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective - Fixes #16959 - The `pbr.rs` example in the 3d section panicked because of the changes in #16638, that was not supposed to happen ## Solution - For now it's sufficient to introduce a `never_param_warn` call when adding the fallible system into the app ## Testing - Tested on my machine via `cargo r --example pbr`, it built and ran successfully --------- Co-authored-by: Freya Pines <freya@Freyas-MacBook-Air.local> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective Fixes: bevyengine#16578 ## Solution This is a patch fix, proper fix requires a breaking change. Added `Panic` enum variant and using is as the system meta default. Warn once behavior can be enabled same way disabling panic (originally disabling wans) is. To fix an issue with the current architecture, where **all** combinator system params get checked together, combinator systems only check params of the first system. This will result in old, panicking behavior on subsequent systems and will be fixed in 0.16. ## Testing Ran unit tests and `fallible_params` example. --------- Co-authored-by: François Mockers <mockersf@gmail.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective - Fixes bevyengine#16959 - The `pbr.rs` example in the 3d section panicked because of the changes in bevyengine#16638, that was not supposed to happen ## Solution - For now it's sufficient to introduce a `never_param_warn` call when adding the fallible system into the app ## Testing - Tested on my machine via `cargo r --example pbr`, it built and ran successfully --------- Co-authored-by: Freya Pines <freya@Freyas-MacBook-Air.local> Co-authored-by: François Mockers <francois.mockers@vleue.com>

Danielmelody · 2025-01-22T14:34:26Z

This is actually a breaking change from 0.15.0, as some system with Single<..> query crashes after a patch update to 0.15.1. Better to intro this in 0.16.0,

# Objective Fixes: bevyengine#16578 ## Solution This is a patch fix, proper fix requires a breaking change. Added `Panic` enum variant and using is as the system meta default. Warn once behavior can be enabled same way disabling panic (originally disabling wans) is. To fix an issue with the current architecture, where **all** combinator system params get checked together, combinator systems only check params of the first system. This will result in old, panicking behavior on subsequent systems and will be fixed in 0.16. ## Testing Ran unit tests and `fallible_params` example. --------- Co-authored-by: François Mockers <mockersf@gmail.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective - Fixes bevyengine#16959 - The `pbr.rs` example in the 3d section panicked because of the changes in bevyengine#16638, that was not supposed to happen ## Solution - For now it's sufficient to introduce a `never_param_warn` call when adding the fallible system into the app ## Testing - Tested on my machine via `cargo r --example pbr`, it built and ran successfully --------- Co-authored-by: Freya Pines <freya@Freyas-MacBook-Air.local> Co-authored-by: François Mockers <francois.mockers@vleue.com>

MiniaczQ added 4 commits December 3, 2024 23:23

go back to panic

6a09fca

adjust tests

c5d7248

update example

555d12b

update docs

405c850

MiniaczQ changed the title ~~go back to panic~~ Set panic as default fallible system param behavior Dec 3, 2024

seivan mentioned this pull request Dec 4, 2024

Option to toggle/revert to 0.14 behaviour of Fallible SystemParams #16578

Closed

MiniaczQ marked this pull request as draft December 4, 2024 12:07

alice-i-cecile added this to the 0.15.1 milestone Dec 5, 2024

MiniaczQ added 3 commits December 8, 2024 18:20

remove combinator system param checking

82d3611

fix test

1330797

fix executor tests

f2cbfe8

MiniaczQ marked this pull request as ready for review December 8, 2024 17:41

MiniaczQ added 2 commits December 8, 2024 18:41

Merge branch 'main' into go-back-to-panic

0f44ba8

fix few doc tests

ce753d1

alice-i-cecile added this pull request to the merge queue Dec 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024

MiniaczQ added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Dec 20, 2024

Merge branch 'main' into go-back-to-panic

e568533

BenjaminBrienen approved these changes Dec 24, 2024

View reviewed changes

don't setup the skin/morph when there is no renderer

7dc6f7f

mockersf mentioned this pull request Dec 24, 2024

don't setup the skin/morph when there is no renderer MiniaczQ/bevy#1

Merged

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 24, 2024

Merge pull request #1 from mockersf/go-back-to-panic

999420f

don't setup the skin/morph when there is no renderer

alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 24, 2024

alice-i-cecile enabled auto-merge December 24, 2024 02:31

alice-i-cecile added this pull request to the merge queue Dec 24, 2024

Merged via the queue into bevyengine:main with commit 460de77 Dec 24, 2024
29 checks passed

pin3-free mentioned this pull request Dec 26, 2024

Fixed panic in pbr example #16976

Merged

rparrett mentioned this pull request Dec 26, 2024

scene_viewer and audio_control examples panic #16978

Closed

This was referenced Apr 7, 2025

Single causes piped systems to panic #18755

Closed

Fix system param validation for piped systems #18785

Merged

alice-i-cecile mentioned this pull request Apr 10, 2025

Eager system parameter validation causes issues with combinator and piped systems #18796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Set panic as default fallible system param behavior #16638

Set panic as default fallible system param behavior #16638

Uh oh!

MiniaczQ commented Dec 3, 2024 •

edited

Loading

Uh oh!

MiniaczQ commented Dec 4, 2024

Uh oh!

alice-i-cecile commented Dec 8, 2024

Uh oh!

MiniaczQ commented Dec 8, 2024 •

edited

Loading

Uh oh!

alice-i-cecile commented Dec 8, 2024

Uh oh!

Uh oh!

mockersf commented Dec 24, 2024

Uh oh!

Uh oh!

Danielmelody commented Jan 22, 2025

Uh oh!

Uh oh!

Uh oh!

Set panic as default fallible system param behavior #16638

Set panic as default fallible system param behavior #16638

Uh oh!

Conversation

MiniaczQ commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

MiniaczQ commented Dec 4, 2024

Uh oh!

alice-i-cecile commented Dec 8, 2024

Uh oh!

MiniaczQ commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Dec 8, 2024

Uh oh!

Uh oh!

mockersf commented Dec 24, 2024

Uh oh!

Uh oh!

Danielmelody commented Jan 22, 2025

Uh oh!

Uh oh!

MiniaczQ commented Dec 3, 2024 •

edited

Loading

MiniaczQ commented Dec 8, 2024 •

edited

Loading