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

@@ -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,

crates/bevy_ecs/src/resource/resource_query.rs

@@ -8,37 +8,41 @@ 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,
 }
 
-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<T>) -> Self {
+pub struct ResFlags<'a, T: Resource> {
+    added: bool,
+    mutated: bool,
+    _marker: PhantomData<&'a T>,
+}
+
+impl<'a, T: Resource> ResFlags<'a, T> {
+    pub fn new(added: bool, mutated: bool) -> Self {
         Self {
-            value: &*value.as_ptr(),
+            added,
+            mutated,
+            _marker: Default::default(),
         }
     }
-}
 
-impl<'a, T: Resource> Deref for ChangedRes<'a, T> {
-    type Target = T;
+    #[inline(always)]
+    pub fn added(&self) -> bool {
+        self.added
+    }
 
-    fn deref(&self) -> &T {
-        self.value
+    #[inline(always)]
+    pub fn mutated(&self) -> bool {
+        self.mutated
     }
-}
 
-/// Shared borrow of a Resource
-#[derive(Debug)]
-pub struct Res<'a, T: Resource> {
-    value: &'a T,
+    #[inline(always)]
+    pub fn changed(&self) -> bool {
+        self.added || self.mutated
+    }
 }
 
 impl<'a, T: Resource> Res<'a, T> {
@@ -66,6 +70,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 +79,11 @@ impl<'a, T: Resource> ResMut<'a, T> {
     ///
     /// # Safety
     /// The pointer must have correct lifetime / storage / ownership
-    pub unsafe fn new(value: NonNull<T>, mutated: NonNull<bool>) -> Self {
+    pub unsafe fn new(value: NonNull<T>, added: bool, mutated: NonNull<bool>) -> Self {
         Self {
             value: value.as_ptr(),
             mutated: mutated.as_ptr(),
+            added,
             _marker: Default::default(),
         }
     }
@@ -100,6 +106,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<T> 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)]

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, ResFlags, Stage, System, SystemStage,
+        With, World,
     };
 
     #[derive(Debug, Eq, PartialEq, Default)]
