From 87ada5b589f910656a383b81d8fed997f338620e Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Wed, 3 Mar 2021 01:59:40 +0000 Subject: [PATCH] Get rid of ChangedRes (#1313) This replaces `ChangedRes` with simple associated methods that return the same info, but don't block execution. Also, since ChangedRes was infectious and was the only reason `FetchSystemParam::get_params` and `System::run_unsafe` returned `Option`s, their implementation could be simplified after this PR is merged, or as part of it with a future commit. --- crates/bevy_ecs/src/lib.rs | 2 +- .../bevy_ecs/src/resource/resource_query.rs | 71 +++++++++++-------- crates/bevy_ecs/src/system/into_system.rs | 62 ++-------------- crates/bevy_ecs/src/system/system_param.rs | 50 ++----------- 4 files changed, 56 insertions(+), 129 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index be73d6c063..d999401fa7 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -13,7 +13,7 @@ pub use system::{Query, *}; pub mod prelude { pub use crate::{ core::WorldBuilderSource, - resource::{ChangedRes, FromResources, Local, NonSend, Res, ResMut, Resource, Resources}, + resource::{FromResources, Local, NonSend, Res, ResMut, Resource, Resources}, schedule::{ ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, ReportExecutionOrderAmbiguities, RunOnce, Schedule, Stage, State, StateStage, diff --git a/crates/bevy_ecs/src/resource/resource_query.rs b/crates/bevy_ecs/src/resource/resource_query.rs index b06c9a75e8..42ca61ea97 100644 --- a/crates/bevy_ecs/src/resource/resource_query.rs +++ b/crates/bevy_ecs/src/resource/resource_query.rs @@ -8,37 +8,12 @@ use std::{ // TODO: align TypeAccess api with Query::Fetch -/// A shared borrow of a Resource -/// that will only return in a query if the Resource has been changed -#[derive(Debug)] -pub struct ChangedRes<'a, T: Resource> { - value: &'a T, -} - -impl<'a, T: Resource> ChangedRes<'a, T> { - /// Creates a reference cell to a Resource from a pointer - /// - /// # Safety - /// The pointer must have correct lifetime / storage - pub unsafe fn new(value: NonNull) -> Self { - Self { - value: &*value.as_ptr(), - } - } -} - -impl<'a, T: Resource> Deref for ChangedRes<'a, T> { - type Target = T; - - fn deref(&self) -> &T { - self.value - } -} - /// Shared borrow of a Resource #[derive(Debug)] pub struct Res<'a, T: Resource> { value: &'a T, + added: bool, + mutated: bool, } impl<'a, T: Resource> Res<'a, T> { @@ -46,9 +21,11 @@ impl<'a, T: Resource> Res<'a, T> { /// /// # Safety /// The pointer must have correct lifetime / storage - pub unsafe fn new(value: NonNull) -> Self { + pub unsafe fn new(value: NonNull, added: bool, changed: bool) -> Self { Self { value: &*value.as_ptr(), + added, + mutated: changed, } } } @@ -61,11 +38,29 @@ impl<'a, T: Resource> Deref for Res<'a, T> { } } +impl<'a, T: Resource> Res<'a, T> { + #[inline(always)] + pub fn added(this: &Self) -> bool { + this.added + } + + #[inline(always)] + pub fn mutated(this: &Self) -> bool { + this.mutated + } + + #[inline(always)] + pub fn changed(this: &Self) -> bool { + this.added || this.mutated + } +} + /// Unique borrow of a Resource #[derive(Debug)] pub struct ResMut<'a, T: Resource> { _marker: PhantomData<&'a T>, value: *mut T, + added: bool, mutated: *mut bool, } @@ -74,10 +69,11 @@ impl<'a, T: Resource> ResMut<'a, T> { /// /// # Safety /// The pointer must have correct lifetime / storage / ownership - pub unsafe fn new(value: NonNull, mutated: NonNull) -> Self { + pub unsafe fn new(value: NonNull, added: bool, mutated: NonNull) -> Self { Self { value: value.as_ptr(), mutated: mutated.as_ptr(), + added, _marker: Default::default(), } } @@ -100,6 +96,23 @@ impl<'a, T: Resource> DerefMut for ResMut<'a, T> { } } +impl<'a, T: Resource> ResMut<'a, T> { + #[inline(always)] + pub fn added(this: Self) -> bool { + this.added + } + + #[inline(always)] + pub fn mutated(this: Self) -> bool { + unsafe { *this.mutated } + } + + #[inline(always)] + pub fn changed(this: Self) -> bool { + this.added || Self::mutated(this) + } +} + /// Local resources are unique per-system. Two instances of the same system will each have their own resource. /// Local resources are automatically initialized using the FromResources trait. #[derive(Debug)] diff --git a/crates/bevy_ecs/src/system/into_system.rs b/crates/bevy_ecs/src/system/into_system.rs index a97903e8f0..ff7b95550c 100644 --- a/crates/bevy_ecs/src/system/into_system.rs +++ b/crates/bevy_ecs/src/system/into_system.rs @@ -340,8 +340,8 @@ mod tests { clear_trackers_system, resource::{Res, ResMut, Resources}, schedule::Schedule, - ChangedRes, Entity, IntoExclusiveSystem, Local, Or, Query, QuerySet, Stage, System, - SystemStage, With, World, + Entity, IntoExclusiveSystem, Local, Query, QuerySet, Stage, System, SystemStage, With, + World, }; #[derive(Debug, Eq, PartialEq, Default)] @@ -444,7 +444,11 @@ mod tests { #[test] fn changed_resource_system() { - fn incr_e_on_flip(_run_on_flip: ChangedRes, mut query: Query<&mut i32>) { + fn incr_e_on_flip(run_on_flip: Res, mut query: Query<&mut i32>) { + if !Res::changed(&run_on_flip) { + return; + } + for mut i in query.iter_mut() { *i += 1; } @@ -475,50 +479,6 @@ mod tests { assert_eq!(*(world.get::(ent).unwrap()), 2); } - #[test] - fn changed_resource_or_system() { - fn incr_e_on_flip( - _or: Or<(Option>, Option>)>, - mut query: Query<&mut i32>, - ) { - for mut i in query.iter_mut() { - *i += 1; - } - } - - let mut world = World::default(); - let mut resources = Resources::default(); - resources.insert(false); - resources.insert::(10); - let ent = world.spawn((0,)); - - let mut schedule = Schedule::default(); - let mut update = SystemStage::parallel(); - update.add_system(incr_e_on_flip.system()); - schedule.add_stage("update", update); - schedule.add_stage( - "clear_trackers", - SystemStage::single(clear_trackers_system.exclusive_system()), - ); - - schedule.run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - schedule.run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - *resources.get_mut::().unwrap() = true; - schedule.run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 2); - - schedule.run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 2); - - *resources.get_mut::().unwrap() = 20; - schedule.run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 3); - } - #[test] #[should_panic] fn conflicting_query_mut_system() { @@ -624,14 +584,6 @@ mod tests { test_for_conflicting_resources(sys.system()) } - #[test] - #[should_panic] - fn conflicting_changed_and_mutable_resource() { - // A tempting pattern, but unsound if allowed. - fn sys(_: ResMut, _: ChangedRes) {} - test_for_conflicting_resources(sys.system()) - } - #[test] #[should_panic] fn conflicting_system_local_resources() { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 603699be1b..01ed94a3de 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1,7 +1,7 @@ use crate::{ - ArchetypeComponent, ChangedRes, Commands, Fetch, FromResources, Local, NonSend, Or, Query, - QueryAccess, QueryFilter, QuerySet, QueryTuple, Res, ResMut, Resource, ResourceIndex, - Resources, SystemState, TypeAccess, World, WorldQuery, + ArchetypeComponent, Commands, Fetch, FromResources, Local, NonSend, Or, Query, QueryAccess, + QueryFilter, QuerySet, QueryTuple, Res, ResMut, Resource, ResourceIndex, Resources, + SystemState, TypeAccess, World, WorldQuery, }; use parking_lot::Mutex; use std::{any::TypeId, marker::PhantomData, sync::Arc}; @@ -173,9 +173,8 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchRes { _world: &'a World, resources: &'a Resources, ) -> Option { - Some(Res::new( - resources.get_unsafe_ref::(ResourceIndex::Global), - )) + let result = resources.get_unsafe_ref_with_added_and_mutated::(ResourceIndex::Global); + Some(Res::new(result.0, *result.1.as_ptr(), *result.2.as_ptr())) } } @@ -204,39 +203,6 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchResMut { system_state.resource_access.add_write(TypeId::of::()); } - #[inline] - unsafe fn get_param( - _system_state: &'a SystemState, - _world: &'a World, - resources: &'a Resources, - ) -> Option { - let (value, _added, mutated) = - resources.get_unsafe_ref_with_added_and_mutated::(ResourceIndex::Global); - Some(ResMut::new(value, mutated)) - } -} - -pub struct FetchChangedRes(PhantomData); - -impl<'a, T: Resource> SystemParam for ChangedRes<'a, T> { - type Fetch = FetchChangedRes; -} - -impl<'a, T: Resource> FetchSystemParam<'a> for FetchChangedRes { - type Item = ChangedRes<'a, T>; - - fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) { - if system_state.resource_access.is_write(&TypeId::of::()) { - panic!( - "System `{}` has a `ChangedRes<{res}>` parameter that conflicts with \ - another parameter with mutable access to the same `{res}` resource.", - system_state.name, - res = std::any::type_name::() - ); - } - system_state.resource_access.add_read(TypeId::of::()); - } - #[inline] unsafe fn get_param( _system_state: &'a SystemState, @@ -245,11 +211,7 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchChangedRes { ) -> Option { let (value, added, mutated) = resources.get_unsafe_ref_with_added_and_mutated::(ResourceIndex::Global); - if *added.as_ptr() || *mutated.as_ptr() { - Some(ChangedRes::new(value)) - } else { - None - } + Some(ResMut::new(value, *added.as_ptr(), mutated)) } }