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.

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

Set panic as default fallible system param behavior #16638

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

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Dec 3, 2024

Objective

Fixes: #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.

@MiniaczQ MiniaczQ changed the title go back to panic Set panic as default fallible system param behavior Dec 3, 2024
@MiniaczQ MiniaczQ marked this pull request as draft December 4, 2024 12:07
@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Dec 4, 2024

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 alice-i-cecile added this to the 0.15.1 milestone Dec 5, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Needs-Help The author needs help finishing this PR. and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 8, 2024
@alice-i-cecile
Copy link
Member

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

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Dec 8, 2024

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
Copy link
Member

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

@MiniaczQ MiniaczQ added S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed S-Needs-Help The author needs help finishing this PR. X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers labels Dec 8, 2024
@MiniaczQ MiniaczQ marked this pull request as ready for review December 8, 2024 17:41
Comment on lines -324 to -328
// Combined systems get skipped together.
(|mut commands: Commands| {
commands.insert_resource(R1);
})
.pipe(|_: In<()>, _: Res<R1>| {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As agreed, this is no longer supported in this scope

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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to toggle/revert to 0.14 behaviour of Fallible SystemParams
2 participants