Set panic as default fallible system param behavior (#16638)

# 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. --------- Co-authored-by: François Mockers <[email protected]> Co-authored-by: François Mockers <[email protected]>
bevyengine · Dec 24, 2024 · 460de77 · 460de77
1 parent ff57e80
commit 460de77
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 39 deletions.
diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs
@@ -544,7 +544,7 @@ impl<'w> From<TicksMut<'w>> for Ticks<'w> {
 /// If you need a unique mutable borrow, use [`ResMut`] instead.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<Res<T>>`] instead if the resource might not always exist.
 pub struct Res<'w, T: ?Sized + Resource> {
@@ -622,7 +622,7 @@ impl_debug!(Res<'w, T>, Resource);
 /// If you need a shared borrow, use [`Res`] instead.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<ResMut<T>>`] instead if the resource might not always exist.
 pub struct ResMut<'w, T: ?Sized + Resource> {
@@ -683,7 +683,7 @@ impl<'w, T: Resource> From<ResMut<'w, T>> for Mut<'w, T> {
 /// over to another thread.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if non-send resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<NonSendMut<T>>`] instead if the resource might not always exist.
 pub struct NonSendMut<'w, T: ?Sized + 'static> {

diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs
@@ -80,7 +80,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
     ///
     /// # Examples
     ///
-    /// ```
+    /// ```should_panic
     /// use bevy_ecs::prelude::*;
     ///
     /// #[derive(Resource, PartialEq)]
@@ -90,7 +90,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
     /// # let mut world = World::new();
     /// # fn my_system() {}
     /// app.add_systems(
-    ///     // The `resource_equals` run condition will fail since we don't initialize `R`,
+    ///     // The `resource_equals` run condition will panic since we don't initialize `R`,
     ///     // just like if we used `Res<R>` in a system.
     ///     my_system.run_if(resource_equals(R(0))),
     /// );

diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs
@@ -308,7 +308,7 @@ mod tests {
         self as bevy_ecs,
         prelude::{IntoSystemConfigs, IntoSystemSetConfigs, Resource, Schedule, SystemSet},
         schedule::ExecutorKind,
-        system::{Commands, In, IntoSystem, Res},
+        system::{Commands, Res, WithParamWarnPolicy},
         world::World,
     };
 
@@ -337,15 +337,11 @@ mod tests {
         schedule.set_executor_kind(executor);
         schedule.add_systems(
             (
-                // Combined systems get skipped together.
-                (|mut commands: Commands| {
-                    commands.insert_resource(R1);
-                })
-                .pipe(|_: In<()>, _: Res<R1>| {}),
                 // This system depends on a system that is always skipped.
-                |mut commands: Commands| {
+                (|mut commands: Commands| {
                     commands.insert_resource(R2);
-                },
+                })
+                .param_warn_once(),
             )
                 .chain(),
         );
@@ -368,18 +364,20 @@ mod tests {
         let mut world = World::new();
         let mut schedule = Schedule::default();
         schedule.set_executor_kind(executor);
-        schedule.configure_sets(S1.run_if(|_: Res<R1>| true));
+        schedule.configure_sets(S1.run_if((|_: Res<R1>| true).param_warn_once()));
         schedule.add_systems((
             // System gets skipped if system set run conditions fail validation.
             (|mut commands: Commands| {
                 commands.insert_resource(R1);
             })
+            .param_warn_once()
             .in_set(S1),
             // System gets skipped if run conditions fail validation.
             (|mut commands: Commands| {
                 commands.insert_resource(R2);
             })
-            .run_if(|_: Res<R2>| true),
+            .param_warn_once()
+            .run_if((|_: Res<R2>| true).param_warn_once()),
         ));
         schedule.run(&mut world);
         assert!(world.get_resource::<R1>().is_none());

diff --git a/crates/bevy_ecs/src/system/combinator.rs b/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) }
     }
 
     fn initialize(&mut self, world: &mut World) {
@@ -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) }
     }
 
     fn validate_param(&mut self, world: &World) -> bool {

diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs
@@ -60,7 +60,7 @@ impl SystemMeta {
             is_send: true,
             has_deferred: false,
             last_run: Tick::new(0),
-            param_warn_policy: ParamWarnPolicy::Once,
+            param_warn_policy: ParamWarnPolicy::Panic,
             #[cfg(feature = "trace")]
             system_span: info_span!("system", name = name),
             #[cfg(feature = "trace")]
@@ -190,6 +190,8 @@ impl SystemMeta {
 /// State machine for emitting warnings when [system params are invalid](System::validate_param).
 #[derive(Clone, Copy)]
 pub enum ParamWarnPolicy {
+    /// Stop app with a panic.
+    Panic,
     /// No warning should ever be emitted.
     Never,
     /// The warning will be emitted once and status will update to [`Self::Never`].
@@ -200,6 +202,7 @@ impl ParamWarnPolicy {
     /// Advances the warn policy after validation failed.
     #[inline]
     fn advance(&mut self) {
+        // Ignore `Panic` case, because it stops execution before this function gets called.
         *self = Self::Never;
     }
 
@@ -209,15 +212,21 @@ impl ParamWarnPolicy {
     where
         P: SystemParam,
     {
-        if matches!(self, Self::Never) {
-            return;
+        match self {
+            Self::Panic => panic!(
+                "{0} could not access system parameter {1}",
+                name,
+                disqualified::ShortName::of::<P>()
+            ),
+            Self::Once => {
+                log::warn!(
+                    "{0} did not run because it requested inaccessible system parameter {1}",
+                    name,
+                    disqualified::ShortName::of::<P>()
+                );
+            }
+            Self::Never => {}
         }
-
-        log::warn!(
-            "{0} did not run because it requested inaccessible system parameter {1}",
-            name,
-            disqualified::ShortName::of::<P>()
-        );
     }
 }
 
@@ -232,6 +241,11 @@ where
     /// Set warn policy.
     fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F>;
 
+    /// Warn only once about invalid system parameters.
+    fn param_warn_once(self) -> FunctionSystem<M, F> {
+        self.with_param_warn_policy(ParamWarnPolicy::Once)
+    }
+
     /// Disable all param warnings.
     fn never_param_warn(self) -> FunctionSystem<M, F> {
         self.with_param_warn_policy(ParamWarnPolicy::Never)

diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs
@@ -1890,7 +1890,7 @@ impl<'w, 'q, Q: QueryData, F: QueryFilter> From<&'q mut Query<'w, '_, Q, F>>
 /// [System parameter] that provides access to single entity's components, much like [`Query::single`]/[`Query::single_mut`].
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if zero or more than one matching entity exists.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<Single<D, F>>`] instead if zero or one matching entities can exist.
 ///
@@ -1926,7 +1926,7 @@ impl<'w, D: QueryData, F: QueryFilter> Single<'w, D, F> {
 /// [System parameter] that works very much like [`Query`] except it always contains at least one matching entity.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if no matching entities exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Much like [`Query::is_empty`] the worst case runtime will be `O(n)` where `n` is the number of *potential* matches.
 /// This can be notably expensive for queries that rely on non-archetypal filters such as [`Added`](crate::query::Added) or [`Changed`](crate::query::Changed)

diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs
@@ -450,7 +450,7 @@ mod tests {
 
         let mut world = World::default();
         // This fails because `T` has not been added to the world yet.
-        let result = world.run_system_once(system);
+        let result = world.run_system_once(system.param_warn_once());
 
         assert!(matches!(result, Err(RunSystemError::InvalidParams(_))));
     }

diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs
@@ -1440,7 +1440,7 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
 /// over to another thread.
 ///
 /// This [`SystemParam`] fails validation if non-send resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<NonSend<T>>`] instead if the resource might not always exist.
 pub struct NonSend<'w, T: 'static> {

diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs
@@ -998,7 +998,7 @@ mod tests {
         fn system(_: Res<T>) {}
 
         let mut world = World::new();
-        let id = world.register_system_cached(system);
+        let id = world.register_system(system.param_warn_once());
         // This fails because `T` has not been added to the world yet.
         let result = world.run_system(id);
 

diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs
@@ -137,6 +137,10 @@ impl Plugin for MeshRenderPlugin {
         load_internal_asset!(app, SKINNING_HANDLE, "skinning.wgsl", Shader::from_wgsl);
         load_internal_asset!(app, MORPH_HANDLE, "morph.wgsl", Shader::from_wgsl);
 
+        if app.get_sub_app(RenderApp).is_none() {
+            return;
+        }
+
         app.add_systems(
             PostUpdate,
             (no_automatic_skin_batching, no_automatic_morph_batching),

diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs
@@ -20,22 +20,20 @@ fn main() {
     App::new()
         .add_plugins(DefaultPlugins)
         .add_systems(Startup, setup)
-        // Systems that fail parameter validation will emit warnings.
-        // The default policy is to emit a warning once per system.
-        // This is good for catching unexpected behavior, but can
-        // lead to spam. You can disable invalid param warnings
-        // per system using the `.never_param_warn()` method.
+        // Default system policy is to panic if parameters fail to be fetched.
+        // We overwrite that configuration, to either warn us once or never.
+        // This is good for catching unexpected behavior without crashing the app,
+        // but can lead to spam.
         .add_systems(
             Update,
             (
-                user_input,
+                user_input.param_warn_once(),
                 move_targets.never_param_warn(),
                 move_pointer.never_param_warn(),
             )
                 .chain(),
         )
-        // We will leave this systems with default warning policy.
-        .add_systems(Update, do_nothing_fail_validation)
+        .add_systems(Update, do_nothing_fail_validation.param_warn_once())
         .run();
 }