Reduce runtime panics through SystemParam validation#15276
Reduce runtime panics through SystemParam validation#15276alice-i-cecile merged 33 commits intobevyengine:mainfrom
SystemParam validation#15276Conversation
alice-i-cecile
added
A-ECS
|
The opinions of the SME-ECS (@JoJoJet @maniwani @james7132 and myself) and frankly the rest of the ECS community are very positive on the broad direction of this work, so I'm marking it as |
|
|
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Very pleased about how non-intrusive this is!
hymm
left a comment
hymm
left a comment
There was a problem hiding this comment.
I made some comments on the safety for the single_threaded executor and I believe the simple executor has the same issues. It's late here so I might be missing something obvious, but I don't see update_archetype_component_access being called for the conditions for either of those two executors.
alice-i-cecile
added
S-Ready-For-Final-Review
crates/bevy_ecs/src/system/system.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// - The caller must ensure that `world` has permission to read any world data | ||
| /// registered in [`Self::archetype_component_access`]. There must be no conflicting |
There was a problem hiding this comment.
Why does this mention update_archetype_component_access while the other validate_params mention init_state?
There was a problem hiding this comment.
This was copy-paste from get_param/run methods, this means that they are also inconsistent with each other.
Agreed it should refer to update_archetype_component_access and some reference to new_archetype instead.
There was a problem hiding this comment.
Addressed, let me know if that's sufficient.
If not I'm afraid we'll need a follow up PR since Alice wants this merged in the next train
There was a problem hiding this comment.
I guess it's good enough now, I'm ok with merging it and maybe improve the wording in a follow up PR
alice-i-cecile
added
S-Waiting-on-Author
alice-i-cecile
added
S-Ready-For-Final-Review
MiniaczQ
removed
the
M-Migration-Guide
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1691 if you'd like to help out. |
9 participants
Objective
The goal of this PR is to introduce
SystemParamvalidation in order to reduce runtime panics.Fixes #15265
Solution
SystemParamnow has a new methodvalidate_param(...) -> bool, which takes immutable variants ofget_paramarguments. The returned value indicates whether the parameter can be acquired from the world. If parameters cannot be acquired for a system, it won't be executed, similarly to run conditions. This reduces panics when using params likeRes,ResMut, etc. as well as allows for new, ergonomic params like #15264 or #15302.Param validation happens at the level of executors. All validation happens directly before executing a system, in case of normal systems they are skipped, in case of conditions they return false.
Warning about system skipping is primitive and subject to change in subsequent PRs.
Testing
Two executor tests check that all executors: