From 460de77a552d00894ab84557fbfbdeea3f41b7c1 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Tue, 24 Dec 2024 03:36:03 +0100 Subject: [PATCH] Set panic as default fallible system param behavior (#16638) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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 Co-authored-by: François Mockers --- crates/bevy_ecs/src/change_detection.rs | 6 ++-- crates/bevy_ecs/src/schedule/condition.rs | 4 +-- crates/bevy_ecs/src/schedule/executor/mod.rs | 18 +++++------ crates/bevy_ecs/src/system/combinator.rs | 4 +-- crates/bevy_ecs/src/system/function_system.rs | 32 +++++++++++++------ crates/bevy_ecs/src/system/query.rs | 4 +-- crates/bevy_ecs/src/system/system.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 2 +- crates/bevy_ecs/src/system/system_registry.rs | 2 +- crates/bevy_pbr/src/render/mesh.rs | 4 +++ examples/ecs/fallible_params.rs | 14 ++++---- 11 files changed, 53 insertions(+), 39 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 025c3804e73e2..72cf8c6784edb 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -544,7 +544,7 @@ impl<'w> From> 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>`] 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>`] instead if the resource might not always exist. pub struct ResMut<'w, T: ?Sized + Resource> { @@ -683,7 +683,7 @@ impl<'w, T: Resource> From> 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>`] 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 index 4f7745b1d687e..4cfdd1cc81f0f 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -80,7 +80,7 @@ pub trait Condition: sealed::Condition /// /// # Examples /// - /// ``` + /// ```should_panic /// use bevy_ecs::prelude::*; /// /// #[derive(Resource, PartialEq)] @@ -90,7 +90,7 @@ pub trait Condition: sealed::Condition /// # 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` 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 index 9218fe33fe0da..f87bbcd10d425 100644 --- 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| {}), // 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| true)); + schedule.configure_sets(S1.run_if((|_: Res| 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| true), + .param_warn_once() + .run_if((|_: Res| true).param_warn_once()), )); schedule.run(&mut world); assert!(world.get_resource::().is_none()); diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 9d8652e9dbc00..1f956c5d4c00e 100644 --- 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 index 3d029d74a9577..eeee2f1c78491 100644 --- 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::

() + ), + Self::Once => { + log::warn!( + "{0} did not run because it requested inaccessible system parameter {1}", + name, + disqualified::ShortName::of::

() + ); + } + Self::Never => {} } - - log::warn!( - "{0} did not run because it requested inaccessible system parameter {1}", - name, - disqualified::ShortName::of::

() - ); } } @@ -232,6 +241,11 @@ where /// Set warn policy. fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem; + /// Warn only once about invalid system parameters. + fn param_warn_once(self) -> FunctionSystem { + self.with_param_warn_policy(ParamWarnPolicy::Once) + } + /// Disable all param warnings. fn never_param_warn(self) -> FunctionSystem { 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 index 6fc2a4caa5d47..cc4312761ead4 100644 --- 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>`] 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 index 6cef4da705399..e0aa69b660db1 100644 --- 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 index 2b38fc6201146..5af22674afa7c 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1440,7 +1440,7 @@ unsafe impl 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>`] 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 index 6cc7556bfda2b..f924618a7182c 100644 --- 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) {} 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 index 2d4047fee6227..0ec344410cdcf 100644 --- 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 index 15a75944f01ae..129a3859d495f 100644 --- 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(); }