From 347eafa6734eeb88f7e394359bc6f54d4430cc94 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 01:04:21 +0200 Subject: [PATCH 01/19] impl solution 1.1 --- crates/bevy_ecs/macros/src/lib.rs | 44 +- crates/bevy_ecs/src/change_detection.rs | 4 +- crates/bevy_ecs/src/event/mod.rs | 16 +- crates/bevy_ecs/src/observer/runner.rs | 3 +- crates/bevy_ecs/src/query/mod.rs | 2 +- .../bevy_ecs/src/reflect/entity_commands.rs | 16 +- crates/bevy_ecs/src/removal_detection.rs | 5 +- crates/bevy_ecs/src/schedule/condition.rs | 55 +- crates/bevy_ecs/src/schedule/executor/mod.rs | 16 +- .../src/schedule/executor/multi_threaded.rs | 29 +- .../bevy_ecs/src/schedule/executor/simple.rs | 18 +- .../src/schedule/executor/single_threaded.rs | 18 +- crates/bevy_ecs/src/schedule/mod.rs | 2 +- crates/bevy_ecs/src/system/adapter_system.rs | 23 +- crates/bevy_ecs/src/system/combinator.rs | 31 +- crates/bevy_ecs/src/system/commands/mod.rs | 22 +- .../src/system/exclusive_function_system.rs | 11 +- crates/bevy_ecs/src/system/function_system.rs | 50 +- crates/bevy_ecs/src/system/mod.rs | 56 +- crates/bevy_ecs/src/system/system.rs | 59 +-- crates/bevy_ecs/src/system/system_name.rs | 5 +- crates/bevy_ecs/src/system/system_param.rs | 499 ++++++------------ crates/bevy_ecs/src/system/system_registry.rs | 8 +- crates/bevy_ecs/src/world/identifier.rs | 5 +- crates/bevy_gizmos/src/gizmos.rs | 18 +- crates/bevy_hierarchy/src/query_extension.rs | 4 +- .../bevy_pbr/src/meshlet/instance_manager.rs | 2 +- crates/bevy_pbr/src/render/mesh.rs | 2 +- crates/bevy_render/src/extract_param.rs | 30 +- crates/bevy_render/src/lib.rs | 7 +- crates/bevy_render/src/render_asset.rs | 2 +- crates/bevy_render/src/render_phase/draw.rs | 2 +- crates/bevy_render/src/renderer/mod.rs | 2 +- .../bevy_render/src/view/window/screenshot.rs | 3 +- crates/bevy_render/src/world_sync.rs | 2 +- crates/bevy_sprite/src/mesh2d/mesh.rs | 2 +- crates/bevy_sprite/src/render/mod.rs | 2 +- crates/bevy_transform/src/helper.rs | 2 +- crates/bevy_ui/src/ghost_hierarchy.rs | 4 +- crates/bevy_winit/src/state.rs | 20 +- examples/async_tasks/async_compute.rs | 2 +- 41 files changed, 389 insertions(+), 714 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index c735bfea59226..5053057516927 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -316,9 +316,11 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { // SAFETY: systems run without conflicts with other systems. // Conflicting params in ParamSet are not accessible at the same time // ParamSets are guaranteed to not conflict with other SystemParams - unsafe { + let maybe_param = unsafe { #param::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick) - } + }; + // `ParamSet::get_param` ensures all sub-params are accessible. + maybe_param.unwrap() } }); } @@ -376,28 +378,22 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { <(#(#param,)*) as SystemParam>::apply(state, system_meta, world); } - #[inline] - unsafe fn validate_param<'w, 's>( - state: &'s Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - ) -> bool { - <(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world) - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - ParamSet { + ) -> Option> { + // TODO: Reduce `get_param` used for validation. + <(#(#param,)*) as SystemParam>::get_param(state, system_meta, world, change_tick)?; + let param = ParamSet { param_states: state, system_meta: system_meta.clone(), world, change_tick, - } + }; + Some(param) } } @@ -626,28 +622,16 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::queue(&mut state.state, system_meta, world); } - #[inline] - unsafe fn validate_param<'w, 's>( - state: &'s Self::State, - system_meta: &#path::system::SystemMeta, - world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, - ) -> bool { - <(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world) - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &#path::system::SystemMeta, world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, change_tick: #path::component::Tick, - ) -> Self::Item<'w, 's> { - let (#(#tuple_patterns,)*) = < - (#(#tuple_types,)*) as #path::system::SystemParam - >::get_param(&mut state.state, system_meta, world, change_tick); - #struct_name { - #(#fields: #field_locals,)* - } + ) -> Option> { + let (#(#tuple_patterns,)*) = <(#(#tuple_types,)*) as #path::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick)?; + let param = #struct_name { #(#fields: #field_locals,)* }; + Some(param) } } diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 5a3adac96fe20..64df4616e4178 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -1256,7 +1256,7 @@ mod tests { // world: 1, system last ran: 0, component changed: 1 // The spawn will be detected since it happened after the system "last ran". - assert!(change_detected_system.run((), &mut world)); + assert!(change_detected_system.run((), &mut world).unwrap()); // world: 1 + MAX_CHANGE_AGE let change_tick = world.change_tick.get_mut(); @@ -1266,7 +1266,7 @@ mod tests { // Since we clamp things to `MAX_CHANGE_AGE` for determinism, // `ComponentTicks::is_changed` will now see `MAX_CHANGE_AGE > MAX_CHANGE_AGE` // and return `false`. - assert!(!change_expired_system.run((), &mut world)); + assert!(!change_expired_system.run((), &mut world).unwrap()); } #[test] diff --git a/crates/bevy_ecs/src/event/mod.rs b/crates/bevy_ecs/src/event/mod.rs index f7f46af258a3c..c2920b2dbb8d0 100644 --- a/crates/bevy_ecs/src/event/mod.rs +++ b/crates/bevy_ecs/src/event/mod.rs @@ -522,20 +522,20 @@ mod tests { }); reader.initialize(&mut world); - let last = reader.run((), &mut world); + let last = reader.run((), &mut world).unwrap(); assert!(last.is_none(), "EventReader should be empty"); world.send_event(TestEvent { i: 0 }); - let last = reader.run((), &mut world); + let last = reader.run((), &mut world).unwrap(); assert_eq!(last, Some(TestEvent { i: 0 })); world.send_event(TestEvent { i: 1 }); world.send_event(TestEvent { i: 2 }); world.send_event(TestEvent { i: 3 }); - let last = reader.run((), &mut world); + let last = reader.run((), &mut world).unwrap(); assert_eq!(last, Some(TestEvent { i: 3 })); - let last = reader.run((), &mut world); + let last = reader.run((), &mut world).unwrap(); assert!(last.is_none(), "EventReader should be empty"); } @@ -552,20 +552,20 @@ mod tests { }); mutator.initialize(&mut world); - let last = mutator.run((), &mut world); + let last = mutator.run((), &mut world).unwrap(); assert!(last.is_none(), "EventMutator should be empty"); world.send_event(TestEvent { i: 0 }); - let last = mutator.run((), &mut world); + let last = mutator.run((), &mut world).unwrap(); assert_eq!(last, Some(TestEvent { i: 0 })); world.send_event(TestEvent { i: 1 }); world.send_event(TestEvent { i: 2 }); world.send_event(TestEvent { i: 3 }); - let last = mutator.run((), &mut world); + let last = mutator.run((), &mut world).unwrap(); assert_eq!(last, Some(TestEvent { i: 3 })); - let last = mutator.run((), &mut world); + let last = mutator.run((), &mut world).unwrap(); assert!(last.is_none(), "EventMutator should be empty"); } diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index f85725d392525..0241f472d9f3e 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -374,8 +374,7 @@ fn observer_system_runner>( // - system is the same type erased system from above unsafe { (*system).update_archetype_component_access(world); - if (*system).validate_param_unsafe(world) { - (*system).run_unsafe(trigger, world); + if (*system).run_unsafe(trigger, world).is_some() { (*system).queue_deferred(world.into_deferred()); } } diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 0a9f664fb50ba..bddd9a498b82e 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -751,7 +751,7 @@ mod tests { // system param let mut q = SystemState::>::new(&mut world); - let q = q.get_mut(&mut world); + let q = q.get_mut(&mut world).unwrap(); let _: Option<&Foo> = q.iter().next(); let _: Option<[&Foo; 2]> = q.iter_combinations::<2>().next(); let _: Option<&Foo> = q.iter_many([e]).next(); diff --git a/crates/bevy_ecs/src/reflect/entity_commands.rs b/crates/bevy_ecs/src/reflect/entity_commands.rs index 3979eee22903f..6c787fb7172cf 100644 --- a/crates/bevy_ecs/src/reflect/entity_commands.rs +++ b/crates/bevy_ecs/src/reflect/entity_commands.rs @@ -398,7 +398,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let entity2 = commands.spawn_empty().id(); @@ -435,7 +435,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); @@ -465,7 +465,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn(ComponentA(0)).id(); @@ -494,7 +494,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn(ComponentA(0)).id(); @@ -523,7 +523,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let bundle = Box::new(BundleA { @@ -553,7 +553,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let bundle = Box::new(BundleA { @@ -583,7 +583,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands .spawn(BundleA { @@ -621,7 +621,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands .spawn(BundleA { diff --git a/crates/bevy_ecs/src/removal_detection.rs b/crates/bevy_ecs/src/removal_detection.rs index e4ecaad7431a8..a4b6a1477eabf 100644 --- a/crates/bevy_ecs/src/removal_detection.rs +++ b/crates/bevy_ecs/src/removal_detection.rs @@ -264,7 +264,8 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.removed_components() + ) -> Option> { + let param = world.removed_components(); + Some(param) } } diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 6956d2715069f..22f931c7dedac 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -1199,9 +1199,10 @@ impl> Adapt for NotMarker { fn adapt( &mut self, input: ::Inner<'_>, - run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out, - ) -> Self::Out { - !run_system(input) + run_system: impl FnOnce(SystemIn<'_, S>) -> Option, + ) -> Option { + let out = run_system(input)?; + Some(!out) } } @@ -1237,10 +1238,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, A>) -> B::Out, - ) -> Self::Out { - a(input) && b(input) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, A>) -> Option, + ) -> Option { + Some(a(input)? && b(input)?) } } @@ -1258,10 +1259,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out { - !(a(input) && b(input)) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option { + Some(!(a(input)? && b(input)?)) } } @@ -1279,10 +1280,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out { - !(a(input) || b(input)) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option { + Some(!(a(input)? || b(input)?)) } } @@ -1300,10 +1301,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out { - a(input) || b(input) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option { + Some(a(input)? || b(input)?) } } @@ -1321,10 +1322,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out { - !(a(input) ^ b(input)) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option { + Some(!(a(input)? ^ b(input)?)) } } @@ -1342,10 +1343,10 @@ where fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out { - a(input) ^ b(input) + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option { + Some(a(input)? ^ b(input)?) } } diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 24113aee2bee0..8330b4af2a4b8 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -161,7 +161,7 @@ mod __rust_begin_short_backtrace { pub(super) unsafe fn readonly_run_unsafe( system: &mut dyn ReadOnlySystem, world: UnsafeWorldCell, - ) -> O { + ) -> Option { black_box(system.run_unsafe((), world)) } @@ -175,23 +175,11 @@ mod __rust_begin_short_backtrace { pub(super) fn readonly_run( system: &mut dyn ReadOnlySystem, world: &mut World, - ) -> O { + ) -> Option { black_box(system.run((), world)) } } -#[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..9fc2cf6140ee5 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: &BoxedSystem, conditions: &mut Conditions, world: UnsafeWorldCell, ) -> bool { @@ -568,19 +567,6 @@ impl ExecutorState { should_run &= system_conditions_met; - if should_run { - // SAFETY: - // - The caller ensures that `world` has permission to read any data - // required by the system. - // - `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; - } - should_run } @@ -750,15 +736,10 @@ unsafe fn evaluate_and_fold_conditions( // - The caller ensures that `world` has permission to read any data // 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: - // - The caller ensures that `world` has permission to read any data - // required by the condition. - // - `update_archetype_component_access` has been called for condition. - unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) } + let result = unsafe { + __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) + }; + result.unwrap_or(false) }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 171125342cd78..25992c0f867ef 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, }; @@ -80,15 +79,6 @@ impl SystemExecutor for SimpleExecutor { should_run &= system_conditions_met; - 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; - } - #[cfg(feature = "trace")] should_run_span.exit(); @@ -99,6 +89,7 @@ impl SystemExecutor for SimpleExecutor { continue; } + let system = &mut schedule.systems[system_index]; if is_apply_deferred(system) { continue; } @@ -138,11 +129,8 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .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) + let result = __rust_begin_short_backtrace::readonly_run(&mut **condition, world); + result.unwrap_or(false) }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs index 93d814d3b2f83..c31b351d22a50 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, }; @@ -86,15 +85,6 @@ impl SystemExecutor for SingleThreadedExecutor { should_run &= system_conditions_met; - 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; - } - #[cfg(feature = "trace")] should_run_span.exit(); @@ -105,6 +95,7 @@ impl SystemExecutor for SingleThreadedExecutor { continue; } + let system = &mut schedule.systems[system_index]; if is_apply_deferred(system) { self.apply_deferred(schedule, world); continue; @@ -170,11 +161,8 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .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) + let result = __rust_begin_short_backtrace::readonly_run(&mut **condition, world); + result.unwrap_or(false) }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index 73689d57ce57b..c1502e31602dc 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/crates/bevy_ecs/src/schedule/mod.rs @@ -1198,7 +1198,7 @@ mod tests { // start a new frame by running ihe begin_frame() system let mut system_state: SystemState>> = SystemState::new(&mut world); - let res = system_state.get_mut(&mut world); + let res = system_state.get_mut(&mut world).unwrap(); Stepping::begin_frame(res); // now run the schedule; this will panic if the executor doesn't diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 8c25e57bbd91e..df44852e418fe 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -58,8 +58,8 @@ pub trait Adapt: Send + Sync + 'static { fn adapt( &mut self, input: ::Inner<'_>, - run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out, - ) -> Self::Out; + run_system: impl FnOnce(SystemIn<'_, S>) -> Option, + ) -> Option; } /// An [`IntoSystem`] creating an instance of [`AdapterSystem`]. @@ -155,7 +155,7 @@ where &mut self, input: SystemIn<'_, Self>, world: UnsafeWorldCell, - ) -> Self::Out { + ) -> Option { // SAFETY: `system.run_unsafe` has the same invariants as `self.run_unsafe`. self.func.adapt(input, |input| unsafe { self.system.run_unsafe(input, world) @@ -163,7 +163,11 @@ where } #[inline] - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut crate::prelude::World) -> Self::Out { + fn run( + &mut self, + input: SystemIn<'_, Self>, + world: &mut crate::prelude::World, + ) -> Option { self.func .adapt(input, |input| self.system.run(input, world)) } @@ -178,11 +182,6 @@ where self.system.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { - self.system.validate_param_unsafe(world) - } - fn initialize(&mut self, world: &mut crate::prelude::World) { self.system.initialize(world); } @@ -228,8 +227,8 @@ where fn adapt( &mut self, input: ::Inner<'_>, - run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out, - ) -> Out { - self(run_system(input)) + run_system: impl FnOnce(SystemIn<'_, S>) -> Option, + ) -> Option { + Some(self(run_system(input)?)) } } diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index ce4c8785feb05..a148409857722 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -101,9 +101,9 @@ pub trait Combine { /// See the trait-level docs for [`Combine`] for an example implementation. fn combine( input: ::Inner<'_>, - a: impl FnOnce(SystemIn<'_, A>) -> A::Out, - b: impl FnOnce(SystemIn<'_, B>) -> B::Out, - ) -> Self::Out; + a: impl FnOnce(SystemIn<'_, A>) -> Option, + b: impl FnOnce(SystemIn<'_, B>) -> Option, + ) -> Option; } /// A [`System`] defined by combining two other systems. @@ -171,7 +171,7 @@ where &mut self, input: SystemIn<'_, Self>, world: UnsafeWorldCell, - ) -> Self::Out { + ) -> Option { Func::combine( input, // SAFETY: The world accesses for both underlying systems have been registered, @@ -186,7 +186,7 @@ where ) } - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { let world = world.as_unsafe_world_cell(); Func::combine( input, @@ -211,11 +211,6 @@ where self.b.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { - self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) - } - fn initialize(&mut self, world: &mut World) { self.a.initialize(world); self.b.initialize(world); @@ -410,13 +405,13 @@ where &mut self, input: SystemIn<'_, Self>, world: UnsafeWorldCell, - ) -> Self::Out { - let value = self.a.run_unsafe(input, world); + ) -> Option { + let value = self.a.run_unsafe(input, world)?; self.b.run_unsafe(value, world) } - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - let value = self.a.run(input, world); + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { + let value = self.a.run(input, world)?; self.b.run(value, world) } @@ -430,14 +425,6 @@ where self.b.queue_deferred(world); } - unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { - self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) - } - - fn validate_param(&mut self, world: &World) -> bool { - self.a.validate_param(world) && self.b.validate_param(world) - } - fn initialize(&mut self, world: &mut World) { self.a.initialize(world); self.b.initialize(world); diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 4f941c07a6721..5af0784082f1d 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -148,31 +148,19 @@ const _: () = { ); } - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &bevy_ecs::system::SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - <(Deferred, &Entities) as bevy_ecs::system::SystemParam>::validate_param( - &state.state, - system_meta, - world, - ) - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &bevy_ecs::system::SystemMeta, world: UnsafeWorldCell<'w>, change_tick: bevy_ecs::component::Tick, - ) -> Self::Item<'w, 's> { - let(f0, f1) = <(Deferred<'s, CommandQueue>, &'w Entities) as bevy_ecs::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick); - Commands { + ) -> Option> { + let(f0, f1) = <(Deferred<'s, CommandQueue>, &'w Entities) as bevy_ecs::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick)?; + let param = Commands { queue: InternalQueue::CommandQueue(f0), entities: f1, - } + }; + Some(param) } } // SAFETY: Only reads Entities diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 3075f3c55772e..432991e269248 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -113,11 +113,11 @@ where &mut self, _input: SystemIn<'_, Self>, _world: UnsafeWorldCell, - ) -> Self::Out { + ) -> Option { panic!("Cannot run exclusive systems with a shared World reference"); } - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { world.last_change_tick_scope(self.system_meta.last_run, |world| { #[cfg(feature = "trace")] let _span_guard = self.system_meta.system_span.enter(); @@ -131,7 +131,7 @@ where world.flush(); self.system_meta.last_run = world.increment_change_tick(); - out + Some(out) }) } @@ -149,11 +149,6 @@ where // might have buffers to apply, but this is handled by `PipeSystem`. } - #[inline] - unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool { - true - } - #[inline] fn initialize(&mut self, world: &mut World) { self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 0614d7339ca4b..839c38b596136 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -316,7 +316,7 @@ impl SystemState { /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only. #[inline] - pub fn get<'w, 's>(&'s mut self, world: &'w World) -> SystemParamItem<'w, 's, Param> + pub fn get<'w, 's>(&'s mut self, world: &'w World) -> Option> where Param: ReadOnlySystemParam, { @@ -329,7 +329,10 @@ impl SystemState { /// Retrieve the mutable [`SystemParam`] values. #[inline] - pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { + pub fn get_mut<'w, 's>( + &'s mut self, + world: &'w mut World, + ) -> Option> { self.validate_world(world.id()); self.update_archetypes(world); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. @@ -344,18 +347,6 @@ impl SystemState { Param::apply(&mut self.param_state, &self.meta, world); } - /// Wrapper over [`SystemParam::validate_param`]. - /// - /// # Safety - /// - /// - The passed [`UnsafeWorldCell`] must have read-only access to - /// world data in `archetype_component_access`. - /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). - pub unsafe fn validate_param(state: &Self, world: UnsafeWorldCell) -> bool { - // SAFETY: Delegated to existing `SystemParam` implementations. - unsafe { Param::validate_param(&state.param_state, &state.meta, world) } - } - /// Returns `true` if `world_id` matches the [`World`] that was used to call [`SystemState::new`]. /// Otherwise, this returns false. #[inline] @@ -423,7 +414,10 @@ impl SystemState { /// /// Users should strongly prefer to use [`SystemState::get`] over this function. #[inline] - pub fn get_manual<'w, 's>(&'s mut self, world: &'w World) -> SystemParamItem<'w, 's, Param> + pub fn get_manual<'w, 's>( + &'s mut self, + world: &'w World, + ) -> Option> where Param: ReadOnlySystemParam, { @@ -445,7 +439,7 @@ impl SystemState { pub fn get_manual_mut<'w, 's>( &'s mut self, world: &'w mut World, - ) -> SystemParamItem<'w, 's, Param> { + ) -> Option> { self.validate_world(world.id()); let change_tick = world.change_tick(); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. @@ -462,7 +456,7 @@ impl SystemState { pub unsafe fn get_unchecked_manual<'w, 's>( &'s mut self, world: UnsafeWorldCell<'w>, - ) -> SystemParamItem<'w, 's, Param> { + ) -> Option> { let change_tick = world.increment_change_tick(); // SAFETY: The invariants are uphold by the caller. unsafe { self.fetch(world, change_tick) } @@ -477,12 +471,12 @@ impl SystemState { &'s mut self, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> SystemParamItem<'w, 's, Param> { + ) -> Option> { // SAFETY: The invariants are uphold by the caller. - let param = + let maybe_param = unsafe { Param::get_param(&mut self.param_state, &self.meta, world, change_tick) }; self.meta.last_run = change_tick; - param + maybe_param } } @@ -620,7 +614,7 @@ where &mut self, input: SystemIn<'_, Self>, world: UnsafeWorldCell, - ) -> Self::Out { + ) -> Option { #[cfg(feature = "trace")] let _span_guard = self.system_meta.system_span.enter(); @@ -631,17 +625,17 @@ where // if the world does not match. // - All world accesses used by `F::Param` have been registered, so the caller // will ensure that there are no data access conflicts. - let params = unsafe { + let param: <>::Param as SystemParam>::Item<'_, '_> = unsafe { F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, world, change_tick, - ) + )? }; - let out = self.func.run(input, params); + let out = self.func.run(input, param); self.system_meta.last_run = change_tick; - out + Some(out) } #[inline] @@ -656,12 +650,6 @@ where F::Param::queue(param_state, &self.system_meta, world); } - #[inline] - unsafe fn validate_param_unsafe(&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) - } - #[inline] fn initialize(&mut self, world: &mut World) { if let Some(id) = self.world_id { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index acf28fac37905..699f3b5504e2d 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1157,7 +1157,7 @@ mod tests { let mut system_state: SystemState<(Res, Query<&B>, ParamSet<(Query<&C>, Query<&D>)>)> = SystemState::new(&mut world); - let (a, query, _) = system_state.get(&world); + let (a, query, _) = system_state.get(&world).unwrap(); assert_eq!(*a, A(42), "returned resource matches initial value"); assert_eq!( *query.single(), @@ -1184,7 +1184,7 @@ mod tests { // The following line shouldn't compile because the parameters used are not ReadOnlySystemParam // let (a, query) = system_state.get(&world); - let (a, mut query) = system_state.get_mut(&mut world); + let (a, mut query) = system_state.get_mut(&mut world).unwrap(); assert_eq!(*a, A(42), "returned resource matches initial value"); assert_eq!( *query.single_mut(), @@ -1203,18 +1203,18 @@ mod tests { let mut system_state: SystemState>> = SystemState::new(&mut world); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!(*query.single(), A(1)); } { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert!(query.get_single().is_err()); } world.entity_mut(entity).get_mut::().unwrap().0 = 2; { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!(*query.single(), A(2)); } } @@ -1241,7 +1241,7 @@ mod tests { let mut system_state = SystemState::>::new(&mut world); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!( query.iter().collect::>(), vec![&A(1)], @@ -1251,7 +1251,7 @@ mod tests { world.spawn((A(2), B(2))); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!( query.iter().collect::>(), vec![&A(1), &A(2)], @@ -1275,19 +1275,19 @@ mod tests { impl State { fn hold_res<'w>(&mut self, world: &'w World) -> Holder<'w> { - let a = self.state.get(world); + let a = self.state.get(world).unwrap(); Holder { value: a.into_inner(), } } fn hold_component<'w>(&mut self, world: &'w World, entity: Entity) -> Holder<'w> { - let q = self.state_q.get(world); + let q = self.state_q.get(world).unwrap(); let a = q.get_inner(entity).unwrap(); Holder { value: a } } fn hold_components<'w>(&mut self, world: &'w World) -> Vec> { let mut components = Vec::new(); - let q = self.state_q.get(world); + let q = self.state_q.get(world).unwrap(); for a in q.iter_inner() { components.push(Holder { value: a }); } @@ -1307,7 +1307,7 @@ mod tests { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); assert_eq!( query.iter_mut().map(|m| *m).collect::>(), vec![A(1), A(2)], @@ -1689,26 +1689,30 @@ mod tests { sys.run(default(), &mut world); // The second system should observe a change made in the first system. - let info = sys.run( - Info { - do_first: true, - ..default() - }, - &mut world, - ); + let info = sys + .run( + Info { + do_first: true, + ..default() + }, + &mut world, + ) + .unwrap(); assert!(!info.first_flag); assert!(info.second_flag); // When a change is made in the second system, the first system // should observe it the next time they are run. - let info1 = sys.run( - Info { - do_second: true, - ..default() - }, - &mut world, - ); - let info2 = sys.run(default(), &mut world); + let info1 = sys + .run( + Info { + do_second: true, + ..default() + }, + &mut world, + ) + .unwrap(); + let info2 = sys.run(default(), &mut world).unwrap(); assert!(!info1.first_flag); assert!(!info1.second_flag); assert!(info2.first_flag); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 60d3c58f62d47..b1756d5aa16e9 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -68,8 +68,11 @@ 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 run_unsafe(&mut self, input: SystemIn<'_, Self>, world: UnsafeWorldCell) - -> Self::Out; + unsafe fn run_unsafe( + &mut self, + input: SystemIn<'_, Self>, + world: UnsafeWorldCell, + ) -> Option; /// Runs the system with the given input in the world. /// @@ -78,15 +81,17 @@ pub trait System: Send + Sync + 'static { /// Unlike [`System::run_unsafe`], this will apply deferred parameters *immediately*. /// /// [`run_readonly`]: ReadOnlySystem::run_readonly - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { let world_cell = world.as_unsafe_world_cell(); self.update_archetype_component_access(world_cell); // SAFETY: // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. - let ret = unsafe { self.run_unsafe(input, world_cell) }; - self.apply_deferred(world); - ret + let result = unsafe { self.run_unsafe(input, world_cell) }; + if result.is_some() { + self.apply_deferred(world); + } + result } /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. @@ -98,38 +103,6 @@ pub trait System: Send + Sync + 'static { /// of this system into the world's command buffer. fn queue_deferred(&mut self, world: DeferredWorld); - /// Validates that all parameters can be acquired and that system can run without panic. - /// Built-in executors use this to prevent invalid systems from running. - /// - /// However calling and respecting [`System::validate_param_unsafe`] or it's safe variant - /// is not a strict requirement, both [`System::run`] and [`System::run_unsafe`] - /// should provide their own safety mechanism to prevent undefined behavior. - /// - /// This method has to be called directly before [`System::run_unsafe`] with no other (relevant) - /// world mutations in between. Otherwise, while it won't lead to any undefined behavior, - /// the validity of the param may change. - /// - /// # Safety - /// - /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data - /// registered in `archetype_component_access`. There must be no conflicting - /// simultaneous accesses while the system is running. - /// - 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; - - /// Safe version of [`System::validate_param_unsafe`]. - /// that runs on exclusive, single-threaded `world` pointer. - fn validate_param(&mut self, world: &World) -> bool { - let world_cell = world.as_unsafe_world_cell_readonly(); - self.update_archetype_component_access(world_cell); - // SAFETY: - // - We have exclusive access to the entire world. - // - `update_archetype_component_access` has been called. - unsafe { self.validate_param_unsafe(world_cell) } - } - /// Initialize the system. fn initialize(&mut self, _world: &mut World); @@ -183,7 +156,7 @@ pub unsafe trait ReadOnlySystem: System { /// /// Unlike [`System::run`], this can be called with a shared reference to the world, /// since this system is known not to modify the world. - fn run_readonly(&mut self, input: SystemIn<'_, Self>, world: &World) -> Self::Out { + fn run_readonly(&mut self, input: SystemIn<'_, Self>, world: &World) -> Option { let world = world.as_unsafe_world_cell_readonly(); self.update_archetype_component_access(world); // SAFETY: @@ -348,11 +321,9 @@ impl RunSystemOnce for &mut World { { let mut system: T::System = IntoSystem::into_system(system); system.initialize(self); - if system.validate_param(self) { - Ok(system.run(input, self)) - } else { - Err(RunSystemError::InvalidParams(system.name())) - } + system + .run(input, self) + .ok_or(RunSystemError::InvalidParams(system.name())) } } diff --git a/crates/bevy_ecs/src/system/system_name.rs b/crates/bevy_ecs/src/system/system_name.rs index 5a3763465cd8a..1d2ca23accc95 100644 --- a/crates/bevy_ecs/src/system/system_name.rs +++ b/crates/bevy_ecs/src/system/system_name.rs @@ -84,8 +84,9 @@ unsafe impl SystemParam for SystemName<'_> { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - SystemName(name) + ) -> Option> { + let param = SystemName(name); + Some(param) } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 013d75265a900..ee167780b5723 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -9,7 +9,7 @@ use crate::{ Access, AccessConflicts, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter, QuerySingleError, QueryState, ReadOnlyQueryData, }, - storage::{ResourceData, SparseSetIndex}, + storage::SparseSetIndex, system::{Query, Single, SystemMeta}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World}, }; @@ -217,44 +217,8 @@ pub unsafe trait SystemParam: Sized { #[allow(unused_variables)] fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {} - /// Validates that the param can be acquired by the [`get_param`](SystemParam::get_param). - /// Built-in executors use this to prevent systems with invalid params from running. - /// For nested [`SystemParam`]s validation will fail if any - /// delegated validation fails. - /// - /// However calling and respecting [`SystemParam::validate_param`] - /// is not a strict requirement, [`SystemParam::get_param`] should - /// provide it's own safety mechanism to prevent undefined behavior. - /// - /// The [`world`](UnsafeWorldCell) can only be used to read param's data - /// and world metadata. No data can be written. - /// - /// When using system parameters that require `change_tick` you can use - /// [`UnsafeWorldCell::change_tick()`]. Even if this isn't the exact - /// same tick used for [`SystemParam::get_param`], the world access - /// ensures that the queried data will be the same in both calls. - /// - /// This method has to be called directly before [`SystemParam::get_param`] with no other (relevant) - /// world mutations inbetween. Otherwise, while it won't lead to any undefined behavior, - /// the validity of the param may change. - /// - /// # Safety - /// - /// - The passed [`UnsafeWorldCell`] must have read-only access to world data - /// registered in [`init_state`](SystemParam::init_state). - /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). - /// - All `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). - unsafe fn validate_param( - _state: &Self::State, - _system_meta: &SystemMeta, - _world: UnsafeWorldCell, - ) -> bool { - // By default we allow panics in [`SystemParam::get_param`] and return `true`. - // Preventing panics is an optional feature. - true - } - /// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction). + /// If fetching parameters fails, [`None`] is returned. /// /// # Safety /// @@ -267,7 +231,7 @@ pub unsafe trait SystemParam: Sized { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state>; + ) -> Option>; } /// A [`SystemParam`] that only reads a given [`World`]. @@ -311,11 +275,12 @@ unsafe impl SystemParam for Qu system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { // SAFETY: We have registered all of the query's world accesses, // so the caller ensures that `world` has permission to access any // world data that the query needs. - unsafe { Query::new(world, state, system_meta.last_run, change_tick) } + let param = unsafe { Query::new(world, state, system_meta.last_run, change_tick) }; + Some(param) } } @@ -392,36 +357,16 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not accessible somewhere elsewhere. let result = unsafe { state.get_single_unchecked_manual(world, system_meta.last_run, change_tick) }; - let single = - result.expect("The query was expected to contain exactly one matching entity."); - Single { + let single = result.ok()?; + Some(Single { item: single, _filter: PhantomData, - } - } - - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - state.validate_world(world.id()); - // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere - // and the query is read only. - let result = unsafe { - state.as_readonly().get_single_unchecked_manual( - world, - system_meta.last_run, - world.change_tick(), - ) - }; - result.is_ok() + }) } } @@ -452,39 +397,20 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not accessible elsewhere. let result = unsafe { state.get_single_unchecked_manual(world, system_meta.last_run, change_tick) }; match result { - Ok(single) => Some(Single { + Ok(single) => Some(Some(Single { item: single, _filter: PhantomData, - }), - Err(QuerySingleError::NoEntities(_)) => None, - Err(QuerySingleError::MultipleEntities(e)) => panic!("{}", e), + })), + Err(QuerySingleError::NoEntities(_)) => Some(None), + Err(QuerySingleError::MultipleEntities(_)) => None, } } - - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - state.validate_world(world.id()); - // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere - // and the query is read only. - let result = unsafe { - state.as_readonly().get_single_unchecked_manual( - world, - system_meta.last_run, - world.change_tick(), - ) - }; - !matches!(result, Err(QuerySingleError::MultipleEntities(_))) - } } // SAFETY: QueryState is constrained to read-only fetches, so it only reads World. @@ -526,24 +452,15 @@ unsafe impl SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - // SAFETY: Delegate to existing `SystemParam` implementations. - let query = unsafe { Query::get_param(state, system_meta, world, change_tick) }; - Populated(query) - } - - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - state.validate_world(world.id()); + ) -> Option> { // SAFETY: - // - We have read-only access to the components accessed by query. - // - The world has been validated. - !unsafe { - state.is_empty_unsafe_world_cell(world, system_meta.last_run, world.change_tick()) + // - Delegate to existing `SystemParam` implementations. + // - Query is always valid. + let query = unsafe { Query::get_param(state, system_meta, world, change_tick) }.unwrap(); + if query.is_empty() { + None + } else { + Some(Populated(query)) } } } @@ -770,37 +687,15 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { component_id } - #[inline] - unsafe fn validate_param( - &component_id: &Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } - .resources - .get(component_id) - .is_some_and(ResourceData::is_present) - } - #[inline] unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - let (ptr, ticks, _caller) = - world - .get_resource_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Resource requested by {} does not exist: {}", - system_meta.name, - core::any::type_name::() - ) - }); - Res { + ) -> Option> { + let (ptr, ticks, _caller) = world.get_resource_with_ticks(component_id)?; + Some(Res { value: ptr.deref(), ticks: Ticks { added: ticks.added.deref(), @@ -810,7 +705,7 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { }, #[cfg(feature = "track_change_detection")] changed_by: _caller.deref(), - } + }) } } @@ -832,8 +727,8 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - world + ) -> Option> { + let param = world .get_resource_with_ticks(component_id) .map(|(ptr, ticks, _caller)| Res { value: ptr.deref(), @@ -845,7 +740,8 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { }, #[cfg(feature = "track_change_detection")] changed_by: _caller.deref(), - }) + }); + Some(param) } } @@ -880,36 +776,15 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { component_id } - #[inline] - unsafe fn validate_param( - &component_id: &Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } - .resources - .get(component_id) - .is_some_and(ResourceData::is_present) - } - #[inline] unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - let value = world - .get_resource_mut_by_id(component_id) - .unwrap_or_else(|| { - panic!( - "Resource requested by {} does not exist: {}", - system_meta.name, - core::any::type_name::() - ) - }); - ResMut { + ) -> Option> { + let value = world.get_resource_mut_by_id(component_id)?; + Some(ResMut { value: value.value.deref_mut::(), ticks: TicksMut { added: value.ticks.added, @@ -919,7 +794,7 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { }, #[cfg(feature = "track_change_detection")] changed_by: value.changed_by, - } + }) } } @@ -938,8 +813,8 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - world + ) -> Option> { + let param = world .get_resource_mut_by_id(component_id) .map(|value| ResMut { value: value.value.deref_mut::(), @@ -951,7 +826,8 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { }, #[cfg(feature = "track_change_detection")] changed_by: value.changed_by, - }) + }); + Some(param) } } @@ -993,9 +869,10 @@ unsafe impl SystemParam for &'_ World { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { // SAFETY: Read-only access to the entire world was registered in `init_state`. - unsafe { world.world() } + let param = unsafe { world.world() }; + Some(param) } } @@ -1015,8 +892,9 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { _system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { - world.into_deferred() + ) -> Option> { + let param = world.into_deferred(); + Some(param) } } @@ -1128,8 +1006,9 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - Local(state.get()) + ) -> Option> { + let param = Local(state.get()); + Some(param) } } @@ -1318,8 +1197,9 @@ unsafe impl SystemParam for Deferred<'_, T> { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - Deferred(state.get()) + ) -> Option> { + let param = Deferred(state.get()); + Some(param) } } @@ -1426,45 +1306,23 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { component_id } - #[inline] - unsafe fn validate_param( - &component_id: &Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } - .non_send_resources - .get(component_id) - .is_some_and(ResourceData::is_present) - } - #[inline] unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - let (ptr, ticks, _caller) = - world - .get_non_send_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Non-send resource requested by {} does not exist: {}", - system_meta.name, - core::any::type_name::() - ) - }); - - NonSend { + ) -> Option> { + let (ptr, ticks, _caller) = world.get_non_send_with_ticks(component_id)?; + let param = NonSend { value: ptr.deref(), ticks: ticks.read(), last_run: system_meta.last_run, this_run: change_tick, #[cfg(feature = "track_change_detection")] changed_by: _caller.deref(), - } + }; + Some(param) } } @@ -1486,8 +1344,8 @@ unsafe impl SystemParam for Option> { system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - world + ) -> Option> { + let param = world .get_non_send_with_ticks(component_id) .map(|(ptr, ticks, _caller)| NonSend { value: ptr.deref(), @@ -1496,7 +1354,8 @@ unsafe impl SystemParam for Option> { this_run: change_tick, #[cfg(feature = "track_change_detection")] changed_by: _caller.deref(), - }) + }); + Some(param) } } @@ -1533,42 +1392,21 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { component_id } - #[inline] - unsafe fn validate_param( - &component_id: &Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } - .non_send_resources - .get(component_id) - .is_some_and(ResourceData::is_present) - } - #[inline] unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - let (ptr, ticks, _caller) = - world - .get_non_send_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Non-send resource requested by {} does not exist: {}", - system_meta.name, - core::any::type_name::() - ) - }); - NonSendMut { + ) -> Option> { + let (ptr, ticks, _caller) = world.get_non_send_with_ticks(component_id)?; + let param = NonSendMut { value: ptr.assert_unique().deref_mut(), ticks: TicksMut::from_tick_cells(ticks, system_meta.last_run, change_tick), #[cfg(feature = "track_change_detection")] changed_by: _caller.deref_mut(), - } + }; + Some(param) } } @@ -1587,15 +1425,16 @@ unsafe impl<'a, T: 'static> SystemParam for Option> { system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - world + ) -> Option> { + let param = world .get_non_send_with_ticks(component_id) .map(|(ptr, ticks, _caller)| NonSendMut { value: ptr.assert_unique().deref_mut(), ticks: TicksMut::from_tick_cells(ticks, system_meta.last_run, change_tick), #[cfg(feature = "track_change_detection")] changed_by: _caller.deref_mut(), - }) + }); + Some(param) } } @@ -1615,8 +1454,9 @@ unsafe impl<'a> SystemParam for &'a Archetypes { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.archetypes() + ) -> Option> { + let param = world.archetypes(); + Some(param) } } @@ -1636,8 +1476,9 @@ unsafe impl<'a> SystemParam for &'a Components { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.components() + ) -> Option> { + let param = world.components(); + Some(param) } } @@ -1657,8 +1498,9 @@ unsafe impl<'a> SystemParam for &'a Entities { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.entities() + ) -> Option> { + let param = world.entities(); + Some(param) } } @@ -1678,8 +1520,9 @@ unsafe impl<'a> SystemParam for &'a Bundles { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.bundles() + ) -> Option> { + let param = world.bundles(); + Some(param) } } @@ -1728,11 +1571,12 @@ unsafe impl SystemParam for SystemChangeTick { system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - SystemChangeTick { + ) -> Option> { + let param = SystemChangeTick { last_run: system_meta.last_run, this_run: change_tick, - } + }; + Some(param) } } @@ -1748,24 +1592,13 @@ unsafe impl SystemParam for Vec { Vec::new() } - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - state - .iter() - .all(|state| T::validate_param(state, system_meta, world)) - } - #[inline] unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Option> { state .iter_mut() // SAFETY: @@ -1817,13 +1650,18 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { - ParamSet { + ) -> Option> { + // TODO: Reduce `get_param` used for validation. + for state in state.iter_mut() { + T::get_param(state, system_meta, world, change_tick)?; + } + let param = ParamSet { param_states: state, system_meta: system_meta.clone(), world, change_tick, - } + }; + Some(param) } unsafe fn new_archetype( @@ -1858,74 +1696,68 @@ impl ParamSet<'_, '_, Vec> { // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. // We have mutable access to the ParamSet, so no other params in the set are active. // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states - unsafe { + let maybe_param = unsafe { T::get_param( &mut self.param_states[index], &self.system_meta, self.world, self.change_tick, ) - } + }; + // `ParamSet::get_param` ensures all sub-params are accessible. + maybe_param.unwrap() } /// Calls a closure for each parameter in the set. pub fn for_each(&mut self, mut f: impl FnMut(T::Item<'_, '_>)) { self.param_states.iter_mut().for_each(|state| { - f( - // SAFETY: - // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. - // We have mutable access to the ParamSet, so no other params in the set are active. - // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states - unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) }, - ); + // SAFETY: + // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. + // We have mutable access to the ParamSet, so no other params in the set are active. + // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states + let maybe_param = + unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) }; + // `ParamSet::get_param` ensures all sub-params are accessible. + let param = maybe_param.unwrap(); + f(param); }); } } macro_rules! impl_system_param_tuple { - ($(#[$meta:meta])* $($param: ident),*) => { + ($(#[$meta:meta])* $($subparam: ident),*) => { $(#[$meta])* // SAFETY: tuple consists only of ReadOnlySystemParams - unsafe impl<$($param: ReadOnlySystemParam),*> ReadOnlySystemParam for ($($param,)*) {} + unsafe impl<$($subparam: ReadOnlySystemParam),*> ReadOnlySystemParam for ($($subparam,)*) {} // SAFETY: implementors of each `SystemParam` in the tuple have validated their impls #[allow(clippy::undocumented_unsafe_blocks)] // false positive by clippy #[allow(non_snake_case)] $(#[$meta])* - unsafe impl<$($param: SystemParam),*> SystemParam for ($($param,)*) { - type State = ($($param::State,)*); - type Item<'w, 's> = ($($param::Item::<'w, 's>,)*); + unsafe impl<$($subparam: SystemParam),*> SystemParam for ($($subparam,)*) { + type State = ($($subparam::State,)*); + type Item<'w, 's> = ($($subparam::Item::<'w, 's>,)*); #[inline] fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - (($($param::init_state(_world, _system_meta),)*)) + (($($subparam::init_state(_world, _system_meta),)*)) } #[inline] #[allow(unused_unsafe)] - unsafe fn new_archetype(($($param,)*): &mut Self::State, _archetype: &Archetype, _system_meta: &mut SystemMeta) { + unsafe fn new_archetype(($($subparam,)*): &mut Self::State, _archetype: &Archetype, _system_meta: &mut SystemMeta) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { $($param::new_archetype($param, _archetype, _system_meta);)* } + unsafe { $($subparam::new_archetype($subparam, _archetype, _system_meta);)* } } #[inline] - fn apply(($($param,)*): &mut Self::State, _system_meta: &SystemMeta, _world: &mut World) { - $($param::apply($param, _system_meta, _world);)* + fn apply(($($subparam,)*): &mut Self::State, _system_meta: &SystemMeta, _world: &mut World) { + $($subparam::apply($subparam, _system_meta, _world);)* } #[inline] - fn queue(($($param,)*): &mut Self::State, _system_meta: &SystemMeta, mut _world: DeferredWorld) { - $($param::queue($param, _system_meta, _world.reborrow());)* - } - - #[inline] - unsafe fn validate_param( - state: &Self::State, - _system_meta: &SystemMeta, - _world: UnsafeWorldCell, - ) -> bool { - let ($($param,)*) = state; - $($param::validate_param($param, _system_meta, _world)&&)* true + fn queue(($($subparam,)*): &mut Self::State, _system_meta: &SystemMeta, mut _world: DeferredWorld) { + $($subparam::queue($subparam, _system_meta, _world.reborrow());)* } #[inline] @@ -1935,9 +1767,10 @@ macro_rules! impl_system_param_tuple { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - let ($($param,)*) = state; - ($($param::get_param($param, _system_meta, _world, _change_tick),)*) + ) -> Option> { + let ($($subparam,)*) = state; + let param = ($($subparam::get_param($subparam, _system_meta, _world, _change_tick)?,)*); + Some(param) } } }; @@ -2084,24 +1917,16 @@ unsafe impl SystemParam for StaticSystemParam<'_, '_, P::queue(state, system_meta, world); } - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - P::validate_param(state, system_meta, world) - } - #[inline] unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { - // SAFETY: Defer to the safety of P::SystemParam - StaticSystemParam(unsafe { P::get_param(state, system_meta, world, change_tick) }) + ) -> Option> { + // SAFETY: Delegate to existing `SystemParam` implementations. + let param = unsafe { P::get_param(state, system_meta, world, change_tick) }?; + Some(StaticSystemParam(param)) } } @@ -2118,8 +1943,9 @@ unsafe impl SystemParam for PhantomData { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { - PhantomData + ) -> Option> { + let param = PhantomData; + Some(param) } } @@ -2298,7 +2124,10 @@ where // - The caller ensures the world has access for the underlying system param, // and since the downcast succeeded, the underlying system param is T. // - The caller ensures the `world` matches. - unsafe { T::Item::get_param(&mut state.0, system_meta, world, change_tick) } + let maybe_param = + unsafe { T::Item::get_param(&mut state.0, system_meta, world, change_tick) }; + // `DynSystemParam` ensures param is accessible. + maybe_param.unwrap() }) } @@ -2331,11 +2160,17 @@ trait DynParamState: Sync + Send { /// Queues any deferred mutations to be applied at the next [`apply_deferred`](crate::prelude::apply_deferred). fn queue(&mut self, system_meta: &SystemMeta, world: DeferredWorld); - /// Refer to [`SystemParam::validate_param`]. + /// Wrapper around [`SystemParam::get_param`]. /// /// # Safety - /// Refer to [`SystemParam::validate_param`]. - unsafe fn validate_param(&self, system_meta: &SystemMeta, world: UnsafeWorldCell) -> bool; + /// + /// - Refer to [`SystemParam::get_param`]. + unsafe fn get_param<'w, 's>( + &'s mut self, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> Option>; } /// A wrapper around a [`SystemParam::State`] that can be used as a trait object in a [`DynSystemParam`]. @@ -2359,8 +2194,23 @@ impl DynParamState for ParamState { T::queue(&mut self.0, system_meta, world); } - unsafe fn validate_param(&self, system_meta: &SystemMeta, world: UnsafeWorldCell) -> bool { - T::validate_param(&self.0, system_meta, world) + unsafe fn get_param<'w, 's>( + &'s mut self, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> Option> { + // TODO: Reduce `get_param` used for validation. + T::get_param(&mut self.0, system_meta, world, change_tick)?; + // SAFETY: + // - `state.0` is a boxed `ParamState`, and its implementation of `as_any_mut` returns `self`. + // - The state was obtained from `SystemParamBuilder::build()`, which registers all [`World`] accesses used + // by [`SystemParam::get_param`] with the provided [`system_meta`](SystemMeta). + // - The caller ensures that the provided world is the same and has the required access. + let param = unsafe { + DynSystemParam::new(self.as_any_mut(), world, system_meta.clone(), change_tick) + }; + Some(param) } } @@ -2374,35 +2224,18 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> { DynSystemParamState::new::<()>(()) } - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - state.0.validate_param(system_meta, world) - } - #[inline] unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { - // SAFETY: - // - `state.0` is a boxed `ParamState`, and its implementation of `as_any_mut` returns `self`. - // - The state was obtained from `SystemParamBuilder::build()`, which registers all [`World`] accesses used - // by [`SystemParam::get_param`] with the provided [`system_meta`](SystemMeta). - // - The caller ensures that the provided world is the same and has the required access. - unsafe { - DynSystemParam::new( - state.0.as_any_mut(), - world, - system_meta.clone(), - change_tick, - ) - } + ) -> Option> { + // SAFETY: Delegate to `DynParamState` which conforms to `SystemParam` safety. + let maybe_param = state.0.get_param(system_meta, world, change_tick); + // `DynSystemParamState` ensures param is accessible. + let param = maybe_param.unwrap(); + Some(param) } unsafe fn new_archetype( diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index 4b5508190acf1..fab7d80d2d99c 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -343,11 +343,9 @@ impl World { initialized = true; } - let result = if system.validate_param(self) { - Ok(system.run(input, self)) - } else { - Err(RegisteredSystemError::InvalidParams(id)) - }; + let result = system + .run(input, self) + .ok_or(RegisteredSystemError::InvalidParams(id)); // return ownership of system trait object (if entity still exists) if let Some(mut entity) = self.get_entity_mut(id.entity) { diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index 9187b818504b5..15143a788b7d9 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -61,8 +61,9 @@ unsafe impl SystemParam for WorldId { _: &SystemMeta, world: UnsafeWorldCell<'world>, _: Tick, - ) -> Self::Item<'world, 'state> { - world.id() + ) -> Option> { + let param = world.id(); + Some(param) } } diff --git a/crates/bevy_gizmos/src/gizmos.rs b/crates/bevy_gizmos/src/gizmos.rs index 86df15333949f..f37002ff62851 100644 --- a/crates/bevy_gizmos/src/gizmos.rs +++ b/crates/bevy_gizmos/src/gizmos.rs @@ -186,23 +186,13 @@ where GizmosState::::apply(&mut state.state, system_meta, world); } - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> bool { - // SAFETY: Delegated to existing `SystemParam` implementations. - unsafe { GizmosState::::validate_param(&state.state, system_meta, world) } - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { // SAFETY: Delegated to existing `SystemParam` implementations. let (f0, f1) = unsafe { GizmosState::::get_param( @@ -211,17 +201,17 @@ where world, change_tick, ) - }; + }?; // Accessing the GizmoConfigStore in the immediate mode API reduces performance significantly. // Implementing SystemParam manually allows us to do it to here // Having config available allows for early returns when gizmos are disabled let (config, config_ext) = f1.into_inner().config::(); - Gizmos { + Some(Gizmos { buffer: f0, enabled: config.enabled, config, config_ext, - } + }) } } diff --git a/crates/bevy_hierarchy/src/query_extension.rs b/crates/bevy_hierarchy/src/query_extension.rs index 36bf790cec1bd..46c573092ed20 100644 --- a/crates/bevy_hierarchy/src/query_extension.rs +++ b/crates/bevy_hierarchy/src/query_extension.rs @@ -176,7 +176,7 @@ mod tests { world.entity_mut(c).add_children(&[d]); let mut system_state = SystemState::<(Query<&Children>, Query<&A>)>::new(world); - let (children_query, a_query) = system_state.get(world); + let (children_query, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query .iter_many(children_query.iter_descendants(a)) @@ -195,7 +195,7 @@ mod tests { world.entity_mut(b).add_children(&[c]); let mut system_state = SystemState::<(Query<&Parent>, Query<&A>)>::new(world); - let (parent_query, a_query) = system_state.get(world); + let (parent_query, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query.iter_many(parent_query.iter_ancestors(c)).collect(); diff --git a/crates/bevy_pbr/src/meshlet/instance_manager.rs b/crates/bevy_pbr/src/meshlet/instance_manager.rs index 4c609848a600e..9a6a5820e2a80 100644 --- a/crates/bevy_pbr/src/meshlet/instance_manager.rs +++ b/crates/bevy_pbr/src/meshlet/instance_manager.rs @@ -189,7 +189,7 @@ pub fn extract_meshlet_mesh_entities( } let system_state = system_state.as_mut().unwrap(); let (instances_query, asset_server, mut assets, mut asset_events) = - system_state.get_mut(&mut main_world); + system_state.get_mut(&mut main_world).unwrap(); // Reset per-frame data instance_manager.reset(render_entities); diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index 91f93c61b2560..c121cae34feec 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -1190,7 +1190,7 @@ impl FromWorld for MeshPipeline { Res, )> = SystemState::new(world); let (render_device, default_sampler, render_queue, view_layouts) = - system_state.get_mut(world); + system_state.get_mut(world).unwrap(); let clustered_forward_buffer_binding_type = render_device .get_supported_read_only_binding_type(CLUSTERED_FORWARD_STORAGE_BUFFER_COUNT); diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 914fe32dfb6e4..e33a53749c8a3 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -74,35 +74,13 @@ where } } - #[inline] - unsafe fn validate_param( - state: &Self::State, - _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 { - return false; - }; - // SAFETY: Type is guaranteed by `SystemState`. - let main_world: &World = unsafe { main_world.deref() }; - // SAFETY: We provide the main world on which this system state was initialized on. - unsafe { - SystemState::

::validate_param( - &state.state, - main_world.as_unsafe_world_cell_readonly(), - ) - } - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Option> { // SAFETY: // - The caller ensures that `world` is the same one that `init_state` was called with. // - The caller ensures that no other `SystemParam`s will conflict with the accesses we have registered. @@ -113,9 +91,9 @@ where world, change_tick, ) - }; - let item = state.state.get(main_world.into_inner()); - Extract { item } + }?; + let item = state.state.get(main_world.into_inner())?; + Some(Extract { item }) } } diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index 40a8c1ebb0d5e..e0ea723cb929c 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -295,7 +295,12 @@ impl Plugin for RenderPlugin { let mut system_state: SystemState< Query<&RawHandleWrapperHolder, With>, > = SystemState::new(app.world_mut()); - let primary_window = system_state.get(app.world()).get_single().ok().cloned(); + let primary_window = system_state + .get(app.world()) + .unwrap() + .get_single() + .ok() + .cloned(); let settings = render_creation.clone(); let async_renderer = async move { let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { diff --git a/crates/bevy_render/src/render_asset.rs b/crates/bevy_render/src/render_asset.rs index 46ee40fd20ef8..095bd9eef93fa 100644 --- a/crates/bevy_render/src/render_asset.rs +++ b/crates/bevy_render/src/render_asset.rs @@ -259,7 +259,7 @@ pub(crate) fn extract_render_asset( ) { main_world.resource_scope( |world, mut cached_state: Mut>| { - let (mut events, mut assets) = cached_state.state.get_mut(world); + let (mut events, mut assets) = cached_state.state.get_mut(world).unwrap(); let mut changed_assets = HashSet::default(); let mut removed = HashSet::default(); diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 2deafa4bf2396..ab0693d068ac9 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -316,7 +316,7 @@ where view: Entity, item: &P, ) -> Result<(), DrawError> { - let param = self.state.get_manual(world); + let param = self.state.get_manual(world).unwrap(); let view = match self.view.get_manual(world, view) { Ok(view) => view, Err(err) => match err { diff --git a/crates/bevy_render/src/renderer/mod.rs b/crates/bevy_render/src/renderer/mod.rs index 85213476bf4c4..6cfb050e91dac 100644 --- a/crates/bevy_render/src/renderer/mod.rs +++ b/crates/bevy_render/src/renderer/mod.rs @@ -77,7 +77,7 @@ pub fn render_system(world: &mut World, state: &mut SystemState>(); + let view_entities = state.get(world).unwrap().iter().collect::>(); for view_entity in view_entities { world.entity_mut(view_entity).remove::(); } diff --git a/crates/bevy_render/src/view/window/screenshot.rs b/crates/bevy_render/src/view/window/screenshot.rs index 38606a9b771ec..f1615747eed93 100644 --- a/crates/bevy_render/src/view/window/screenshot.rs +++ b/crates/bevy_render/src/view/window/screenshot.rs @@ -220,7 +220,8 @@ fn extract_screenshots( *system_state = Some(SystemState::new(&mut main_world)); } let system_state = system_state.as_mut().unwrap(); - let (mut commands, primary_window, screenshots) = system_state.get_mut(&mut main_world); + let (mut commands, primary_window, screenshots) = + system_state.get_mut(&mut main_world).unwrap(); targets.clear(); seen_targets.clear(); diff --git a/crates/bevy_render/src/world_sync.rs b/crates/bevy_render/src/world_sync.rs index cec1ce748c386..461edc366a87a 100644 --- a/crates/bevy_render/src/world_sync.rs +++ b/crates/bevy_render/src/world_sync.rs @@ -176,7 +176,7 @@ pub(crate) fn despawn_temporary_render_entities( state: &mut SystemState>>, mut local: Local>, ) { - let query = state.get(world); + let query = state.get(world).unwrap(); local.extend(query.iter()); diff --git a/crates/bevy_sprite/src/mesh2d/mesh.rs b/crates/bevy_sprite/src/mesh2d/mesh.rs index b0b5f39bc9af8..b3f9e9c515a87 100644 --- a/crates/bevy_sprite/src/mesh2d/mesh.rs +++ b/crates/bevy_sprite/src/mesh2d/mesh.rs @@ -256,7 +256,7 @@ impl FromWorld for Mesh2dPipeline { Res, Res, )> = SystemState::new(world); - let (render_device, render_queue, default_sampler) = system_state.get_mut(world); + let (render_device, render_queue, default_sampler) = system_state.get_mut(world).unwrap(); let render_device = render_device.into_inner(); let tonemapping_lut_entries = get_lut_bind_group_layout_entries(); let view_layout = render_device.create_bind_group_layout( diff --git a/crates/bevy_sprite/src/render/mod.rs b/crates/bevy_sprite/src/render/mod.rs index 63b025d5a6747..fbfbdd8640173 100644 --- a/crates/bevy_sprite/src/render/mod.rs +++ b/crates/bevy_sprite/src/render/mod.rs @@ -61,7 +61,7 @@ impl FromWorld for SpritePipeline { Res, Res, )> = SystemState::new(world); - let (render_device, default_sampler, render_queue) = system_state.get_mut(world); + let (render_device, default_sampler, render_queue) = system_state.get_mut(world).unwrap(); let tonemapping_lut_entries = get_lut_bind_group_layout_entries(); let view_layout = render_device.create_bind_group_layout( diff --git a/crates/bevy_transform/src/helper.rs b/crates/bevy_transform/src/helper.rs index 1ae3ca9616a8d..9859695d4b479 100644 --- a/crates/bevy_transform/src/helper.rs +++ b/crates/bevy_transform/src/helper.rs @@ -137,7 +137,7 @@ mod tests { let transform = *app.world().get::(leaf_entity).unwrap(); let mut state = SystemState::::new(app.world_mut()); - let helper = state.get(app.world()); + let helper = state.get(app.world()).unwrap(); let computed_transform = helper.compute_global_transform(leaf_entity).unwrap(); diff --git a/crates/bevy_ui/src/ghost_hierarchy.rs b/crates/bevy_ui/src/ghost_hierarchy.rs index 01ae3bd9450c1..9f602b0d52987 100644 --- a/crates/bevy_ui/src/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/ghost_hierarchy.rs @@ -163,7 +163,7 @@ mod tests { }); let mut system_state = SystemState::<(UiRootNodes, Query<&A>)>::new(world); - let (ui_root_nodes, a_query) = system_state.get(world); + let (ui_root_nodes, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query.iter_many(ui_root_nodes.iter()).collect(); @@ -194,7 +194,7 @@ mod tests { world.entity_mut(n9).add_children(&[n10]); let mut system_state = SystemState::<(UiChildren, Query<&A>)>::new(world); - let (ui_children, a_query) = system_state.get(world); + let (ui_children, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query .iter_many(ui_children.iter_ui_children(n1)) diff --git a/crates/bevy_winit/src/state.rs b/crates/bevy_winit/src/state.rs index 56006e56a3e31..762f587754639 100644 --- a/crates/bevy_winit/src/state.rs +++ b/crates/bevy_winit/src/state.rs @@ -228,7 +228,10 @@ impl ApplicationHandler for WinitAppRunnerState { winit_windows, mut windows, mut access_kit_adapters, - ) = self.event_writer_system_state.get_mut(self.app.world_mut()); + ) = self + .event_writer_system_state + .get_mut(self.app.world_mut()) + .unwrap(); let Some(window) = winit_windows.get_window_entity(window_id) else { warn!("Skipped event {event:?} for unknown winit Window Id {window_id:?}"); @@ -433,9 +436,12 @@ impl ApplicationHandler for WinitAppRunnerState { // (even if app did not update, some may have been created by plugin setup) let mut create_window = SystemState::>>::from_world(self.world_mut()); - create_monitors(event_loop, create_monitor.get_mut(self.world_mut())); + create_monitors( + event_loop, + create_monitor.get_mut(self.world_mut()).unwrap(), + ); create_monitor.apply(self.world_mut()); - create_windows(event_loop, create_window.get_mut(self.world_mut())); + create_windows(event_loop, create_window.get_mut(self.world_mut()).unwrap()); create_window.apply(self.world_mut()); let mut redraw_event_reader = EventCursor::::default(); @@ -449,7 +455,7 @@ impl ApplicationHandler for WinitAppRunnerState { } } - let (config, windows) = focused_windows_state.get(self.world()); + let (config, windows) = focused_windows_state.get(self.world()).unwrap(); let focused = windows.iter().any(|(_, window)| window.focused); let mut update_mode = config.update_mode(focused); @@ -536,7 +542,7 @@ impl ApplicationHandler for WinitAppRunnerState { let begin_frame_time = Instant::now(); if should_update { - let (_, windows) = focused_windows_state.get(self.world()); + let (_, windows) = focused_windows_state.get(self.world()).unwrap(); // If no windows exist, this will evaluate to `true`. let all_invisible = windows.iter().all(|w| !w.1.visible); @@ -554,7 +560,7 @@ impl ApplicationHandler for WinitAppRunnerState { } // Running the app may have changed the WinitSettings resource, so we have to re-extract it. - let (config, windows) = focused_windows_state.get(self.world()); + let (config, windows) = focused_windows_state.get(self.world()).unwrap(); let focused = windows.iter().any(|(_, window)| window.focused); update_mode = config.update_mode(focused); } @@ -782,7 +788,7 @@ impl WinitAppRunnerState { Query<(Entity, &mut PendingCursor), Changed>, )> = SystemState::new(self.world_mut()); let (winit_windows, mut cursor_cache, mut windows) = - windows_state.get_mut(self.world_mut()); + windows_state.get_mut(self.world_mut()).unwrap(); for (entity, mut pending_cursor) in windows.iter_mut() { let Some(winit_window) = winit_windows.get_window(entity) else { diff --git a/examples/async_tasks/async_compute.rs b/examples/async_tasks/async_compute.rs index 6e8d5e43b0f57..c293e1f7a3b6e 100644 --- a/examples/async_tasks/async_compute.rs +++ b/examples/async_tasks/async_compute.rs @@ -77,7 +77,7 @@ fn spawn_tasks(mut commands: Commands) { Res, )>::new(world); let (box_mesh_handle, box_material_handle) = - system_state.get_mut(world); + system_state.get_mut(world).unwrap(); (box_mesh_handle.clone(), box_material_handle.clone()) }; From d171da57ef407450cf2f24543a3016cbbfc979bd Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 15:38:43 +0200 Subject: [PATCH 02/19] add param validation to system --- crates/bevy_ecs/src/system/adapter_system.rs | 6 ++++++ crates/bevy_ecs/src/system/combinator.rs | 12 +++++++++++ .../src/system/exclusive_function_system.rs | 5 +++++ crates/bevy_ecs/src/system/function_system.rs | 20 +++++++++++++++++++ crates/bevy_ecs/src/system/system.rs | 7 +++++++ 5 files changed, 50 insertions(+) diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index df44852e418fe..34e676f97d4fd 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -162,6 +162,12 @@ where }) } + #[inline] + unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + // SAFETY: Delegate to existing `System` implementations. + self.system.try_acquire_params(world) + } + #[inline] fn run( &mut self, diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index a148409857722..1a7ed7d400622 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -186,6 +186,12 @@ where ) } + #[inline] + unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + // SAFETY: Delegate to existing `System` implementations. + self.a.try_acquire_params(world) && self.b.try_acquire_params(world) + } + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { let world = world.as_unsafe_world_cell(); Func::combine( @@ -410,6 +416,12 @@ where self.b.run_unsafe(value, world) } + #[inline] + unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + // SAFETY: Delegate to existing `System` implementations. + self.a.try_acquire_params(world) && self.b.try_acquire_params(world) + } + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { let value = self.a.run(input, world)?; self.b.run(value, world) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 432991e269248..c8629c5d16ca8 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -117,6 +117,11 @@ where panic!("Cannot run exclusive systems with a shared World reference"); } + #[inline] + unsafe fn try_acquire_params(&mut self, _world: UnsafeWorldCell) -> bool { + true + } + fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { world.last_change_tick_scope(self.system_meta.last_run, |world| { #[cfg(feature = "trace")] diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 839c38b596136..1325cb4c9220b 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -638,6 +638,26 @@ where Some(out) } + #[inline] + unsafe fn try_acquire_params( + &mut self, + world: UnsafeWorldCell, + ) -> bool { + let change_tick = world.change_tick(); + // SAFETY: + // - The caller has invoked `update_archetype_component_access`, which will panic + // if the world does not match. + // - All world accesses used by `F::Param` have been registered, so the caller + // will ensure that there are no data access conflicts. + F::Param::get_param( + self.param_state.as_mut().expect(Self::PARAM_MESSAGE), + &self.system_meta, + world, + change_tick, + ) + .is_some() + } + #[inline] fn apply_deferred(&mut self, world: &mut World) { let param_state = self.param_state.as_mut().expect(Self::PARAM_MESSAGE); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index b1756d5aa16e9..683e06e1b5233 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -94,6 +94,13 @@ pub trait System: Send + Sync + 'static { result } + /// Checks if all parameters can be acquired. + /// + /// # Safety + /// + /// - Same as [`System::run_unsafe`] + unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool; + /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. /// /// This is where [`Commands`](crate::system::Commands) get applied. From 717d006eaaa5482c73499b3220dd6fca0c85b6e7 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 16:00:16 +0200 Subject: [PATCH 03/19] readd warnings --- .../src/schedule/executor/multi_threaded.rs | 10 +++++++--- crates/bevy_ecs/src/schedule/executor/simple.rs | 8 +++++--- .../src/schedule/executor/single_threaded.rs | 8 +++++--- crates/bevy_ecs/src/system/adapter_system.rs | 4 ++-- crates/bevy_ecs/src/system/combinator.rs | 8 ++++---- .../src/system/exclusive_function_system.rs | 7 ++++++- crates/bevy_ecs/src/system/function_system.rs | 16 +++++++++------- crates/bevy_ecs/src/system/system.rs | 12 +++++++++++- 8 files changed, 49 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 53453240dcb88..1847a5e38814e 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -572,7 +572,7 @@ impl ExecutorState { // - The caller ensures that `world` has permission to read any data // required by the system. // - `update_archetype_component_access` has been called for system. - let valid_params = unsafe { system.validate_param_unsafe(world) }; + let valid_params = unsafe { system.try_acquire_params_unsafe(world) }; if !valid_params { self.skipped_systems.insert(system_index); } @@ -748,14 +748,18 @@ unsafe fn evaluate_and_fold_conditions( // - The caller ensures that `world` has permission to read any data // required by the condition. // - `update_archetype_component_access` has been called for condition. - if !unsafe { condition.validate_param_unsafe(world) } { + if !unsafe { condition.try_acquire_params_unsafe(world) } { return false; } // SAFETY: // - The caller ensures that `world` has permission to read any data // required by the condition. // - `update_archetype_component_access` has been called for condition. - unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) } + let maybe_out = unsafe { + __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) + }; + // We checked params can be acquired. + maybe_out.unwrap() }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index bd3a2abf2c3ef..46e0d2afc3433 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/crates/bevy_ecs/src/schedule/executor/simple.rs @@ -81,7 +81,7 @@ impl SystemExecutor for SimpleExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_param(world); + let valid_params = system.try_acquire_params(world); should_run &= valid_params; } @@ -135,10 +135,12 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_param(world) { + if !condition.try_acquire_params(world) { return false; } - __rust_begin_short_backtrace::readonly_run(&mut **condition, world) + let maybe_out = __rust_begin_short_backtrace::readonly_run(&mut **condition, world); + // We checked params can be acquired. + maybe_out.unwrap() }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs index d0c9f505c39f3..191e1ae6530ed 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -87,7 +87,7 @@ impl SystemExecutor for SingleThreadedExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_param(world); + let valid_params = system.try_acquire_params(world); should_run &= valid_params; } @@ -167,10 +167,12 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_param(world) { + if !condition.try_acquire_params(world) { return false; } - __rust_begin_short_backtrace::readonly_run(&mut **condition, world) + let maybe_out = __rust_begin_short_backtrace::readonly_run(&mut **condition, world); + // We checked params can be acquired. + maybe_out.unwrap() }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 34e676f97d4fd..0556b4ca74caf 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -163,9 +163,9 @@ where } #[inline] - unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.system.try_acquire_params(world) + self.system.try_acquire_params_unsafe(world) } #[inline] diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 1a7ed7d400622..d0ed0ce8e83af 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -187,9 +187,9 @@ where } #[inline] - unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.try_acquire_params(world) && self.b.try_acquire_params(world) + self.a.try_acquire_params_unsafe(world) && self.b.try_acquire_params_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { @@ -417,9 +417,9 @@ where } #[inline] - unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.try_acquire_params(world) && self.b.try_acquire_params(world) + self.a.try_acquire_params_unsafe(world) && self.b.try_acquire_params_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index c8629c5d16ca8..03de28a78f72a 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -118,7 +118,12 @@ where } #[inline] - unsafe fn try_acquire_params(&mut self, _world: UnsafeWorldCell) -> bool { + unsafe fn try_acquire_params_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + panic!("Cannot run exclusive systems with a shared World reference"); + } + + #[inline] + fn try_acquire_params(&mut self, _world: &mut World) -> bool { true } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index a142536c1bc43..e2fbd5d035a55 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -727,23 +727,25 @@ where } #[inline] - unsafe fn try_acquire_params( - &mut self, - world: UnsafeWorldCell, - ) -> bool { + unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let change_tick = world.change_tick(); + // TODO: Reduce `get_param` used for validation. // SAFETY: // - The caller has invoked `update_archetype_component_access`, which will panic // if the world does not match. // - All world accesses used by `F::Param` have been registered, so the caller // will ensure that there are no data access conflicts. - F::Param::get_param( + let maybe_param = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, world, change_tick, - ) - .is_some() + ); + let can_acquire = maybe_param.is_some(); + if !can_acquire { + self.system_meta.advance_param_warn_policy(); + } + can_acquire } #[inline] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 683e06e1b5233..8f19960f059f6 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -99,7 +99,17 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// - Same as [`System::run_unsafe`] - unsafe fn try_acquire_params(&mut self, world: UnsafeWorldCell) -> bool; + unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool; + + /// Like [`System::try_acquire_params_unsafe`] if all parameters can be acquired. + fn try_acquire_params(&mut self, world: &mut World) -> bool { + let world_cell = world.as_unsafe_world_cell(); + self.update_archetype_component_access(world_cell); + // SAFETY: + // - We have exclusive access to the entire world. + // - `update_archetype_component_access` has been called. + unsafe { self.try_acquire_params_unsafe(world_cell) } + } /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. /// From dd22f4c835dedbbe674f7f7a810860e10174d73b Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 16:34:53 +0200 Subject: [PATCH 04/19] revert exclusive world try acq params --- crates/bevy_ecs/src/system/exclusive_function_system.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 03de28a78f72a..a2aa2e2e0f74d 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -119,11 +119,6 @@ where #[inline] unsafe fn try_acquire_params_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { - panic!("Cannot run exclusive systems with a shared World reference"); - } - - #[inline] - fn try_acquire_params(&mut self, _world: &mut World) -> bool { true } From 64366cb121fb87f78569f545b57c9e16b4f77925 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 17:23:23 +0200 Subject: [PATCH 05/19] fix some docs --- crates/bevy_ecs/src/change_detection.rs | 18 +++++++++--------- crates/bevy_ecs/src/system/adapter_system.rs | 8 ++++---- crates/bevy_ecs/src/system/combinator.rs | 8 ++++---- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 64df4616e4178..fd8e7d4231b4f 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -164,17 +164,17 @@ pub trait DetectChangesMut: DetectChanges { /// # world.insert_resource(Score(1)); /// # let mut score_changed = IntoSystem::into_system(resource_changed::); /// # score_changed.initialize(&mut world); - /// # score_changed.run((), &mut world); + /// # score_changed.run((), &mut world).unwrap(); /// # /// # let mut schedule = Schedule::default(); /// # schedule.add_systems(reset_score); /// # /// # // first time `reset_score` runs, the score is changed. /// # schedule.run(&mut world); - /// # assert!(score_changed.run((), &mut world)); + /// # assert!(score_changed.run((), &mut world).unwrap()); /// # // second time `reset_score` runs, the score is not changed. /// # schedule.run(&mut world); - /// # assert!(!score_changed.run((), &mut world)); + /// # assert!(!score_changed.run((), &mut world).unwrap()); /// ``` #[inline] #[track_caller] @@ -235,23 +235,23 @@ pub trait DetectChangesMut: DetectChanges { /// # world.insert_resource(Score(1)); /// # let mut score_changed = IntoSystem::into_system(resource_changed::); /// # score_changed.initialize(&mut world); - /// # score_changed.run((), &mut world); + /// # score_changed.run((), &mut world).unwrap(); /// # /// # let mut score_changed_event = IntoSystem::into_system(on_event::); /// # score_changed_event.initialize(&mut world); - /// # score_changed_event.run((), &mut world); + /// # score_changed_event.run((), &mut world).unwrap(); /// # /// # let mut schedule = Schedule::default(); /// # schedule.add_systems(reset_score); /// # /// # // first time `reset_score` runs, the score is changed. /// # schedule.run(&mut world); - /// # assert!(score_changed.run((), &mut world)); - /// # assert!(score_changed_event.run((), &mut world)); + /// # assert!(score_changed.run((), &mut world).unwrap()); + /// # assert!(score_changed_event.run((), &mut world).unwrap()); /// # // second time `reset_score` runs, the score is not changed. /// # schedule.run(&mut world); - /// # assert!(!score_changed.run((), &mut world)); - /// # assert!(!score_changed_event.run((), &mut world)); + /// # assert!(!score_changed.run((), &mut world).unwrap()); + /// # assert!(!score_changed_event.run((), &mut world).unwrap()); /// ``` #[inline] #[must_use = "If you don't need to handle the previous value, use `set_if_neq` instead."] diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 0556b4ca74caf..d0894282f9087 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -33,15 +33,15 @@ use crate::{ /// fn adapt( /// &mut self, /// input: ::Inner<'_>, -/// run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out, -/// ) -> Self::Out { -/// !run_system(input) +/// run_system: impl FnOnce(SystemIn<'_, S>) -> Option, +/// ) -> Option { +/// Some(!(run_system(input)?)) /// } /// } /// # let mut world = World::new(); /// # let mut system = NotSystem::new(NotMarker, IntoSystem::into_system(|| false), "".into()); /// # system.initialize(&mut world); -/// # assert!(system.run((), &mut world)); +/// # assert!(system.run((), &mut world).unwrap()); /// ``` #[diagnostic::on_unimplemented( message = "`{Self}` can not adapt a system of type `{S}`", diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index d0ed0ce8e83af..bb8786c6f6c95 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -38,10 +38,10 @@ use super::{IntoSystem, ReadOnlySystem, System}; /// /// fn combine( /// _input: Self::In, -/// a: impl FnOnce(A::In) -> A::Out, -/// b: impl FnOnce(B::In) -> B::Out, -/// ) -> Self::Out { -/// a(()) ^ b(()) +/// a: impl FnOnce(A::In) -> Option, +/// b: impl FnOnce(B::In) -> Option, +/// ) -> Option { +/// Some(a(())? ^ b(())?) /// } /// } /// From 7a5a2b9d8b24f3371f40e70ec0fbe86c0ef6efdc Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 17:40:09 +0200 Subject: [PATCH 06/19] more docs fixes --- crates/bevy_ecs/src/system/combinator.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 6 +++--- crates/bevy_ecs/src/system/input.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index bb8786c6f6c95..e3ac2c00e3585 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -334,7 +334,7 @@ where /// // pipe the `parse_message_system`'s output into the `filter_system`s input /// let mut piped_system = IntoSystem::into_system(parse_message_system.pipe(filter_system)); /// piped_system.initialize(&mut world); -/// assert_eq!(piped_system.run((), &mut world), Some(42)); +/// assert_eq!(piped_system.run((), &mut world).unwrap(), Some(42)); /// } /// /// #[derive(Resource)] diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index e2fbd5d035a55..9b7573c46fd87 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -256,7 +256,7 @@ where /// /// // Use system_state.get_mut(&mut world) and unpack your system parameters into variables! /// // system_state.get(&world) provides read-only versions of your system parameters instead. -/// let (event_writer, maybe_resource, query) = system_state.get_mut(&mut world); +/// let (event_writer, maybe_resource, query) = system_state.get_mut(&mut world).unwrap(); /// /// // If you are using `Commands`, you can choose when you want to apply them to the world. /// // You need to manually call `.apply(world)` on the `SystemState` to apply them. @@ -286,7 +286,7 @@ where /// /// // Later, fetch the cached system state, saving on overhead /// world.resource_scope(|world, mut cached_state: Mut| { -/// let mut event_reader = cached_state.event_state.get_mut(world); +/// let mut event_reader = cached_state.event_state.get_mut(world).unwrap(); /// /// for events in event_reader.read() { /// println!("Hello World!"); @@ -865,7 +865,7 @@ where /// // pipe the `parse_message_system`'s output into the `filter_system`s input /// let mut piped_system = IntoSystem::into_system(pipe(parse_message, filter)); /// piped_system.initialize(&mut world); -/// assert_eq!(piped_system.run((), &mut world), Some(42)); +/// assert_eq!(piped_system.run((), &mut world).unwrap(), Some(42)); /// } /// /// #[derive(Resource)] diff --git a/crates/bevy_ecs/src/system/input.rs b/crates/bevy_ecs/src/system/input.rs index 57032f71928ef..88c5efb480041 100644 --- a/crates/bevy_ecs/src/system/input.rs +++ b/crates/bevy_ecs/src/system/input.rs @@ -62,7 +62,7 @@ impl SystemInput for () { /// let mut square_system = IntoSystem::into_system(square); /// square_system.initialize(&mut world); /// -/// assert_eq!(square_system.run(12, &mut world), 144); +/// assert_eq!(square_system.run(12, &mut world).unwrap(), 144); /// ``` /// /// [`SystemParam`]: crate::system::SystemParam From 8f44f3bc1296395147f3cc92118ade6a45027767 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 19:16:13 +0200 Subject: [PATCH 07/19] fix unsafe comment --- crates/bevy_ecs/macros/src/lib.rs | 22 +++- .../src/schedule/executor/multi_threaded.rs | 4 +- .../bevy_ecs/src/schedule/executor/simple.rs | 4 +- .../src/schedule/executor/single_threaded.rs | 4 +- crates/bevy_ecs/src/system/adapter_system.rs | 4 +- crates/bevy_ecs/src/system/combinator.rs | 8 +- .../src/system/exclusive_function_system.rs | 3 +- crates/bevy_ecs/src/system/function_system.rs | 10 +- crates/bevy_ecs/src/system/system.rs | 6 +- crates/bevy_ecs/src/system/system_param.rs | 118 ++++++++++++++++-- 10 files changed, 152 insertions(+), 31 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 219b1b5461e26..d7a2169217f51 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -389,8 +389,6 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Option> { - // TODO: Reduce `get_param` used for validation. - <(#(#param,)*) as SystemParam>::get_param(state, system_meta, world, change_tick)?; let param = ParamSet { param_states: state, system_meta: system_meta.clone(), @@ -399,6 +397,16 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { }; Some(param) } + + #[inline] + unsafe fn validate_param<'w, 's>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> bool { + <(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world, change_tick) + } } impl<'w, 's, #(#param: SystemParam,)*> ParamSet<'w, 's, (#(#param,)*)> @@ -637,6 +645,16 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let param = #struct_name { #(#fields: #field_locals,)* }; Some(param) } + + #[inline] + unsafe fn validate_param<'w, 's>( + state: &'s mut Self::State, + system_meta: &#path::system::SystemMeta, + world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, + change_tick: #path::component::Tick, + ) -> bool { + <(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&mut state.state, system_meta, world, change_tick) + } } // Safety: Each field is `ReadOnlySystemParam`, so this can only read from the `World` diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 1847a5e38814e..4ddac07947682 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -572,7 +572,7 @@ impl ExecutorState { // - The caller ensures that `world` has permission to read any data // required by the system. // - `update_archetype_component_access` has been called for system. - let valid_params = unsafe { system.try_acquire_params_unsafe(world) }; + let valid_params = unsafe { system.validate_params_unsafe(world) }; if !valid_params { self.skipped_systems.insert(system_index); } @@ -748,7 +748,7 @@ unsafe fn evaluate_and_fold_conditions( // - The caller ensures that `world` has permission to read any data // required by the condition. // - `update_archetype_component_access` has been called for condition. - if !unsafe { condition.try_acquire_params_unsafe(world) } { + if !unsafe { condition.validate_params_unsafe(world) } { return false; } // SAFETY: diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 46e0d2afc3433..2562845f9b2c6 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/crates/bevy_ecs/src/schedule/executor/simple.rs @@ -81,7 +81,7 @@ impl SystemExecutor for SimpleExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.try_acquire_params(world); + let valid_params = system.validate_params(world); should_run &= valid_params; } @@ -135,7 +135,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.try_acquire_params(world) { + if !condition.validate_params(world) { return false; } let maybe_out = __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 191e1ae6530ed..c5335cd0c2a64 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -87,7 +87,7 @@ impl SystemExecutor for SingleThreadedExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.try_acquire_params(world); + let valid_params = system.validate_params(world); should_run &= valid_params; } @@ -167,7 +167,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.try_acquire_params(world) { + if !condition.validate_params(world) { return false; } let maybe_out = __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 d0894282f9087..2276e28470870 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -163,9 +163,9 @@ where } #[inline] - unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.system.try_acquire_params_unsafe(world) + self.system.validate_params_unsafe(world) } #[inline] diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index e3ac2c00e3585..b4ddb94476a7e 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -187,9 +187,9 @@ where } #[inline] - unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.try_acquire_params_unsafe(world) && self.b.try_acquire_params_unsafe(world) + self.a.validate_params_unsafe(world) && self.b.validate_params_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { @@ -417,9 +417,9 @@ where } #[inline] - unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.try_acquire_params_unsafe(world) && self.b.try_acquire_params_unsafe(world) + self.a.validate_params_unsafe(world) && self.b.validate_params_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index a2aa2e2e0f74d..9585c6d4e0ef2 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -118,7 +118,8 @@ where } #[inline] - unsafe fn try_acquire_params_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + unsafe fn validate_params_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + // All exclusive system parameters 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 9b7573c46fd87..8b23d478eb0bb 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -727,25 +727,23 @@ where } #[inline] - unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let change_tick = world.change_tick(); - // TODO: Reduce `get_param` used for validation. // SAFETY: // - The caller has invoked `update_archetype_component_access`, which will panic // if the world does not match. // - All world accesses used by `F::Param` have been registered, so the caller // will ensure that there are no data access conflicts. - let maybe_param = F::Param::get_param( + let is_valid = F::Param::validate_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, world, change_tick, ); - let can_acquire = maybe_param.is_some(); - if !can_acquire { + if !is_valid { self.system_meta.advance_param_warn_policy(); } - can_acquire + is_valid } #[inline] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 8f19960f059f6..c86854dc0adc1 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -99,16 +99,16 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// - Same as [`System::run_unsafe`] - unsafe fn try_acquire_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool; + unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool; /// Like [`System::try_acquire_params_unsafe`] if all parameters can be acquired. - fn try_acquire_params(&mut self, world: &mut World) -> bool { + fn validate_params(&mut self, world: &mut World) -> bool { let world_cell = world.as_unsafe_world_cell(); self.update_archetype_component_access(world_cell); // SAFETY: // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. - unsafe { self.try_acquire_params_unsafe(world_cell) } + unsafe { self.validate_params_unsafe(world_cell) } } /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index ee167780b5723..d9d289e84b6cd 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -9,7 +9,7 @@ use crate::{ Access, AccessConflicts, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter, QuerySingleError, QueryState, ReadOnlyQueryData, }, - storage::SparseSetIndex, + storage::{ResourceData, SparseSetIndex}, system::{Query, Single, SystemMeta}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World}, }; @@ -232,6 +232,29 @@ pub unsafe trait SystemParam: Sized { world: UnsafeWorldCell<'world>, change_tick: Tick, ) -> Option>; + + /// Check if param can be acquired. + /// The default implementation relies on [`Self::get_param`], + /// but has to be overwriten in cases like [`NonSend`]. + /// + /// # Safety + /// + /// - The passed [`UnsafeWorldCell`] must have access to any world data + /// registered in [`init_state`](SystemParam::init_state). + /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). + /// - all `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). + unsafe fn validate_param( + state: &mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool { + let is_valid = Self::get_param(state, system_meta, world, change_tick).is_some(); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid + } } /// A [`SystemParam`] that only reads a given [`World`]. @@ -1324,6 +1347,24 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { }; Some(param) } + + #[inline] + unsafe fn validate_param( + &mut component_id: &mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + _change_tick: Tick, + ) -> bool { + // SAFETY: Read-only access to resource metadata. + let is_valid = unsafe { world.storages() } + .non_send_resources + .get(component_id) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid + } } // SAFETY: Only reads a single World non-send resource @@ -1357,6 +1398,16 @@ unsafe impl SystemParam for Option> { }); Some(param) } + + #[inline] + unsafe fn validate_param( + &mut _component_id: &mut Self::State, + _system_meta: &SystemMeta, + _world: UnsafeWorldCell, + _change_tick: Tick, + ) -> bool { + true + } } // SAFETY: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this @@ -1651,10 +1702,6 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { world: UnsafeWorldCell<'world>, change_tick: Tick, ) -> Option> { - // TODO: Reduce `get_param` used for validation. - for state in state.iter_mut() { - T::get_param(state, system_meta, world, change_tick)?; - } let param = ParamSet { param_states: state, system_meta: system_meta.clone(), @@ -1664,6 +1711,21 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { Some(param) } + #[inline] + unsafe fn validate_param( + state: &mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool { + // SAFETY: Delegate to existing `SystemParam` implementations. + unsafe { + state + .iter_mut() + .all(|state| T::validate_param(state, system_meta, world, change_tick)) + } + } + unsafe fn new_archetype( state: &mut Self::State, archetype: &Archetype, @@ -1772,6 +1834,17 @@ macro_rules! impl_system_param_tuple { let param = ($($subparam::get_param($subparam, _system_meta, _world, _change_tick)?,)*); Some(param) } + + #[inline] + unsafe fn validate_param( + state: &mut Self::State, + _system_meta: &SystemMeta, + _world: UnsafeWorldCell, + _change_tick: Tick, + ) -> bool { + let ($($subparam,)*) = state; + $($subparam::validate_param($subparam, _system_meta, _world, _change_tick)&&)* true + } } }; } @@ -2171,6 +2244,18 @@ trait DynParamState: Sync + Send { world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Option>; + + /// Wrapper around [`SystemParam::validate_param`]. + /// + /// # Safety + /// + /// - Refer to [`SystemParam::validate_param`]. + unsafe fn validate_param( + &mut self, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool; } /// A wrapper around a [`SystemParam::State`] that can be used as a trait object in a [`DynSystemParam`]. @@ -2200,8 +2285,6 @@ impl DynParamState for ParamState { world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Option> { - // TODO: Reduce `get_param` used for validation. - T::get_param(&mut self.0, system_meta, world, change_tick)?; // SAFETY: // - `state.0` is a boxed `ParamState`, and its implementation of `as_any_mut` returns `self`. // - The state was obtained from `SystemParamBuilder::build()`, which registers all [`World`] accesses used @@ -2212,6 +2295,16 @@ impl DynParamState for ParamState { }; Some(param) } + + unsafe fn validate_param( + &mut self, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool { + // SAFETY: Delegate to existing `SystemParam` implementations. + T::validate_param(&mut self.0, system_meta, world, change_tick) + } } // SAFETY: `init_state` creates a state of (), which performs no access. The interesting safety checks are on the `SystemParamBuilder`. @@ -2238,6 +2331,17 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> { Some(param) } + #[inline] + unsafe fn validate_param( + state: &mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool { + // SAFETY: Delegate to `DynParamState` which conforms to `SystemParam` safety. + state.0.validate_param(system_meta, world, change_tick) + } + unsafe fn new_archetype( state: &mut Self::State, archetype: &Archetype, From 13b5552a3b8817c174183f37d8cf9705b3c2d864 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 19:23:21 +0200 Subject: [PATCH 08/19] ci fixes --- crates/bevy_ecs/src/system/adapter_system.rs | 3 ++- crates/bevy_ecs/src/system/system_param.rs | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 2276e28470870..806c6952048b8 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -35,7 +35,8 @@ use crate::{ /// input: ::Inner<'_>, /// run_system: impl FnOnce(SystemIn<'_, S>) -> Option, /// ) -> Option { -/// Some(!(run_system(input)?)) +/// let out = run_system(input)?; +/// Some(!out) /// } /// } /// # let mut world = World::new(); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index d9d289e84b6cd..7b246fd17e49e 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -235,7 +235,7 @@ pub unsafe trait SystemParam: Sized { /// Check if param can be acquired. /// The default implementation relies on [`Self::get_param`], - /// but has to be overwriten in cases like [`NonSend`]. + /// but has to be overwritten in cases like [`NonSend`]. /// /// # Safety /// @@ -946,10 +946,10 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { /// write_system.initialize(world); /// read_system.initialize(world); /// -/// assert_eq!(read_system.run((), world), 0); +/// assert_eq!(read_system.run((), world).unwrap(), 0); /// write_system.run((), world); /// // Note how the read local is still 0 due to the locals not being shared. -/// assert_eq!(read_system.run((), world), 0); +/// assert_eq!(read_system.run((), world).unwrap(), 0); /// ``` /// /// N.B. A [`Local`]s value cannot be read or written to outside of the containing system. From f6c97d108bf5b6c89065767827924f5be25c8230 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 19:40:56 +0200 Subject: [PATCH 09/19] more fixes --- .../tests/ui/system_state_get_lifetime_safety.rs | 4 ++-- .../tests/ui/system_state_iter_lifetime_safety.rs | 4 ++-- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 4 ++-- crates/bevy_ecs/src/schedule/executor/simple.rs | 4 ++-- crates/bevy_ecs/src/schedule/executor/single_threaded.rs | 4 ++-- crates/bevy_ecs/src/system/adapter_system.rs | 4 ++-- crates/bevy_ecs/src/system/combinator.rs | 8 ++++---- crates/bevy_ecs/src/system/exclusive_function_system.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 2 +- crates/bevy_ecs/src/system/system.rs | 8 ++++---- 10 files changed, 22 insertions(+), 22 deletions(-) diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs index e1912372b61fd..fb748124405b8 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs @@ -15,9 +15,9 @@ struct State { impl State { fn get_component(&mut self, world: &mut World, entity: Entity) { let q1 = self.state_r.get(&world); - let a1 = q1.get(entity).unwrap(); + let a1 = q1.get(entity).unwrap().unwrap(); - let mut q2 = self.state_w.get_mut(world); + let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 let _ = q2.get_mut(entity).unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs index 88325b2a5f2db..ebef8c91b33e3 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs @@ -15,9 +15,9 @@ struct State { impl State { fn get_components(&mut self, world: &mut World) { let q1 = self.state_r.get(&world); - let a1 = q1.iter().next().unwrap(); + let a1 = q1.iter().next().unwrap().unwrap(); - let mut q2 = self.state_w.get_mut(world); + let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 let _ = q2.iter_mut().next().unwrap(); diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 4ddac07947682..b4a47de39e770 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -572,7 +572,7 @@ impl ExecutorState { // - The caller ensures that `world` has permission to read any data // required by the system. // - `update_archetype_component_access` has been called for system. - let valid_params = unsafe { system.validate_params_unsafe(world) }; + let valid_params = unsafe { system.validate_param_unsafe(world) }; if !valid_params { self.skipped_systems.insert(system_index); } @@ -748,7 +748,7 @@ unsafe fn evaluate_and_fold_conditions( // - The caller ensures that `world` has permission to read any data // required by the condition. // - `update_archetype_component_access` has been called for condition. - if !unsafe { condition.validate_params_unsafe(world) } { + if !unsafe { condition.validate_param_unsafe(world) } { return false; } // SAFETY: diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 2562845f9b2c6..0c3e64ab6f55a 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/crates/bevy_ecs/src/schedule/executor/simple.rs @@ -81,7 +81,7 @@ impl SystemExecutor for SimpleExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_params(world); + let valid_params = system.validate_param(world); should_run &= valid_params; } @@ -135,7 +135,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_params(world) { + if !condition.validate_param(world) { return false; } let maybe_out = __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 c5335cd0c2a64..7fcff70abe35f 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -87,7 +87,7 @@ impl SystemExecutor for SingleThreadedExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_params(world); + let valid_params = system.validate_param(world); should_run &= valid_params; } @@ -167,7 +167,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_params(world) { + if !condition.validate_param(world) { return false; } let maybe_out = __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 806c6952048b8..1f4d890bb8daa 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -164,9 +164,9 @@ where } #[inline] - unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.system.validate_params_unsafe(world) + self.system.validate_param_unsafe(world) } #[inline] diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index b4ddb94476a7e..6202db189d345 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -187,9 +187,9 @@ where } #[inline] - unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.validate_params_unsafe(world) && self.b.validate_params_unsafe(world) + self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { @@ -417,9 +417,9 @@ where } #[inline] - unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.validate_params_unsafe(world) && self.b.validate_params_unsafe(world) + self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 9585c6d4e0ef2..e143a19885b68 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -118,7 +118,7 @@ where } #[inline] - unsafe fn validate_params_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { // All exclusive system parameters 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 8b23d478eb0bb..d5dbe08277006 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -727,7 +727,7 @@ where } #[inline] - unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let change_tick = world.change_tick(); // SAFETY: // - The caller has invoked `update_archetype_component_access`, which will panic diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index c86854dc0adc1..ffd2a83e321ed 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -99,16 +99,16 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// - Same as [`System::run_unsafe`] - unsafe fn validate_params_unsafe(&mut self, world: UnsafeWorldCell) -> bool; + unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool; - /// Like [`System::try_acquire_params_unsafe`] if all parameters can be acquired. - fn validate_params(&mut self, world: &mut World) -> bool { + /// Like [`System::validate_param_unsafe`] if all parameters can be acquired. + fn validate_param(&mut self, world: &mut World) -> bool { let world_cell = world.as_unsafe_world_cell(); self.update_archetype_component_access(world_cell); // SAFETY: // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. - unsafe { self.validate_params_unsafe(world_cell) } + unsafe { self.validate_param_unsafe(world_cell) } } /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. From 492415a26cca74935b41c27b9b2f5f2a3b4ac3c3 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 19:48:24 +0200 Subject: [PATCH 10/19] fix nonsendmut --- crates/bevy_ecs/src/system/system_param.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 7b246fd17e49e..96a285c9f0ad2 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1459,6 +1459,16 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { }; Some(param) } + + #[inline] + unsafe fn validate_param( + state: &mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + change_tick: Tick, + ) -> bool { + NonSend::::validate_param(state, system_meta, world, change_tick) + } } // SAFETY: this impl defers to `NonSendMut`, which initializes and validates the correct world access. @@ -1487,6 +1497,16 @@ unsafe impl<'a, T: 'static> SystemParam for Option> { }); Some(param) } + + #[inline] + unsafe fn validate_param( + &mut _component_id: &mut Self::State, + _system_meta: &SystemMeta, + _world: UnsafeWorldCell, + _change_tick: Tick, + ) -> bool { + true + } } // SAFETY: Only reads World archetypes From 9bd020279077cc9d7343c4aa25244766df49b0f9 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 19:58:02 +0200 Subject: [PATCH 11/19] more fixes... --- crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs | 2 +- crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs | 2 +- .../compile_fail/tests/ui/system_state_get_lifetime_safety.rs | 2 +- .../compile_fail/tests/ui/system_state_iter_lifetime_safety.rs | 2 +- .../tests/ui/system_state_iter_mut_overlap_safety.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs index a8db25b2235d3..22b05c15b628d 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs @@ -10,7 +10,7 @@ fn main() { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); dbg!("hi"); { let data: &Foo = query.get(e).unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs index 489c81d356bec..38ff28045393a 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs @@ -12,7 +12,7 @@ fn main() { world.spawn(Foo(10)); let mut system_state = SystemState::>::new(&mut world); - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); { let mut lens_a = query.transmute_lens::<&mut Foo>(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs index fb748124405b8..ddf25ccfde047 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs @@ -15,7 +15,7 @@ struct State { impl State { fn get_component(&mut self, world: &mut World, entity: Entity) { let q1 = self.state_r.get(&world); - let a1 = q1.get(entity).unwrap().unwrap(); + let a1 = q1.get(entity).unwrap(); let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs index ebef8c91b33e3..9b868f3862850 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs @@ -15,7 +15,7 @@ struct State { impl State { fn get_components(&mut self, world: &mut World) { let q1 = self.state_r.get(&world); - let a1 = q1.iter().next().unwrap().unwrap(); + let a1 = q1.iter().next().unwrap(); let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs index 3c767b91be919..bae4d55d9c49c 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs @@ -11,7 +11,7 @@ fn main() { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); let mut_vec = query.iter_mut().collect::>>(); assert_eq!( // this should fail to compile due to the later use of mut_vec From 4fc8f33ef42d52e48723bf3f59ada3ec14dcd753 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 20:08:17 +0200 Subject: [PATCH 12/19] hopefuly last ci fix --- .../compile_fail/tests/ui/system_state_get_lifetime_safety.rs | 2 +- .../compile_fail/tests/ui/system_state_iter_lifetime_safety.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs index ddf25ccfde047..82680d44acbcf 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs @@ -14,7 +14,7 @@ struct State { impl State { fn get_component(&mut self, world: &mut World, entity: Entity) { - let q1 = self.state_r.get(&world); + let q1 = self.state_r.get(&world).unwrap(); let a1 = q1.get(entity).unwrap(); let mut q2 = self.state_w.get_mut(world).unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs index 9b868f3862850..6582cf63d0ff3 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs @@ -14,7 +14,7 @@ struct State { impl State { fn get_components(&mut self, world: &mut World) { - let q1 = self.state_r.get(&world); + let q1 = self.state_r.get(&world).unwrap(); let a1 = q1.iter().next().unwrap(); let mut q2 = self.state_w.get_mut(world).unwrap(); From 1310b4134d067f04e0593c4fb3d988ba24658b31 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 22:28:09 +0200 Subject: [PATCH 13/19] patch benches --- benches/benches/bevy_ecs/fragmentation/mod.rs | 4 ++-- benches/benches/bevy_ecs/world/world_get.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/benches/benches/bevy_ecs/fragmentation/mod.rs b/benches/benches/bevy_ecs/fragmentation/mod.rs index ae44aae4a48c5..edd224a607863 100644 --- a/benches/benches/bevy_ecs/fragmentation/mod.rs +++ b/benches/benches/bevy_ecs/fragmentation/mod.rs @@ -25,7 +25,7 @@ fn iter_frag_empty(c: &mut Criterion) { spawn_empty_frag_archetype::(&mut world); let mut q: SystemState> = SystemState::)>>::new(&mut world); - let query = q.get(&world); + let query = q.get(&world).unwrap(); b.iter(move || { let mut res = 0; query.iter().for_each(|(e, t)| { @@ -39,7 +39,7 @@ fn iter_frag_empty(c: &mut Criterion) { spawn_empty_frag_archetype::(&mut world); let mut q: SystemState> = SystemState::)>>::new(&mut world); - let query = q.get(&world); + let query = q.get(&world).unwrap(); b.iter(move || { let mut res = 0; query.iter().for_each(|(e, t)| { diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index 4c235cd1b46e3..3cac653f41dd5 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -269,7 +269,7 @@ pub fn query_get(criterion: &mut Criterion) { .collect(); entities.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -288,7 +288,7 @@ pub fn query_get(criterion: &mut Criterion) { .collect(); entities.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -319,7 +319,7 @@ pub fn query_get_many(criterion: &mut Criterion) { entity_groups.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -342,7 +342,7 @@ pub fn query_get_many(criterion: &mut Criterion) { entity_groups.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; From 1eb4ed2c4d830a8a225e6b63cc6777777cc1274e Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 23:11:59 +0200 Subject: [PATCH 14/19] post merge update --- crates/bevy_ecs/src/system/system_param.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 32d7bfee38811..48a134128604f 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -2386,10 +2386,12 @@ unsafe impl SystemParam for FilteredResources<'_, '_> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Option> { // SAFETY: The caller ensures that `world` has access to anything registered in `init_state` or `build`, // and the builder registers `access` in `build`. - unsafe { FilteredResources::new(world, state, system_meta.last_run, change_tick) } + let param = + unsafe { FilteredResources::new(world, state, system_meta.last_run, change_tick) }; + Some(param) } } @@ -2413,10 +2415,12 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Option> { // SAFETY: The caller ensures that `world` has access to anything registered in `init_state` or `build`, // and the builder registers `access` in `build`. - unsafe { FilteredResourcesMut::new(world, state, system_meta.last_run, change_tick) } + let param = + unsafe { FilteredResourcesMut::new(world, state, system_meta.last_run, change_tick) }; + Some(param) } } From f06b4d0a380d2cb3260d9de375cdf81d7da7d81d Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sat, 12 Oct 2024 17:28:50 +0200 Subject: [PATCH 15/19] only check first params for combined systems --- .../bevy_ecs/src/schedule/executor/multi_threaded.rs | 4 ++-- crates/bevy_ecs/src/schedule/executor/simple.rs | 4 ++-- .../src/schedule/executor/single_threaded.rs | 4 ++-- crates/bevy_ecs/src/system/adapter_system.rs | 4 ++-- crates/bevy_ecs/src/system/combinator.rs | 8 ++++---- .../bevy_ecs/src/system/exclusive_function_system.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 2 +- crates/bevy_ecs/src/system/system.rs | 12 +++++++----- crates/bevy_ecs/src/system/system_param.rs | 2 +- 9 files changed, 22 insertions(+), 20 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index b4a47de39e770..5fb730ee8bc41 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -572,7 +572,7 @@ impl ExecutorState { // - The caller ensures that `world` has permission to read any data // required by the system. // - `update_archetype_component_access` has been called for system. - let valid_params = unsafe { system.validate_param_unsafe(world) }; + let valid_params = unsafe { system.can_run_unsafe(world) }; if !valid_params { self.skipped_systems.insert(system_index); } @@ -748,7 +748,7 @@ unsafe fn evaluate_and_fold_conditions( // - The caller ensures that `world` has permission to read any data // required by the condition. // - `update_archetype_component_access` has been called for condition. - if !unsafe { condition.validate_param_unsafe(world) } { + if !unsafe { condition.can_run_unsafe(world) } { return false; } // SAFETY: diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 0c3e64ab6f55a..bee73443c53e2 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/crates/bevy_ecs/src/schedule/executor/simple.rs @@ -81,7 +81,7 @@ impl SystemExecutor for SimpleExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_param(world); + let valid_params = system.can_run(world); should_run &= valid_params; } @@ -135,7 +135,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_param(world) { + if !condition.can_run(world) { return false; } let maybe_out = __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 7fcff70abe35f..c8a9ee0c1953d 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -87,7 +87,7 @@ impl SystemExecutor for SingleThreadedExecutor { let system = &mut schedule.systems[system_index]; if should_run { - let valid_params = system.validate_param(world); + let valid_params = system.can_run(world); should_run &= valid_params; } @@ -167,7 +167,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W conditions .iter_mut() .map(|condition| { - if !condition.validate_param(world) { + if !condition.can_run(world) { return false; } let maybe_out = __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 1f4d890bb8daa..cbda2ff4bac55 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -164,9 +164,9 @@ where } #[inline] - unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn can_run_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.system.validate_param_unsafe(world) + self.system.can_run_unsafe(world) } #[inline] diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 6202db189d345..e804e3f4c7d6b 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -187,9 +187,9 @@ where } #[inline] - unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn can_run_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) + self.a.can_run_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { @@ -417,9 +417,9 @@ where } #[inline] - unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn can_run_unsafe(&mut self, world: UnsafeWorldCell) -> bool { // SAFETY: Delegate to existing `System` implementations. - self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) + self.a.can_run_unsafe(world) } fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Option { diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index e143a19885b68..bbcd347ab05db 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -118,7 +118,7 @@ where } #[inline] - unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + unsafe fn can_run_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { // All exclusive system parameters 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 d5dbe08277006..f406dcb673762 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -727,7 +727,7 @@ where } #[inline] - unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { + unsafe fn can_run_unsafe(&mut self, world: UnsafeWorldCell) -> bool { let change_tick = world.change_tick(); // SAFETY: // - The caller has invoked `update_archetype_component_access`, which will panic diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index ffd2a83e321ed..10542d57bf681 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -94,21 +94,23 @@ pub trait System: Send + Sync + 'static { result } - /// Checks if all parameters can be acquired. + /// Checks if initial system params can be acquired. + /// When running multiple chained, combined, etc. systems, this + /// only checks whether the first system can acquire it's params. /// /// # Safety /// /// - Same as [`System::run_unsafe`] - unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool; + unsafe fn can_run_unsafe(&mut self, world: UnsafeWorldCell) -> bool; - /// Like [`System::validate_param_unsafe`] if all parameters can be acquired. - fn validate_param(&mut self, world: &mut World) -> bool { + /// Like [`System::can_run_unsafe`], but safe. + fn can_run(&mut self, world: &mut World) -> bool { let world_cell = world.as_unsafe_world_cell(); self.update_archetype_component_access(world_cell); // SAFETY: // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. - unsafe { self.validate_param_unsafe(world_cell) } + unsafe { self.can_run_unsafe(world_cell) } } /// Applies any [`Deferred`](crate::system::Deferred) system parameters (or other system buffers) of this system to the world. diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 48a134128604f..ce6f7f192cb8d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -237,7 +237,7 @@ pub unsafe trait SystemParam: Sized { ) -> Option>; /// Check if param can be acquired. - /// The default implementation relies on [`Self::get_param`], + /// The default implementation relies on [`SystemParam::get_param`], /// but has to be overwritten in cases like [`NonSend`]. /// /// # Safety From 75ffb40214e7cfecbc945f5b4a822f0e5e5735be Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sat, 12 Oct 2024 18:07:02 +0200 Subject: [PATCH 16/19] post merge fix --- crates/bevy_hierarchy/src/query_extension.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/query_extension.rs b/crates/bevy_hierarchy/src/query_extension.rs index 616dd49ff34ea..891f385ad5f52 100644 --- a/crates/bevy_hierarchy/src/query_extension.rs +++ b/crates/bevy_hierarchy/src/query_extension.rs @@ -354,7 +354,7 @@ mod tests { world.entity_mut(a1).add_children(&[a3]); let mut system_state = SystemState::<(Query<&Children>, Query<&A>)>::new(world); - let (children_query, a_query) = system_state.get(world); + let (children_query, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query .iter_many(children_query.iter_descendants_depth_first(a0)) @@ -390,7 +390,7 @@ mod tests { world.entity_mut(a1).add_children(&[a2]); let mut system_state = SystemState::>::new(world); - let parent_query = system_state.get(world); + let parent_query = system_state.get(world).unwrap(); assert_eq!(a0, parent_query.root_ancestor(a2)); assert_eq!(a0, parent_query.root_ancestor(a1)); @@ -407,7 +407,7 @@ mod tests { world.entity_mut(a1).add_children(&[a3]); let mut system_state = SystemState::<(Query<&Children>, Query<&A>)>::new(world); - let (children_query, a_query) = system_state.get(world); + let (children_query, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query.iter_many(children_query.iter_leaves(a0)).collect(); @@ -425,7 +425,7 @@ mod tests { let mut system_state = SystemState::<(Query<(Option<&Parent>, Option<&Children>)>, Query<&A>)>::new(world); - let (hierarchy_query, a_query) = system_state.get(world); + let (hierarchy_query, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query .iter_many(hierarchy_query.iter_siblings(a1)) From 0bf1250de278ef36e1a943d8992ceb78f5b9ba5a Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 13 Oct 2024 18:07:07 +0200 Subject: [PATCH 17/19] update test requirements --- crates/bevy_ecs/src/schedule/executor/mod.rs | 85 ++++++++++---------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 8330b4af2a4b8..c6260847772e0 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -184,83 +184,84 @@ mod __rust_begin_short_backtrace { mod tests { use crate::{ self as bevy_ecs, - prelude::{IntoSystemConfigs, IntoSystemSetConfigs, Resource, Schedule, SystemSet}, + prelude::{Resource, Schedule}, schedule::ExecutorKind, - system::{Commands, In, IntoSystem, Res}, + system::{In, IntoSystem, Res, ResMut}, world::World, }; #[derive(Resource)] struct R1; - #[derive(Resource)] - struct R2; - const EXECUTORS: [ExecutorKind; 3] = [ ExecutorKind::Simple, ExecutorKind::SingleThreaded, ExecutorKind::MultiThreaded, ]; + #[derive(Default, Resource)] + struct ExecTracker(Vec); + + /// Regression test: https://github.com/bevyengine/bevy/pull/15606 #[test] - fn invalid_system_param_skips() { + fn skip_on_first_system() { for executor in EXECUTORS { - invalid_system_param_skips_core(executor); + skip_on_first_system_inner(executor); } } - fn invalid_system_param_skips_core(executor: ExecutorKind) { + fn skip_on_first_system_inner(executor: ExecutorKind) { let mut world = World::new(); + world.init_resource::(); let mut schedule = Schedule::default(); 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| { - commands.insert_resource(R2); - }, - ) - .chain(), + (|mut t: ResMut, _: Res| t.0.push(0)) + .pipe(|_: In<()>, mut t: ResMut, _: Res| t.0.push(1)), ); schedule.run(&mut world); - assert!(world.get_resource::().is_none()); - assert!(world.get_resource::().is_some()); + assert_eq!(world.resource::().0, &[]); } - #[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)] - struct S1; + /// Regression test: https://github.com/bevyengine/bevy/pull/15606 + #[test] + fn skip_on_second_system() { + for executor in EXECUTORS { + skip_on_second_system_inner(executor); + } + } + fn skip_on_second_system_inner(executor: ExecutorKind) { + let mut world = World::new(); + world.init_resource::(); + let mut schedule = Schedule::default(); + schedule.set_executor_kind(executor); + schedule.add_systems( + (|mut t: ResMut| t.0.push(0)) + .pipe(|_: In<()>, mut t: ResMut, _: Res| t.0.push(1)), + ); + schedule.run(&mut world); + assert_eq!(world.resource::().0, &[0]); + } + + /// Regression test: https://github.com/bevyengine/bevy/pull/15606 #[test] - fn invalid_condition_param_skips_system() { + fn run_full_combined_system() { for executor in EXECUTORS { - invalid_condition_param_skips_system_core(executor); + run_full_combined_system_inner(executor); } } - fn invalid_condition_param_skips_system_core(executor: ExecutorKind) { + fn run_full_combined_system_inner(executor: ExecutorKind) { let mut world = World::new(); + world.init_resource::(); let mut schedule = Schedule::default(); schedule.set_executor_kind(executor); - schedule.configure_sets(S1.run_if(|_: Res| true)); - schedule.add_systems(( - // System gets skipped if system set run conditions fail validation. - (|mut commands: Commands| { - commands.insert_resource(R1); - }) - .in_set(S1), - // System gets skipped if run conditions fail validation. - (|mut commands: Commands| { - commands.insert_resource(R2); - }) - .run_if(|_: Res| true), - )); + schedule.add_systems( + (|mut t: ResMut| t.0.push(0)) + .pipe(|_: In<()>, mut t: ResMut| t.0.push(1)), + ); schedule.run(&mut world); - assert!(world.get_resource::().is_none()); - assert!(world.get_resource::().is_none()); + assert_eq!(world.resource::().0, &[0, 1]); } } From 3b0cb96d10ace5736ae9129f0b02dde12a67fe00 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 13 Oct 2024 18:12:45 +0200 Subject: [PATCH 18/19] fix links --- crates/bevy_ecs/src/schedule/executor/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index c6260847772e0..f71a1ecec4ef8 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -202,7 +202,7 @@ mod tests { #[derive(Default, Resource)] struct ExecTracker(Vec); - /// Regression test: https://github.com/bevyengine/bevy/pull/15606 + /// Regression test: #[test] fn skip_on_first_system() { for executor in EXECUTORS { @@ -223,7 +223,7 @@ mod tests { assert_eq!(world.resource::().0, &[]); } - /// Regression test: https://github.com/bevyengine/bevy/pull/15606 + /// Regression test: #[test] fn skip_on_second_system() { for executor in EXECUTORS { @@ -244,7 +244,7 @@ mod tests { assert_eq!(world.resource::().0, &[0]); } - /// Regression test: https://github.com/bevyengine/bevy/pull/15606 + /// Regression test: #[test] fn run_full_combined_system() { for executor in EXECUTORS { From 8a25c58745c0f9fdc41526ff272897dbabee3d7f Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 13 Oct 2024 18:24:06 +0200 Subject: [PATCH 19/19] fix ref --- 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 f406dcb673762..0e9560749c68b 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -138,7 +138,7 @@ impl SystemMeta { } } -/// State machine for emitting warnings when [system params are invalid](System::validate_param). +/// State machine for emitting warnings when [system params are invalid](SystemParam::validate_param). #[derive(Clone, Copy)] pub enum ParamWarnPolicy { /// No warning should ever be emitted.