From c3329c2bc57a985ecd41bb97ccb32aa5f5ad040b Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 02:43:33 +0200 Subject: [PATCH 01/11] update to new convention --- crates/bevy_ecs/src/schedule/executor/mod.rs | 12 ---- .../src/schedule/executor/multi_threaded.rs | 5 +- .../bevy_ecs/src/schedule/executor/simple.rs | 5 -- .../src/schedule/executor/single_threaded.rs | 5 -- crates/bevy_ecs/src/system/adapter_system.rs | 2 +- crates/bevy_ecs/src/system/combinator.rs | 4 +- .../src/system/exclusive_function_system.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 72 ++++++++++++++++++- crates/bevy_ecs/src/system/system.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 52 ++++++++++---- crates/bevy_render/src/extract_param.rs | 3 +- 11 files changed, 116 insertions(+), 48 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 24113aee2bee0..37fd9ff7246c8 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -180,18 +180,6 @@ mod __rust_begin_short_backtrace { } } -#[macro_export] -/// Emits a warning about system being skipped. -macro_rules! warn_system_skipped { - ($ty:literal, $sys:expr) => { - bevy_utils::tracing::warn!( - "{} {} was skipped due to inaccessible system parameters.", - $ty, - $sys - ) - }; -} - #[cfg(test)] mod tests { use crate::{ diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 8782793e32f71..53453240dcb88 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -19,7 +19,6 @@ use crate::{ query::Access, schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule}, system::BoxedSystem, - warn_system_skipped, world::{unsafe_world_cell::UnsafeWorldCell, World}, }; @@ -524,7 +523,7 @@ impl ExecutorState { unsafe fn should_run( &mut self, system_index: usize, - system: &BoxedSystem, + system: &mut BoxedSystem, conditions: &mut Conditions, world: UnsafeWorldCell, ) -> bool { @@ -575,7 +574,6 @@ impl ExecutorState { // - `update_archetype_component_access` has been called for system. let valid_params = unsafe { system.validate_param_unsafe(world) }; if !valid_params { - warn_system_skipped!("System", system.name()); self.skipped_systems.insert(system_index); } should_run &= valid_params; @@ -751,7 +749,6 @@ unsafe fn evaluate_and_fold_conditions( // required by the condition. // - `update_archetype_component_access` has been called for condition. if !unsafe { condition.validate_param_unsafe(world) } { - warn_system_skipped!("Condition", condition.name()); return false; } // SAFETY: diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 171125342cd78..508f1fbffd07a 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/crates/bevy_ecs/src/schedule/executor/simple.rs @@ -7,7 +7,6 @@ use crate::{ schedule::{ executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, }, - warn_system_skipped, world::World, }; @@ -83,9 +82,6 @@ impl SystemExecutor for SimpleExecutor { let system = &mut schedule.systems[system_index]; if should_run { let valid_params = system.validate_param(world); - if !valid_params { - warn_system_skipped!("System", system.name()); - } should_run &= valid_params; } @@ -139,7 +135,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W .iter_mut() .map(|condition| { if !condition.validate_param(world) { - warn_system_skipped!("Condition", condition.name()); return false; } __rust_begin_short_backtrace::readonly_run(&mut **condition, world) diff --git a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs index 93d814d3b2f83..9cc6199ce6a4a 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -5,7 +5,6 @@ use fixedbitset::FixedBitSet; use crate::{ schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule}, - warn_system_skipped, world::World, }; @@ -89,9 +88,6 @@ impl SystemExecutor for SingleThreadedExecutor { let system = &mut schedule.systems[system_index]; if should_run { let valid_params = system.validate_param(world); - if !valid_params { - warn_system_skipped!("System", system.name()); - } should_run &= valid_params; } @@ -171,7 +167,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W .iter_mut() .map(|condition| { if !condition.validate_param(world) { - warn_system_skipped!("Condition", condition.name()); return false; } __rust_begin_short_backtrace::readonly_run(&mut **condition, world) diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 8c25e57bbd91e..dfb111302a699 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -179,7 +179,7 @@ where } #[inline] - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { self.system.validate_param_unsafe(world) } diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index ce4c8785feb05..e586b2c46e8d2 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -212,7 +212,7 @@ where } #[inline] - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) } @@ -430,7 +430,7 @@ where self.b.queue_deferred(world); } - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) } diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 3075f3c55772e..4f90653f2a7c8 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -150,7 +150,7 @@ where } #[inline] - unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { true } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 0614d7339ca4b..78e26e0d49dde 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -43,6 +43,7 @@ pub struct SystemMeta { is_send: bool, has_deferred: bool, pub(crate) last_run: Tick, + warn_policy: WarnPolicy, #[cfg(feature = "trace")] pub(crate) system_span: Span, #[cfg(feature = "trace")] @@ -59,6 +60,7 @@ impl SystemMeta { is_send: true, has_deferred: false, last_run: Tick::new(0), + warn_policy: WarnPolicy::Once, #[cfg(feature = "trace")] system_span: info_span!("system", name = name), #[cfg(feature = "trace")] @@ -75,6 +77,7 @@ impl SystemMeta { /// Sets the name of of this system. /// /// Useful to give closure systems more readable and unique names for debugging and tracing. + #[inline] pub fn set_name(&mut self, new_name: impl Into>) { let new_name: Cow<'static, str> = new_name.into(); #[cfg(feature = "trace")] @@ -108,9 +111,70 @@ impl SystemMeta { /// Marks the system as having deferred buffers like [`Commands`](`super::Commands`) /// This lets the scheduler insert [`apply_deferred`](`crate::prelude::apply_deferred`) systems automatically. + #[inline] pub fn set_has_deferred(&mut self) { self.has_deferred = true; } + + /// Changes the warn policy. + #[inline] + pub fn set_warn_policy(&mut self, warn_policy: WarnPolicy) { + self.warn_policy = warn_policy; + } + + /// Advances the warn policy after validation failed. + #[inline] + pub fn advance_warn_policy(&mut self) { + self.warn_policy.advance(); + } + + /// Emits a warning about inaccessible system param if policy allows it. + #[inline] + pub fn try_warn