@@ -444,43 +444,11 @@ mod tests {
 
     #[test]
     fn changed_resource_system() {
-        fn incr_e_on_flip(_run_on_flip: ChangedRes<bool>, mut query: Query<&mut i32>) {
-            for mut i in query.iter_mut() {
-                *i += 1;
+        fn incr_e_on_flip(run_on_flip: ResFlags<bool>, mut query: Query<&mut i32>) {
+            if !run_on_flip.changed() {
+                return;
             }
-        }
-
-        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.exclusive_system()),
-        );
-
-        schedule.run(&mut world, &mut resources);
-        assert_eq!(*(world.get::<i32>(ent).unwrap()), 1);
 
-        schedule.run(&mut world, &mut resources);
-        assert_eq!(*(world.get::<i32>(ent).unwrap()), 1);
-
-        *resources.get_mut::<bool>().unwrap() = true;
-        schedule.run(&mut world, &mut resources);
-        assert_eq!(*(world.get::<i32>(ent).unwrap()), 2);
-    }
-
-    #[test]
-    fn changed_resource_or_system() {
-        fn incr_e_on_flip(
-            _or: Or<(Option<ChangedRes<bool>>, Option<ChangedRes<i32>>)>,
-            mut query: Query<&mut i32>,
-        ) {
             for mut i in query.iter_mut() {
                 *i += 1;
             }
@@ -489,7 +457,6 @@ mod tests {
         let mut world = World::default();
         let mut resources = Resources::default();
         resources.insert(false);
-        resources.insert::<i32>(10);
         let ent = world.spawn((0,));
 
         let mut schedule = Schedule::default();
@@ -510,13 +477,6 @@ mod tests {
         *resources.get_mut::<bool>().unwrap() = true;
         schedule.run(&mut world, &mut resources);
         assert_eq!(*(world.get::<i32>(ent).unwrap()), 2);
-
-        schedule.run(&mut world, &mut resources);
-        assert_eq!(*(world.get::<i32>(ent).unwrap()), 2);
-
-        *resources.get_mut::<i32>().unwrap() = 20;
-        schedule.run(&mut world, &mut resources);
-        assert_eq!(*(world.get::<i32>(ent).unwrap()), 3);
     }
 
     #[test]
@@ -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<BufferRes>, _: ChangedRes<BufferRes>) {}
-        test_for_conflicting_resources(sys.system())
-    }
-
     #[test]
     #[should_panic]
     fn conflicting_system_local_resources() {

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, ResFlags, ResMut, Resource, ResourceIndex, Resources,
+    SystemState, TypeAccess, World, WorldQuery,
 };
 use parking_lot::Mutex;
 use std::{any::TypeId, marker::PhantomData, sync::Arc};
@@ -179,29 +179,25 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchRes<T> {
     }
 }
 
-pub struct FetchResMut<T>(PhantomData<T>);
+pub struct FetchResFlags<T>(PhantomData<T>);
 
-impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
-    type Fetch = FetchResMut<T>;
+impl<'a, T: Resource> SystemParam for ResFlags<'a, T> {
+    type Fetch = FetchResFlags<T>;
 }
-impl<'a, T: Resource> FetchSystemParam<'a> for FetchResMut<T> {
-    type Item = ResMut<'a, T>;
+
+impl<'a, T: Resource> FetchSystemParam<'a> for FetchResFlags<T> {
+    type Item = ResFlags<'a, T>;
 
     fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) {
-        // If a system already has access to the resource in another parameter, then we fail early.
-        // e.g. `fn(Res<Foo>, ResMut<Foo>)` or `fn(ResMut<Foo>, ResMut<Foo>)` must not be allowed.
-        if system_state
-            .resource_access
-            .is_read_or_write(&TypeId::of::<T>())
-        {
+        if system_state.resource_access.is_write(&TypeId::of::<T>()) {
             panic!(
-                "System `{}` has a `ResMut<{res}>` parameter that conflicts with \
-                another parameter to the same `{res}` resource. `ResMut` must have unique access.",
+                "System `{}` has a `Res<{res}>` parameter that conflicts with \
+                another parameter with mutable access to the same `{res}` resource.",
                 system_state.name,
                 res = std::any::type_name::<T>()
             );
         }
-        system_state.resource_access.add_write(TypeId::of::<T>());
+        system_state.resource_access.add_read(TypeId::of::<T>());
     }
 
     #[inline]
@@ -210,31 +206,35 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchResMut<T> {
         _world: &'a World,
         resources: &'a Resources,
     ) -> Option<Self::Item> {
-        let (value, _added, mutated) =
+        let (_, added, mutated) =
             resources.get_unsafe_ref_with_added_and_mutated::<T>(ResourceIndex::Global);
-        Some(ResMut::new(value, mutated))
+        Some(ResFlags::new(*added.as_ptr(), *mutated.as_ptr()))
     }
 }
 
-pub struct FetchChangedRes<T>(PhantomData<T>);
+pub struct FetchResMut<T>(PhantomData<T>);
 
-impl<'a, T: Resource> SystemParam for ChangedRes<'a, T> {
-    type Fetch = FetchChangedRes<T>;
+impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
+    type Fetch = FetchResMut<T>;
 }
-
-impl<'a, T: Resource> FetchSystemParam<'a> for FetchChangedRes<T> {
-    type Item = ChangedRes<'a, T>;
+impl<'a, T: Resource> FetchSystemParam<'a> for FetchResMut<T> {
+    type Item = ResMut<'a, T>;
 
     fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) {
-        if system_state.resource_access.is_write(&TypeId::of::<T>()) {
+        // If a system already has access to the resource in another parameter, then we fail early.
+        // e.g. `fn(Res<Foo>, ResMut<Foo>)` or `fn(ResMut<Foo>, ResMut<Foo>)` must not be allowed.
+        if system_state
+            .resource_access
+            .is_read_or_write(&TypeId::of::<T>())
+        {
             panic!(
-                "System `{}` has a `ChangedRes<{res}>` parameter that conflicts with \
-                another parameter with mutable access to the same `{res}` resource.",
+                "System `{}` has a `ResMut<{res}>` parameter that conflicts with \
+                another parameter to the same `{res}` resource. `ResMut` must have unique access.",
                 system_state.name,
                 res = std::any::type_name::<T>()
             );
         }
-        system_state.resource_access.add_read(TypeId::of::<T>());
+        system_state.resource_access.add_write(TypeId::of::<T>());
     }
 
     #[inline]
@@ -245,11 +245,7 @@ impl<'a, T: Resource> FetchSystemParam<'a> for FetchChangedRes<T> {
     ) -> Option<Self::Item> {
         let (value, added, mutated) =
             resources.get_unsafe_ref_with_added_and_mutated::<T>(ResourceIndex::Global);
-        if *added.as_ptr() || *mutated.as_ptr() {
-            Some(ChangedRes::new(value))
-        } else {
-            None
-        }
+        Some(ResMut::new(value, *added.as_ptr(), mutated))
     }
 }