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

Set panic as default fallible system param behavior #16638

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

Set panic as default fallible system param behavior #16638

wants to merge 9 commits into from
+51 −41

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Dec 3, 2024 •
edited
Loading

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
@seivan seivan mentioned this pull request Dec 4, 2024
Open
@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 •
edited
Loading

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
MiniaczQ
MiniaczQ commented Dec 8, 2024
View reviewed changes
crates/bevy_ecs/src/schedule/executor/mod.rs
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

Diddykonga
Diddykonga approved these changes Dec 15, 2024
View reviewed changes
Copy link

@Diddykonga Diddykonga left a comment

Choose a reason for hiding this comment

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

LGTM

andriyDev
andriyDev reviewed Dec 15, 2024
View reviewed changes
crates/bevy_ecs/src/system/combinator.rs
@@ -214,7 +214,7 @@ where
#[inline]
unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
// SAFETY: Delegate to other `System` implementations.
unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
unsafe { self.a.validate_param_unsafe(world) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this ok? Surely we should check that both params are valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I should have read above in the PR discussion, can you just add a to-do or at least a comment explanation why we've decided to do this?

crates/bevy_ecs/src/system/combinator.rs
@@ -433,7 +433,7 @@ where

unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
// SAFETY: Delegate to other `System` implementations.
unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
unsafe { self.a.validate_param_unsafe(world) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@alice-i-cecile 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andriyDev andriyDev andriyDev left review comments

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

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@Diddykonga Diddykonga Diddykonga approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Milestone
0.15.1
Development

Successfully merging this pull request may close these issues.

Option to toggle/revert to 0.14 behaviour of Fallible SystemParams
5 participants
@MiniaczQ @alice-i-cecile @Diddykonga @bushrat011899 @andriyDev