(&self) + where + P: SystemParam, + { + self.warn_policy.try_warn::

(&self.name); + } +} + +/// State machine for emitting warnings when [system params are invalid](System::validate_param). +#[derive(Clone, Copy)] +pub enum WarnPolicy { + /// No warning should ever be emitted. + Never, + /// The warning will be emitted once and status will update to [`Self::Never`]. + Once, + /// The warning will be emitted every time. + Always, +} + +impl WarnPolicy { + /// Advances the warn policy after validation failed. + #[inline] + fn advance(&mut self) { + *self = match self { + Self::Never => Self::Never, + Self::Once => Self::Never, + Self::Always => Self::Always, + }; + } + + /// Emits a warning about inaccessible system param if policy allows it. + #[inline] + fn try_warn

(&self, name: &str) + where + P: SystemParam, + { + if matches!(self, Self::Never) { + return; + } + bevy_utils::tracing::warn!( + "System {0} will not run because it requested inaccessible system parameter {1}", + name, + std::any::type_name::

() + ); + } } // TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference @@ -657,9 +721,13 @@ where } #[inline] - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE); - F::Param::validate_param(param_state, &self.system_meta, world) + let is_valid = unsafe { F::Param::validate_param(param_state, &self.system_meta, world) }; + if !is_valid { + self.system_meta.advance_warn_policy(); + } + is_valid } #[inline] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 689c9f725f208..74c18173ab15e 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -117,7 +117,7 @@ pub trait System: Send + Sync + 'static { /// - The method [`System::update_archetype_component_access`] must be called at some /// point before this one, with the same exact [`World`]. If [`System::update_archetype_component_access`] /// panics (or otherwise does not return for any reason), this method must not be called. - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool; + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool; /// Safe version of [`System::validate_param_unsafe`]. /// that runs on exclusive, single-threaded `world` pointer. diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 2d05a23778db0..66d044c1f248b 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -419,7 +419,11 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo world.change_tick(), ) }; - result.is_ok() + let is_valid = result.is_ok(); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } } @@ -481,7 +485,11 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam world.change_tick(), ) }; - !matches!(result, Err(QuerySingleError::MultipleEntities(_))) + let is_valid = !matches!(result, Err(QuerySingleError::MultipleEntities(_))); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } } @@ -756,14 +764,18 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } #[inline] @@ -866,14 +878,18 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } #[inline] @@ -1412,14 +1428,18 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .non_send_resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } #[inline] @@ -1519,14 +1539,18 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .non_send_resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn::(); + } + is_valid } #[inline] diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 914fe32dfb6e4..e8ec5b457f1e9 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -77,12 +77,13 @@ where #[inline] unsafe fn validate_param( state: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to world data registered in `init_state`. let result = unsafe { world.get_resource_by_id(state.main_world_state) }; let Some(main_world) = result else { + system_meta.try_warn::<&World>(); return false; }; // SAFETY: Type is guaranteed by `SystemState`. From 5f1cdb57340f6231bd3d24fa9cde78ae53fafa4d Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 03:10:35 +0200 Subject: [PATCH 02/11] fix docs --- crates/bevy_ecs/src/system/exclusive_function_system.rs | 1 + crates/bevy_ecs/src/system/function_system.rs | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 4f90653f2a7c8..8b1a06fb3a60c 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -151,6 +151,7 @@ where #[inline] unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + // All exclusive system params are always available. true } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 78e26e0d49dde..04db17d95d324 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -154,8 +154,7 @@ impl WarnPolicy { #[inline] fn advance(&mut self) { *self = match self { - Self::Never => Self::Never, - Self::Once => Self::Never, + Self::Never | Self::Once => Self::Never, Self::Always => Self::Always, }; } @@ -172,7 +171,7 @@ impl WarnPolicy { bevy_utils::tracing::warn!( "System {0} will not run because it requested inaccessible system parameter {1}", name, - std::any::type_name::

