From a4407348c9e93b086f90fd45ea6efd1b965e55f3 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 25 Jan 2021 20:19:34 +0300 Subject: [PATCH 1/2] Replace ChangedRes with associated methods --- crates/bevy_ecs/src/lib.rs | 2 +- .../bevy_ecs/src/resource/resource_query.rs | 65 ++++--- crates/bevy_ecs/src/system/into_system.rs | 169 +++++++++--------- crates/bevy_ecs/src/system/system_param.rs | 50 +----- 4 files changed, 130 insertions(+), 156 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 70be39c666110..83c6950d3d044 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, Res, ResMut, Resource, Resources}, + resource::{FromResources, Local, Res, ResMut, Resource, Resources}, schedule::{Schedule, State, StateStage, SystemStage}, system::{Commands, IntoSystem, Query, System}, Added, Bundle, Changed, Component, Entity, Flags, In, IntoChainSystem, Mut, Mutated, Or, diff --git a/crates/bevy_ecs/src/resource/resource_query.rs b/crates/bevy_ecs/src/resource/resource_query.rs index ef4d560dfd0bb..420442a9c7b46 100644 --- a/crates/bevy_ecs/src/resource/resource_query.rs +++ b/crates/bevy_ecs/src/resource/resource_query.rs @@ -8,26 +8,29 @@ 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 +/// Shared borrow of a Resource #[derive(Debug)] -pub struct ChangedRes<'a, T: Resource> { +pub struct Res<'a, T: Resource> { value: &'a T, + added: bool, + mutated: bool, } -impl<'a, T: Resource> ChangedRes<'a, T> { +impl<'a, T: Resource> Res<'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 { + pub unsafe fn new(value: NonNull, added: bool, changed: bool) -> Self { Self { value: &*value.as_ptr(), + added, + mutated: changed, } } } -impl<'a, T: Resource> Deref for ChangedRes<'a, T> { +impl<'a, T: Resource> Deref for Res<'a, T> { type Target = T; fn deref(&self) -> &T { @@ -35,29 +38,20 @@ impl<'a, T: Resource> Deref for ChangedRes<'a, T> { } } -/// Shared borrow of a Resource -#[derive(Debug)] -pub struct Res<'a, T: Resource> { - value: &'a T, -} - impl<'a, T: Resource> Res<'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(), - } + #[inline(always)] + pub fn added(this: Self) -> bool { + this.added } -} -impl<'a, T: Resource> Deref for Res<'a, T> { - type Target = T; + #[inline(always)] + pub fn mutated(this: Self) -> bool { + this.mutated + } - fn deref(&self) -> &T { - self.value + #[inline(always)] + pub fn changed(this: Self) -> bool { + this.added || this.mutated } } @@ -66,6 +60,7 @@ impl<'a, T: Resource> Deref for Res<'a, T> { 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 5027ca05b6f47..50e8952f8aa69 100644 --- a/crates/bevy_ecs/src/system/into_system.rs +++ b/crates/bevy_ecs/src/system/into_system.rs @@ -333,10 +333,9 @@ impl_into_system!(A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P); mod tests { use super::IntoSystem; use crate::{ - clear_trackers_system, resource::{Res, ResMut, Resources}, schedule::Schedule, - ChangedRes, Entity, Local, Or, Query, QuerySet, System, SystemStage, With, World, + Entity, Local, Query, QuerySet, System, SystemStage, With, World, }; #[derive(Debug, Eq, PartialEq, Default)] @@ -437,82 +436,82 @@ mod tests { assert!(*resources.get::().unwrap(), "system ran"); } - #[test] - fn changed_resource_system() { - fn incr_e_on_flip(_run_on_flip: ChangedRes, 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); - 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.system()), - ); - - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - *resources.get_mut::().unwrap() = true; - schedule.initialize_and_run(&mut world, &mut resources); - 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.system()), - ); - - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 1); - - *resources.get_mut::().unwrap() = true; - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 2); - - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 2); - - *resources.get_mut::().unwrap() = 20; - schedule.initialize_and_run(&mut world, &mut resources); - assert_eq!(*(world.get::(ent).unwrap()), 3); - } + // #[test] + // fn changed_resource_system() { + // fn incr_e_on_flip(_run_on_flip: ChangedRes, 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); + // 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.system()), + // ); + + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 1); + + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 1); + + // *resources.get_mut::().unwrap() = true; + // schedule.initialize_and_run(&mut world, &mut resources); + // 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.system()), + // ); + + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 1); + + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 1); + + // *resources.get_mut::().unwrap() = true; + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 2); + + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 2); + + // *resources.get_mut::().unwrap() = 20; + // schedule.initialize_and_run(&mut world, &mut resources); + // assert_eq!(*(world.get::(ent).unwrap()), 3); + // } #[test] #[should_panic] @@ -619,13 +618,13 @@ 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_changed_and_mutable_resource() { + // // A tempting pattern, but unsound if allowed. + // fn sys(_: ResMut, _: ChangedRes) {} + // test_for_conflicting_resources(sys.system()) + // } #[test] #[should_panic] diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 0b29e91371e0c..18824220b9447 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, Or, Query, QueryAccess, - QueryFilter, QuerySet, QueryTuple, Res, ResMut, Resource, ResourceIndex, Resources, - SystemState, TypeAccess, World, WorldQuery, + ArchetypeComponent, Commands, Fetch, FromResources, Local, 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}; @@ -168,9 +168,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())) } } @@ -199,39 +198,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, @@ -240,11 +206,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)) } } From da6b0713676f50a6b6bda462a3e5981733bc6e7d Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Fri, 12 Feb 2021 17:03:58 +0300 Subject: [PATCH 2/2] cargo fmt --- crates/bevy_ecs/src/system/system_param.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index ef389c5cc1dbd..01ed94a3de4db 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, 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};