From fbcf3f89d04ca11fdea9376a841e9ef81d1f6465 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sat, 18 Jul 2020 01:05:06 -0700 Subject: [PATCH] ecs: rename ComMut to Track and fix nested change queries --- crates/bevy_ecs/hecs/src/query.rs | 18 +++++- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_ecs/src/world/component.rs | 84 ++++++++++++++++++-------- 3 files changed, 77 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/hecs/src/query.rs b/crates/bevy_ecs/hecs/src/query.rs index 551c55edf1..83cee2ef81 100644 --- a/crates/bevy_ecs/hecs/src/query.rs +++ b/crates/bevy_ecs/hecs/src/query.rs @@ -203,6 +203,10 @@ impl<'a, T: Fetch<'a>> Fetch<'a> for TryFetch { unsafe fn next(&mut self) -> Option { Some(self.0.as_mut()?.next()) } + + unsafe fn should_skip(&self) -> bool { + self.0.as_ref().map_or(false, |fetch| fetch.should_skip()) + } } /// Query transformer skipping entities that have a `T` component @@ -258,6 +262,10 @@ impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchWithout { unsafe fn next(&mut self) -> F::Item { self.0.next() } + + unsafe fn should_skip(&self) -> bool { + self.0.should_skip() + } } /// Query transformer skipping entities that do not have a `T` component @@ -315,6 +323,10 @@ impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchWith { unsafe fn next(&mut self) -> F::Item { self.0.next() } + + unsafe fn should_skip(&self) -> bool { + self.0.should_skip() + } } /// A borrow of a `World` sufficient to execute the query `Q` @@ -536,7 +548,7 @@ impl ChunkIter { continue; } - break Some(self.fetch.next()) + break Some(self.fetch.next()); } } } @@ -636,6 +648,10 @@ macro_rules! tuple_impl { let ($($name,)*) = self; ($($name.next(),)*) } + + unsafe fn should_skip(&self) -> bool { + false + } } impl<$($name: Query),*> Query for ($($name,)*) { diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index ee4fdf05cb..4d5a0d689b 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -15,7 +15,7 @@ pub mod prelude { system::{ Commands, IntoForEachSystem, IntoQuerySystem, IntoThreadLocalSystem, Query, System, }, - world::{WorldBuilderSource, ComMut}, + world::{WorldBuilderSource, Track}, Bundle, Component, Entity, Ref, RefMut, With, Without, World, }; } diff --git a/crates/bevy_ecs/src/world/component.rs b/crates/bevy_ecs/src/world/component.rs index 10ec4110f5..3d8a38c507 100644 --- a/crates/bevy_ecs/src/world/component.rs +++ b/crates/bevy_ecs/src/world/component.rs @@ -7,36 +7,36 @@ use std::{ }; /// Unique borrow of an entity's component -pub struct ComMut<'a, T: Component> { +pub struct Track<'a, T: Component> { value: &'a mut T, modified: &'a mut bool, } -unsafe impl Send for ComMut<'_, T> {} -unsafe impl Sync for ComMut<'_, T> {} +unsafe impl Send for Track<'_, T> {} +unsafe impl Sync for Track<'_, T> {} -impl<'a, T: Component> Deref for ComMut<'a, T> { +impl<'a, T: Component> Deref for Track<'a, T> { type Target = T; fn deref(&self) -> &T { self.value } } -impl<'a, T: Component> DerefMut for ComMut<'a, T> { +impl<'a, T: Component> DerefMut for Track<'a, T> { fn deref_mut(&mut self) -> &mut T { *self.modified = true; self.value } } -impl<'a, T: Component> HecsQuery for ComMut<'a, T> { - type Fetch = FetchComMut; +impl<'a, T: Component> HecsQuery for Track<'a, T> { + type Fetch = FetchTrack; } #[doc(hidden)] -pub struct FetchComMut(NonNull, NonNull); +pub struct FetchTrack(NonNull, NonNull); -impl<'a, T: Component> Fetch<'a> for FetchComMut { - type Item = ComMut<'a, T>; +impl<'a, T: Component> Fetch<'a> for FetchTrack { + type Item = Track<'a, T>; fn access(archetype: &Archetype) -> Option { if archetype.has::() { @@ -63,12 +63,12 @@ impl<'a, T: Component> Fetch<'a> for FetchComMut { archetype.release_mut::(); } - unsafe fn next(&mut self) -> ComMut<'a, T> { + unsafe fn next(&mut self) -> Track<'a, T> { let component = self.0.as_ptr(); let modified = self.1.as_ptr(); self.0 = NonNull::new_unchecked(component.add(1)); self.1 = NonNull::new_unchecked(modified.add(1)); - ComMut { + Track { value: &mut *component, modified: &mut *modified, } @@ -114,7 +114,7 @@ impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchChanged { unsafe fn should_skip(&self) -> bool { // skip if the current item wasn't changed - !*self.2.as_ref() + !*self.2.as_ref() || self.0.should_skip() } unsafe fn next(&mut self) -> F::Item { @@ -125,52 +125,64 @@ impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchChanged { #[cfg(test)] mod tests { - use crate::{Changed, ComMut}; + use crate::{Changed, Track}; use hecs::{Entity, World}; struct A(usize); - struct B; + struct B(usize); struct C; #[test] fn modified_trackers() { let mut world = World::default(); - let e1 = world.spawn((A(0), B)); - let e2 = world.spawn((A(0), B)); - let e3 = world.spawn((A(0), B)); + let e1 = world.spawn((A(0), B(0))); + let e2 = world.spawn((A(0), B(0))); + let e3 = world.spawn((A(0), B(0))); world.spawn((A(0), B)); - for (i, mut a) in world.query::>().iter().enumerate() { + for (i, mut a) in world.query::>().iter().enumerate() { if i % 2 == 0 { a.0 += 1; } } - fn get_changed(world: &World) -> Vec { + fn get_changed_a(world: &World) -> Vec { world .query::>() .iter() .collect::>() }; - assert_eq!(get_changed(&world), vec![e1, e3]); + assert_eq!(get_changed_a(&world), vec![e1, e3]); // ensure changing an entity's archetypes also moves its modified state world.insert(e1, (C,)).unwrap(); - assert_eq!(get_changed(&world), vec![e3, e1], "changed entities list should not change (although the order will due to archetype moves)"); + assert_eq!(get_changed_a(&world), vec![e3, e1], "changed entities list should not change (although the order will due to archetype moves)"); // spawning a new A entity should not change existing modified state world.insert(e1, (A(0), B)).unwrap(); - assert_eq!(get_changed(&world), vec![e3, e1], "changed entities list should not change"); + assert_eq!( + get_changed_a(&world), + vec![e3, e1], + "changed entities list should not change" + ); // removing an unchanged entity should not change modified state world.despawn(e2).unwrap(); - assert_eq!(get_changed(&world), vec![e3, e1], "changed entities list should not change"); + assert_eq!( + get_changed_a(&world), + vec![e3, e1], + "changed entities list should not change" + ); // removing a changed entity should remove it from enumeration world.despawn(e1).unwrap(); - assert_eq!(get_changed(&world), vec![e3], "e1 should no longer be returned"); + assert_eq!( + get_changed_a(&world), + vec![e3], + "e1 should no longer be returned" + ); world.clear_trackers(); @@ -180,4 +192,26 @@ mod tests { .collect::>() .is_empty()); } + + #[test] + fn nested_changed_query() { + let mut world = World::default(); + world.spawn((A(0), B(0))); + let e2 = world.spawn((A(0), B(0))); + world.spawn((A(0), B(0))); + + for mut a in world.query::>().iter() { + a.0 += 1; + } + + for mut b in world.query::>().iter().skip(1).take(1) { + b.0 += 1; + } + + let a_b_changed = world + .query::>>() + .iter() + .collect::>(); + assert_eq!(a_b_changed, vec![e2]); + } }