() + core::any::type_name::

() ); } } @@ -723,6 +722,7 @@ where #[inline] unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE); + // SAFETY: Delegate to `SystemParam` implementation. let is_valid = unsafe { F::Param::validate_param(param_state, &self.system_meta, world) }; if !is_valid { self.system_meta.advance_warn_policy(); From 158de13783fc57749a2355fc5092ff26405c92a1 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 04:15:58 +0200 Subject: [PATCH 03/11] disableable warnings --- crates/bevy_ecs/Cargo.toml | 3 +- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 48 +++++++++++++++++++ examples/ecs/fallible_params.rs | 38 +++++++++++---- 4 files changed, 81 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index 9b10c1f981e32..df8ce8bfd6fb9 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -11,10 +11,11 @@ categories = ["game-engines", "data-structures"] rust-version = "1.77.0" [features] -default = ["bevy_reflect"] +default = ["bevy_reflect", "bevy_warn_invalid_param"] trace = [] multi_threaded = ["bevy_tasks/multi_threaded", "arrayvec"] bevy_debug_stepping = [] +bevy_warn_invalid_param = [] serialize = ["dep:serde"] track_change_detection = [] reflect_functions = ["bevy_reflect", "bevy_reflect/functions"] diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 82231a503e7ab..b7808c1c9f8d2 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -60,7 +60,7 @@ pub mod prelude { Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder, - SystemParamFunction, + SystemParamFunction, WithWarnPolicy, }, world::{ Command, EntityMut, EntityRef, EntityWorldMut, FromWorld, OnAdd, OnInsert, OnRemove, diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 04db17d95d324..5656b2101b128 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -115,7 +115,10 @@ impl SystemMeta { pub fn set_has_deferred(&mut self) { self.has_deferred = true; } +} +#[cfg(feature = "bevy_warn_invalid_param")] +impl SystemMeta { /// Changes the warn policy. #[inline] pub fn set_warn_policy(&mut self, warn_policy: WarnPolicy) { @@ -138,6 +141,17 @@ impl SystemMeta { } } +// No-op when warnings are disabled. +#[cfg(not(feature = "bevy_warn_invalid_param"))] +impl SystemMeta { + #[inline] + pub fn set_warn_policy(&mut self, _warn_policy: WarnPolicy) {} + #[inline] + pub fn advance_warn_policy(&mut self) {} + #[inline] + pub fn try_warn(&self) {} +} + /// State machine for emitting warnings when [system params are invalid](System::validate_param). #[derive(Clone, Copy)] pub enum WarnPolicy { @@ -176,6 +190,40 @@ impl WarnPolicy { } } +/// Trait for manipulating warn policy of systems. +#[doc(hidden)] +pub trait WithWarnPolicy +where + M: 'static, + F: SystemParamFunction, + Self: Sized, +{ + /// Set warn policy. + fn with_warn_policy(self, warn_policy: WarnPolicy) -> FunctionSystem; + + /// Disable all warnings. + fn never_warn(self) -> FunctionSystem { + self.with_warn_policy(WarnPolicy::Never) + } + + /// Show all warnings. + fn always_warn(self) -> FunctionSystem { + self.with_warn_policy(WarnPolicy::Always) + } +} + +impl WithWarnPolicy for F +where + M: 'static, + F: SystemParamFunction, +{ + fn with_warn_policy(self, warn_policy: WarnPolicy) -> FunctionSystem { + let mut system = IntoSystem::into_system(self); + system.system_meta.warn_policy = warn_policy; + system + } +} + // TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference // (to avoid the need for unwrapping to retrieve SystemMeta) /// Holds on to persistent state required to drive [`SystemParam`] for a [`System`]. diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index 06fefc04418a3..cecb17c4e4ec3 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -19,9 +19,27 @@ fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, setup) - // We add all the systems one after another. - // We don't need to use run conditions here. - .add_systems(Update, (user_input, move_targets, move_pointer).chain()) + // 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 and performance drop due to additional checks. + // + // There are two ways of disabling warnings: + // - During development, it's preferential to disable warnings selectively per system + // using the `.never_warn()` method. + // - When releasing the game, it's best to disable all warnings by removing the + // `bevy_warn_invalid_param` feature flag, which is enabled by default in `bevy_ecs`. + .add_systems( + Update, + ( + user_input, + move_targets.never_warn(), + move_pointer.never_warn(), + ) + .chain(), + ) + // We will leave this systems with default warning policy. + .add_systems(Update, do_nothing_fail_validation) .run(); } @@ -66,9 +84,9 @@ fn setup(mut commands: Commands, asset_server: Res) { )); } -// System that reads user input. -// If user presses 'A' we spawn a new random enemy. -// If user presses 'R' we remove a random enemy (if any exist). +/// System that reads user input. +/// If user presses 'A' we spawn a new random enemy. +/// If user presses 'R' we remove a random enemy (if any exist). fn user_input( mut commands: Commands, enemies: Query>, @@ -104,8 +122,8 @@ fn user_input( } } -// System that moves the enemies in a circle. -// TODO: Use [`NonEmptyQuery`] when it exists. +/// System that moves the enemies in a circle. +/// TODO: Use `Populated` when it exists. fn move_targets(mut enemies: Query<(&mut Transform, &mut Enemy)>, time: Res

