Repository: Ralith/hecs
Pull request #12 (2019-12-16 03:42:34)

Fix disjoint queries, strip out useless scope accessors
These didn't account for queries that were safe on the same archetype.


Patch:
diff --git a/src/query.rs b/src/query.rs
index 971bbbe..60a5481 100644
--- a/src/query.rs
+++ b/src/query.rs
@@ -29,16 +29,17 @@ pub trait Query {
 pub trait Fetch<'a>: Sized {
     /// Type of value to be fetched
     type Item;
+
     /// Whether `get` will borrow from `archetype`
     fn wants(archetype: &Archetype) -> bool;
-    /// Whether `get` will uniquely borrow from `archetype`. Implies `wants`.
-    fn wants_mut(archetype: &Archetype) -> bool;
+
     /// Acquire dynamic borrows from `archetype`
     fn borrow(archetype: &Archetype);
     /// Construct a `Fetch` for `archetype` if it should be traversed
     fn get(archetype: &'a Archetype) -> Option<Self>;
     /// Release dynamic borrows acquired by `borrow`
     fn release(archetype: &Archetype);
+
     /// Access the next item in this archetype without bounds checking
     unsafe fn next(&mut self) -> Self::Item;
 }
@@ -55,9 +56,7 @@ impl<'a, T: Component> Fetch<'a> for FetchRead<T> {
     fn wants(archetype: &Archetype) -> bool {
         archetype.has::<T>()
     }
-    fn wants_mut(_: &Archetype) -> bool {
-        false
-    }
+
     fn borrow(archetype: &Archetype) {
         archetype.borrow::<T>();
     }
@@ -67,6 +66,7 @@ impl<'a, T: Component> Fetch<'a> for FetchRead<T> {
     fn release(archetype: &Archetype) {
         archetype.release::<T>();
     }
+
     unsafe fn next(&mut self) -> &'a T {
         let x = self.0.as_ptr();
         self.0 = NonNull::new_unchecked(x.add(1));
@@ -86,9 +86,7 @@ impl<'a, T: Component> Fetch<'a> for FetchWrite<T> {
     fn wants(archetype: &Archetype) -> bool {
         archetype.has::<T>()
     }
-    fn wants_mut(archetype: &Archetype) -> bool {
-        archetype.has::<T>()
-    }
+
     fn borrow(archetype: &Archetype) {
         archetype.borrow_mut::<T>();
     }
@@ -98,6 +96,7 @@ impl<'a, T: Component> Fetch<'a> for FetchWrite<T> {
     fn release(archetype: &Archetype) {
         archetype.release_mut::<T>();
     }
+
     unsafe fn next(&mut self) -> &'a mut T {
         let x = self.0.as_ptr();
         self.0 = NonNull::new_unchecked(x.add(1));
@@ -114,12 +113,10 @@ pub struct TryFetch<T>(Option<T>);
 
 impl<'a, T: Fetch<'a>> Fetch<'a> for TryFetch<T> {
     type Item = Option<T::Item>;
-    fn wants(archetype: &Archetype) -> bool {
-        T::wants(archetype)
-    }
-    fn wants_mut(archetype: &Archetype) -> bool {
-        T::wants_mut(archetype)
+    fn wants(_: &Archetype) -> bool {
+        true
     }
+
     fn borrow(archetype: &Archetype) {
         T::borrow(archetype)
     }
@@ -129,6 +126,7 @@ impl<'a, T: Fetch<'a>> Fetch<'a> for TryFetch<T> {
     fn release(archetype: &Archetype) {
         T::release(archetype)
     }
+
     unsafe fn next(&mut self) -> Option<T::Item> {
         Some(self.0.as_mut()?.next())
     }
@@ -145,7 +143,9 @@ impl<'w, Q: Query> QueryBorrow<'w, Q> {
     pub(crate) fn new(meta: &'w [EntityMeta], archetypes: &'w [Archetype]) -> Self {
         for x in archetypes {
             // TODO: Release prior borrows on failure?
-            Q::Fetch::borrow(x);
+            if Q::Fetch::wants(x) {
+                Q::Fetch::borrow(x);
+            }
         }
         Self {
             meta,
@@ -167,7 +167,9 @@ impl<'w, Q: Query> QueryBorrow<'w, Q> {
 impl<'w, Q: Query> Drop for QueryBorrow<'w, Q> {
     fn drop(&mut self) {
         for x in self.archetypes {
-            Q::Fetch::release(x);
+            if Q::Fetch::wants(x) {
+                Q::Fetch::release(x);
+            }
         }
     }
 }
@@ -239,10 +241,7 @@ macro_rules! tuple_impl {
             fn wants(archetype: &Archetype) -> bool {
                 $($name::wants(archetype) &&)* true
             }
-            #[allow(unused_variables)]
-            fn wants_mut(archetype: &Archetype) -> bool {
-                $($name::wants_mut(archetype) &&)* true
-            }
+
             #[allow(unused_variables)]
             fn borrow(archetype: &Archetype) {
                 $($name::borrow(archetype);)*
@@ -255,6 +254,7 @@ macro_rules! tuple_impl {
             fn release(archetype: &Archetype) {
                 $($name::release(archetype);)*
             }
+
             unsafe fn next(&mut self) -> Self::Item {
                 #[allow(non_snake_case)]
                 let ($($name,)*) = self;
diff --git a/src/world.rs b/src/world.rs
index 8fb8001..7715b02 100644
--- a/src/world.rs
+++ b/src/world.rs
@@ -353,24 +353,6 @@ impl World {
     pub fn remove_one<T: Component>(&mut self, entity: Entity) -> Result<T, ComponentError> {
         self.remove::<(T,)>(entity).map(|(x,)| x)
     }
-
-    // /// Determine which collections of entities will be borrowed by `Q`
-    // ///
-    // /// The identifiers returned by this are invalidated when any entity or component is added or
-    // /// removed.
-    // pub fn query_scope<'a, Q: Query<'a>>(&self) -> impl Iterator<Item = u32> + '_ {
-    //     (0..self.archetypes.len() as u32)
-    //         .filter(move |&i| Q::Fetch::wants(&self.archetypes[i as usize]))
-    // }
-
-    // /// Determine which collections of entities will be uniquely borrowed by `Q`
-    // ///
-    // /// The identifiers returned by this are invalidated when any entity or component is added or
-    // /// removed.
-    // pub fn query_scope_mut<'a, Q: Query<'a>>(&self) -> impl Iterator<Item = u32> + '_ {
-    //     (0..self.archetypes.len() as u32)
-    //         .filter(move |&i| Q::Fetch::wants_mut(&self.archetypes[i as usize]))
-    // }
 }
 
 unsafe impl Sync for World {}


Test patch:
diff --git a/tests/tests.rs b/tests/tests.rs
index 526b9d6..001f9d0 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -180,6 +180,16 @@ fn illegal_borrow_2() {
     world.query::<(&mut i32, &mut i32)>();
 }
 
+#[test]
+fn disjoint_queries() {
+    let mut world = World::new();
+    world.spawn(("abc", true));
+    world.spawn(("def", 456));
+
+    let _a = world.query::<(&mut &str, &bool)>();
+    let _b = world.query::<(&mut &str, &i32)>();
+}
+
 #[test]
 fn shared_borrow() {
     let mut world = World::new();