(&self) + pub fn try_warn_param

(&self) where P: SystemParam, { - self.warn_policy.try_warn::

(&self.name); + self.param_warn_policy.try_warn::

(&self.name); } } @@ -147,31 +147,28 @@ impl SystemMeta { #[cfg(not(feature = "bevy_warn_invalid_param"))] impl SystemMeta { #[inline] - pub fn set_warn_policy(&mut self, _warn_policy: WarnPolicy) {} + pub fn set_warn_policy(&mut self, _warn_policy: ParamWarnPolicy) {} #[inline] - pub fn advance_warn_policy(&mut self) {} + pub fn advance_param_warn_policy(&mut self) {} #[inline] - pub fn try_warn(&self) {} + pub fn try_warn_param(&self) {} } /// State machine for emitting warnings when [system params are invalid](System::validate_param). #[derive(Clone, Copy)] -pub enum WarnPolicy { +pub enum ParamWarnPolicy { /// No warning should ever be emitted. Never, /// The warning will be emitted once and status will update to [`Self::Never`]. Once, - /// The warning will be emitted every time. - Always, } -impl WarnPolicy { +impl ParamWarnPolicy { /// Advances the warn policy after validation failed. #[inline] fn advance(&mut self) { *self = match self { Self::Never | Self::Once => Self::Never, - Self::Always => Self::Always, }; } @@ -194,34 +191,29 @@ impl WarnPolicy { /// Trait for manipulating warn policy of systems. #[doc(hidden)] -pub trait WithWarnPolicy +pub trait WithParamWarnPolicy where M: 'static, F: SystemParamFunction, Self: Sized, { /// Set warn policy. - fn with_warn_policy(self, warn_policy: WarnPolicy) -> FunctionSystem; + fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem; - /// Disable all warnings. - fn never_warn(self) -> FunctionSystem { - self.with_warn_policy(WarnPolicy::Never) - } - - /// Show all warnings. - fn always_warn(self) -> FunctionSystem { - self.with_warn_policy(WarnPolicy::Always) + /// Disable all param warnings. + fn never_param_warn(self) -> FunctionSystem { + self.with_param_warn_policy(ParamWarnPolicy::Never) } } -impl WithWarnPolicy for F +impl WithParamWarnPolicy for F where M: 'static, F: SystemParamFunction, { - fn with_warn_policy(self, warn_policy: WarnPolicy) -> FunctionSystem { + fn with_param_warn_policy(self, param_warn_policy: ParamWarnPolicy) -> FunctionSystem { let mut system = IntoSystem::into_system(self); - system.system_meta.warn_policy = warn_policy; + system.system_meta.param_warn_policy = param_warn_policy; system } } @@ -779,7 +771,7 @@ where // will ensure that there are no data access conflicts. let is_valid = unsafe { F::Param::validate_param(param_state, &self.system_meta, world) }; if !is_valid { - self.system_meta.advance_warn_policy(); + self.system_meta.advance_param_warn_policy(); } is_valid } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 66d044c1f248b..4f3912c9563cb 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -421,7 +421,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo }; let is_valid = result.is_ok(); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } @@ -487,7 +487,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam }; let is_valid = !matches!(result, Err(QuerySingleError::MultipleEntities(_))); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } @@ -773,7 +773,7 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { .get(component_id) .is_some_and(ResourceData::is_present); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } @@ -887,7 +887,7 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { .get(component_id) .is_some_and(ResourceData::is_present); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } @@ -1437,7 +1437,7 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { .get(component_id) .is_some_and(ResourceData::is_present); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } @@ -1548,7 +1548,7 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { .get(component_id) .is_some_and(ResourceData::is_present); if !is_valid { - system_meta.try_warn::(); + system_meta.try_warn_param::(); } is_valid } diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index e8ec5b457f1e9..89b6edef1903d 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -83,7 +83,7 @@ where // SAFETY: Read-only access to world data registered in `init_state`. let result = unsafe { world.get_resource_by_id(state.main_world_state) }; let Some(main_world) = result else { - system_meta.try_warn::<&World>(); + system_meta.try_warn_param::<&World>(); return false; }; // SAFETY: Type is guaranteed by `SystemState`. diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index cecb17c4e4ec3..b4c1b2e3cfb5d 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -26,15 +26,15 @@ fn main() { // // There are two ways of disabling warnings: // - During development, it's preferential to disable warnings selectively per system - // using the `.never_warn()` method. + // using the `.never_param_warn()` method. // - When releasing the game, it's best to disable all warnings by removing the // `bevy_warn_invalid_param` feature flag, which is enabled by default in `bevy_ecs`. .add_systems( Update, ( user_input, - move_targets.never_warn(), - move_pointer.never_warn(), + move_targets.never_param_warn(), + move_pointer.never_param_warn(), ) .chain(), ) From 137c53f421bab0a17a5b5ffcb087decbadab169f Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 12:41:42 +0200 Subject: [PATCH 06/11] rename feature flag --- crates/bevy_ecs/Cargo.toml | 4 ++-- crates/bevy_ecs/src/system/function_system.rs | 8 ++++---- examples/ecs/fallible_params.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index df8ce8bfd6fb9..048e4e5e0b36f 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -11,11 +11,11 @@ categories = ["game-engines", "data-structures"] rust-version = "1.77.0" [features] -default = ["bevy_reflect", "bevy_warn_invalid_param"] +default = ["bevy_reflect", "bevy_param_warn"] trace = [] multi_threaded = ["bevy_tasks/multi_threaded", "arrayvec"] bevy_debug_stepping = [] -bevy_warn_invalid_param = [] +bevy_param_warn = [] serialize = ["dep:serde"] track_change_detection = [] reflect_functions = ["bevy_reflect", "bevy_reflect/functions"] diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 5d9da46d6755f..1926f22e4c63f 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -43,7 +43,7 @@ pub struct SystemMeta { is_send: bool, has_deferred: bool, pub(crate) last_run: Tick, - #[cfg(feature = "bevy_warn_invalid_param")] + #[cfg(feature = "bevy_param_warn")] param_warn_policy: ParamWarnPolicy, #[cfg(feature = "trace")] pub(crate) system_span: Span, @@ -61,7 +61,7 @@ impl SystemMeta { is_send: true, has_deferred: false, last_run: Tick::new(0), - #[cfg(feature = "bevy_warn_invalid_param")] + #[cfg(feature = "bevy_param_warn")] param_warn_policy: ParamWarnPolicy::Once, #[cfg(feature = "trace")] system_span: info_span!("system", name = name), @@ -119,7 +119,7 @@ impl SystemMeta { } } -#[cfg(feature = "bevy_warn_invalid_param")] +#[cfg(feature = "bevy_param_warn")] impl SystemMeta { /// Changes the warn policy. #[inline] @@ -144,7 +144,7 @@ impl SystemMeta { } // No-op when warnings are disabled. -#[cfg(not(feature = "bevy_warn_invalid_param"))] +#[cfg(not(feature = "bevy_param_warn"))] impl SystemMeta { #[inline] pub fn set_warn_policy(&mut self, _warn_policy: ParamWarnPolicy) {} diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index b4c1b2e3cfb5d..cc67739c43a6e 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -28,7 +28,7 @@ fn main() { // - During development, it's preferential to disable warnings selectively per system // using the `.never_param_warn()` method. // - When releasing the game, it's best to disable all warnings by removing the - // `bevy_warn_invalid_param` feature flag, which is enabled by default in `bevy_ecs`. + // `bevy_param_warn` feature flag, which is enabled by default in `bevy_ecs`. .add_systems( Update, ( From 5b1c2289bcddf72bad508f6360dada2d976b9590 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 18:48:00 +0200 Subject: [PATCH 07/11] remove feature --- crates/bevy_ecs/Cargo.toml | 3 +-- crates/bevy_ecs/src/system/function_system.rs | 16 +--------------- examples/ecs/fallible_params.rs | 9 ++------- 3 files changed, 4 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index 048e4e5e0b36f..9b10c1f981e32 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -11,11 +11,10 @@ categories = ["game-engines", "data-structures"] rust-version = "1.77.0" [features] -default = ["bevy_reflect", "bevy_param_warn"] +default = ["bevy_reflect"] trace = [] multi_threaded = ["bevy_tasks/multi_threaded", "arrayvec"] bevy_debug_stepping = [] -bevy_param_warn = [] serialize = ["dep:serde"] track_change_detection = [] reflect_functions = ["bevy_reflect", "bevy_reflect/functions"] diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1926f22e4c63f..15490043d5ac5 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -43,7 +43,6 @@ pub struct SystemMeta { is_send: bool, has_deferred: bool, pub(crate) last_run: Tick, - #[cfg(feature = "bevy_param_warn")] param_warn_policy: ParamWarnPolicy, #[cfg(feature = "trace")] pub(crate) system_span: Span, @@ -61,7 +60,6 @@ impl SystemMeta { is_send: true, has_deferred: false, last_run: Tick::new(0), - #[cfg(feature = "bevy_param_warn")] param_warn_policy: ParamWarnPolicy::Once, #[cfg(feature = "trace")] system_span: info_span!("system", name = name), @@ -119,7 +117,6 @@ impl SystemMeta { } } -#[cfg(feature = "bevy_param_warn")] impl SystemMeta { /// Changes the warn policy. #[inline] @@ -143,17 +140,6 @@ impl SystemMeta { } } -// No-op when warnings are disabled. -#[cfg(not(feature = "bevy_param_warn"))] -impl SystemMeta { - #[inline] - pub fn set_warn_policy(&mut self, _warn_policy: ParamWarnPolicy) {} - #[inline] - pub fn advance_param_warn_policy(&mut self) {} - #[inline] - pub fn try_warn_param(&self) {} -} - /// State machine for emitting warnings when [system params are invalid](System::validate_param). #[derive(Clone, Copy)] pub enum ParamWarnPolicy { @@ -213,7 +199,7 @@ where { fn with_param_warn_policy(self, param_warn_policy: ParamWarnPolicy) -> FunctionSystem { let mut system = IntoSystem::into_system(self); - system.system_meta.param_warn_policy = param_warn_policy; + system.system_meta.set_param_warn_policy(param_warn_policy); system } } diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index cc67739c43a6e..f37b018d981d1 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -22,13 +22,8 @@ fn main() { // 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 and performance drop due to additional checks. - // - // There are two ways of disabling warnings: - // - During development, it's preferential to disable warnings selectively per system - // using the `.never_param_warn()` method. - // - When releasing the game, it's best to disable all warnings by removing the - // `bevy_param_warn` feature flag, which is enabled by default in `bevy_ecs`. + // lead to spam. You can disable invalid param warnings + // per system using the `.never_param_warn()` method. .add_systems( Update, ( From d6338beba6784dda4ee8a8c810ab9664d40a05a7 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 20:05:53 +0200 Subject: [PATCH 08/11] pr suggestions --- crates/bevy_ecs/src/system/function_system.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 15490043d5ac5..1941d5d014a09 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -115,18 +115,16 @@ impl SystemMeta { pub fn set_has_deferred(&mut self) { self.has_deferred = true; } -} -impl SystemMeta { /// Changes the warn policy. #[inline] - pub fn set_param_warn_policy(&mut self, warn_policy: ParamWarnPolicy) { + pub(crate) fn set_param_warn_policy(&mut self, warn_policy: ParamWarnPolicy) { self.param_warn_policy = warn_policy; } /// Advances the warn policy after validation failed. #[inline] - pub fn advance_param_warn_policy(&mut self) { + pub(crate) fn advance_param_warn_policy(&mut self) { self.param_warn_policy.advance(); } @@ -167,10 +165,11 @@ impl ParamWarnPolicy { if matches!(self, Self::Never) { return; } + bevy_utils::tracing::warn!( "System {0} will not run because it requested inaccessible system parameter {1}", name, - core::any::type_name::

() + disqualified::ShortName::of::

() ); } } From 6ed333f9c24923b0f39010b8f05da83910b7d3c2 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 20:45:43 +0200 Subject: [PATCH 09/11] post merge fix --- crates/bevy_ecs/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 146ea580bfb51..f27d42ecb38de 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -58,8 +58,8 @@ pub mod prelude { }, system::{ Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local, - NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut, - Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder, + NonSend, NonSendMut, ParallelCommands, ParamSet, Populated, Query, ReadOnlySystem, Res, + ResMut, Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder, SystemParamFunction, WithParamWarnPolicy, }, world::{ From caaa623a4bf436e87fd6baceaad4af2962222a4f Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 03:27:11 +0200 Subject: [PATCH 10/11] Update crates/bevy_ecs/src/system/function_system.rs Co-authored-by: SpecificProtagonist --- crates/bevy_ecs/src/system/function_system.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1941d5d014a09..2a757df629a22 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -151,9 +151,7 @@ impl ParamWarnPolicy { /// Advances the warn policy after validation failed. #[inline] fn advance(&mut self) { - *self = match self { - Self::Never | Self::Once => Self::Never, - }; + *self = Self::Never; } /// Emits a warning about inaccessible system param if policy allows it. From 98dc20f256fa938a30be2fbe11b3313e2530012d Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 15:04:23 +0200 Subject: [PATCH 11/11] better warning phrasing --- crates/bevy_ecs/src/system/function_system.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 2a757df629a22..fcd6a829e8afd 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -165,7 +165,7 @@ impl ParamWarnPolicy { } bevy_utils::tracing::warn!( - "System {0} will not run because it requested inaccessible system parameter {1}", + "{0} did not run because it requested inaccessible system parameter {1}", name, disqualified::ShortName::of::

() );