From d9ae46d5779f85703ff04a93f80feef9bd261b56 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 14:49:55 -0600 Subject: [PATCH 01/15] [ecs] implement is_empty for queries --- crates/bevy_ecs/src/system/query.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 908651125ac2b..55731f3d372ca 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -543,6 +543,17 @@ where >())), } } + + /// Returns true if this query contains no elements. + pub fn is_empty(&self) -> bool { + // SAFE: system runs without conflicts with other systems. + // same-system queries have runtime borrow checks when they conflict + let iter = unsafe { + self.state + .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) + }; + iter.peekable().peek().is_none() + } } /// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`] From cea38cd44107e27b8f657076a5ea8fea9a72f720 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 15:07:11 -0600 Subject: [PATCH 02/15] add test --- crates/bevy_ecs/src/system/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index b10ec03c6506e..84b6fb175ed02 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -438,6 +438,18 @@ mod tests { assert_eq!(conflicts, vec![b_id, d_id]); } + #[test] + fn query_is_empty() { + fn sys(query: Query<&A>) { + assert!(query.is_empty()); + } + + let mut world = World::default(); + let mut sys = sys.system(); + sys.initialize(&mut world); + sys.run((), &mut world); + } + #[test] #[allow(clippy::too_many_arguments)] fn can_have_16_parameters() { From 52424b22aebb541ca275e395675fd9202bb15c98 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 15:17:29 -0600 Subject: [PATCH 03/15] remove peekable --- crates/bevy_ecs/src/system/query.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 55731f3d372ca..ca546351435cd 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -548,11 +548,11 @@ where pub fn is_empty(&self) -> bool { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict - let iter = unsafe { + let mut iter = unsafe { self.state .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) }; - iter.peekable().peek().is_none() + iter.next().is_none() } } From d3ae8c8b309cd28f95c26459c58b401b62d5933a Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 15:18:48 -0600 Subject: [PATCH 04/15] make inline --- crates/bevy_ecs/src/system/query.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index ca546351435cd..2b7d8be361a8e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -545,6 +545,7 @@ where } /// Returns true if this query contains no elements. + #[inline] pub fn is_empty(&self) -> bool { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict From 19463a37220e6b407d010b8ef3229977fed50a73 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 15:26:38 -0600 Subject: [PATCH 05/15] add sad path test --- crates/bevy_ecs/src/system/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 84b6fb175ed02..ca36e7f826cd5 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -440,11 +440,14 @@ mod tests { #[test] fn query_is_empty() { - fn sys(query: Query<&A>) { - assert!(query.is_empty()); + fn sys(empty: Query<&A>, not_empty: Query<&B>) { + assert!(empty.is_empty()); + assert!(!not_empty.is_empty()); } let mut world = World::default(); + world.spawn().insert(B); + let mut sys = sys.system(); sys.initialize(&mut world); sys.run((), &mut world); From ef425658e78f2513b3914c76a9f1c6c5cd1bf173 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:20:16 -0600 Subject: [PATCH 06/15] rework is_empty to hopefully be sound --- crates/bevy_ecs/src/query/iter.rs | 61 +++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/state.rs | 6 +++ crates/bevy_ecs/src/system/mod.rs | 19 ++++++--- crates/bevy_ecs/src/system/query.rs | 9 +---- 4 files changed, 83 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 497ad579fa081..467b59f73f833 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -69,6 +69,67 @@ where current_index: 0, } } + + #[inline(always)] + #[allow(clippy::wrong_self_convention)] + pub(crate) fn is_empty(mut self) -> bool { + // NOTE: this mimics the behavior of `QueryIter::next()` + unsafe { + if self.is_dense { + loop { + if self.current_index == self.current_len { + let table_id = match self.table_id_iter.next() { + Some(table_id) => table_id, + None => return true, + }; + let table = &self.tables[*table_id]; + self.fetch.set_table(&self.query_state.fetch_state, table); + self.filter.set_table(&self.query_state.filter_state, table); + self.current_len = table.len(); + self.current_index = 0; + continue; + } + + if !self.filter.table_filter_fetch(self.current_index) { + self.current_index += 1; + continue; + } + + return false; + } + } else { + loop { + if self.current_index == self.current_len { + let archetype_id = match self.archetype_id_iter.next() { + Some(archetype_id) => archetype_id, + None => return true, + }; + let archetype = &self.archetypes[*archetype_id]; + self.fetch.set_archetype( + &self.query_state.fetch_state, + archetype, + self.tables, + ); + self.filter.set_archetype( + &self.query_state.filter_state, + archetype, + self.tables, + ); + self.current_len = archetype.len(); + self.current_index = 0; + continue; + } + + if !self.filter.archetype_filter_fetch(self.current_index) { + self.current_index += 1; + continue; + } + + return false; + } + } + } + } } impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F> diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index d8a12f3a7b2a5..664a6785435fb 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -68,6 +68,12 @@ where state } + #[inline] + pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { + let iter = unsafe { self.iter_unchecked_manual(world, last_change_tick, change_tick) }; + iter.is_empty() + } + pub fn validate_world_and_update_archetypes(&mut self, world: &World) { if world.id() != self.world_id { panic!("Attempted to use {} with a mismatched World. QueryStates can only be used with the World they were created from.", diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index ca36e7f826cd5..2810c316f7e91 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -440,17 +440,26 @@ mod tests { #[test] fn query_is_empty() { - fn sys(empty: Query<&A>, not_empty: Query<&B>) { + fn without_filter(not_empty: Query<&A>, empty: Query<&B>) { + assert!(!not_empty.is_empty()); assert!(empty.is_empty()); + } + + fn with_filter(not_empty: Query<&A, With>, empty: Query<&A, With>) { assert!(!not_empty.is_empty()); + assert!(empty.is_empty()); } let mut world = World::default(); - world.spawn().insert(B); + world.spawn().insert(A).insert(C); + + let mut without_filter = without_filter.system(); + without_filter.initialize(&mut world); + without_filter.run((), &mut world); - let mut sys = sys.system(); - sys.initialize(&mut world); - sys.run((), &mut world); + let mut with_filter = with_filter.system(); + with_filter.initialize(&mut world); + with_filter.run((), &mut world); } #[test] diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 2b7d8be361a8e..c19ab54147386 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -547,13 +547,8 @@ where /// Returns true if this query contains no elements. #[inline] pub fn is_empty(&self) -> bool { - // SAFE: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict - let mut iter = unsafe { - self.state - .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) - }; - iter.next().is_none() + self.state + .is_empty(self.world, self.last_change_tick, self.change_tick) } } From 17439c190dcdab091c7590ff5447c96f75b7163f Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:29:21 -0600 Subject: [PATCH 07/15] add some safety docs --- crates/bevy_ecs/src/query/iter.rs | 3 ++- crates/bevy_ecs/src/query/state.rs | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 467b59f73f833..0ae7e0274541a 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -73,7 +73,8 @@ where #[inline(always)] #[allow(clippy::wrong_self_convention)] pub(crate) fn is_empty(mut self) -> bool { - // NOTE: this mimics the behavior of `QueryIter::next()` + // NOTE: this mimics the behavior of `QueryIter::next()`, expect that it + // never gets a `Self::Item`. unsafe { if self.is_dense { loop { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 664a6785435fb..699f784bd8707 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -70,8 +70,12 @@ where #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - let iter = unsafe { self.iter_unchecked_manual(world, last_change_tick, change_tick) }; - iter.is_empty() + // SAFE: the iterator is instantly consumed via `is_empty` and the implementation of + // `QueryIter::is_empty` never creates any references (mutable or immutable) to the `Item`. + unsafe { + self.iter_unchecked_manual(world, last_change_tick, change_tick) + .is_empty() + } } pub fn validate_world_and_update_archetypes(&mut self, world: &World) { From fd4dde492956aa3452ab9948b10afdceb26ff950 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:38:08 -0600 Subject: [PATCH 08/15] rename QueryIter function to any_remaining --- crates/bevy_ecs/src/query/iter.rs | 3 ++- crates/bevy_ecs/src/query/state.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 0ae7e0274541a..8c4d69c7271c3 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -70,9 +70,10 @@ where } } + /// Consumes `self` and returns true is there were any elements left to yield. #[inline(always)] #[allow(clippy::wrong_self_convention)] - pub(crate) fn is_empty(mut self) -> bool { + pub(crate) fn any_remaining(mut self) -> bool { // NOTE: this mimics the behavior of `QueryIter::next()`, expect that it // never gets a `Self::Item`. unsafe { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 699f784bd8707..6ab071c1df04a 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -74,7 +74,7 @@ where // `QueryIter::is_empty` never creates any references (mutable or immutable) to the `Item`. unsafe { self.iter_unchecked_manual(world, last_change_tick, change_tick) - .is_empty() + .any_remaining() } } From 3bf67476b0433d8dedc4ded4ef0c1d7f1f4ff327 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:45:42 -0600 Subject: [PATCH 09/15] fix typo --- crates/bevy_ecs/src/query/iter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 8c4d69c7271c3..ee1941ba417f3 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -70,7 +70,7 @@ where } } - /// Consumes `self` and returns true is there were any elements left to yield. + /// Consumes `self` and returns true if there were any elements remaining in this iterator. #[inline(always)] #[allow(clippy::wrong_self_convention)] pub(crate) fn any_remaining(mut self) -> bool { From 7b68322f5a10fe0d6ed73cb56f56d7b811bb9de4 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:46:44 -0600 Subject: [PATCH 10/15] remove clippy warning --- crates/bevy_ecs/src/query/iter.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index ee1941ba417f3..b52a674c54f6a 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -72,7 +72,6 @@ where /// Consumes `self` and returns true if there were any elements remaining in this iterator. #[inline(always)] - #[allow(clippy::wrong_self_convention)] pub(crate) fn any_remaining(mut self) -> bool { // NOTE: this mimics the behavior of `QueryIter::next()`, expect that it // never gets a `Self::Item`. From 18c958655083ef278b8be80f1f7fd09bb1da5c16 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 28 May 2021 21:49:05 -0600 Subject: [PATCH 11/15] update comment to match function name --- crates/bevy_ecs/src/query/state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 6ab071c1df04a..69999d9694492 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -70,8 +70,8 @@ where #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - // SAFE: the iterator is instantly consumed via `is_empty` and the implementation of - // `QueryIter::is_empty` never creates any references (mutable or immutable) to the `Item`. + // SAFE: the iterator is instantly consumed via `any_remaining` and the implementation of + // `QueryIter::any_remaining` never creates any references to the `>::Item`. unsafe { self.iter_unchecked_manual(world, last_change_tick, change_tick) .any_remaining() From c70f6968dcfc68ac5c3fa74b84d759dce0431f41 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 29 May 2021 11:28:09 -0600 Subject: [PATCH 12/15] remove self.fetch from is_empty impl --- crates/bevy_ecs/src/query/iter.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index b52a674c54f6a..b8895e8ad6c29 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -84,7 +84,6 @@ where None => return true, }; let table = &self.tables[*table_id]; - self.fetch.set_table(&self.query_state.fetch_state, table); self.filter.set_table(&self.query_state.filter_state, table); self.current_len = table.len(); self.current_index = 0; @@ -106,11 +105,6 @@ where None => return true, }; let archetype = &self.archetypes[*archetype_id]; - self.fetch.set_archetype( - &self.query_state.fetch_state, - archetype, - self.tables, - ); self.filter.set_archetype( &self.query_state.filter_state, archetype, From 865a6542cf348840204c50f8f1960617d36b1237 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 29 May 2021 11:29:19 -0600 Subject: [PATCH 13/15] fix typo --- crates/bevy_ecs/src/query/iter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index b8895e8ad6c29..7734e4951170f 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -73,7 +73,7 @@ where /// Consumes `self` and returns true if there were any elements remaining in this iterator. #[inline(always)] pub(crate) fn any_remaining(mut self) -> bool { - // NOTE: this mimics the behavior of `QueryIter::next()`, expect that it + // NOTE: this mimics the behavior of `QueryIter::next()`, except that it // never gets a `Self::Item`. unsafe { if self.is_dense { From cc62ffda0beac05883247eeeff6d582178566ca0 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 29 May 2021 12:08:13 -0600 Subject: [PATCH 14/15] add todo docs --- crates/bevy_ecs/src/system/query.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index c19ab54147386..6173889eeef33 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -547,6 +547,8 @@ where /// Returns true if this query contains no elements. #[inline] pub fn is_empty(&self) -> bool { + // TODO: This code can be replaced with `self.iter().next().is_none()` if/when + // we sort out how to convert "write" queries to "read" queries. self.state .is_empty(self.world, self.last_change_tick, self.change_tick) } From 59cbce28d032e988373216eebb7303e1a03470bd Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Tue, 1 Jun 2021 22:19:14 -0600 Subject: [PATCH 15/15] fix name of none_remaining --- crates/bevy_ecs/src/query/iter.rs | 4 ++-- crates/bevy_ecs/src/query/state.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 7734e4951170f..82ddf458cb40f 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -70,9 +70,9 @@ where } } - /// Consumes `self` and returns true if there were any elements remaining in this iterator. + /// Consumes `self` and returns true if there were no elements remaining in this iterator. #[inline(always)] - pub(crate) fn any_remaining(mut self) -> bool { + pub(crate) fn none_remaining(mut self) -> bool { // NOTE: this mimics the behavior of `QueryIter::next()`, except that it // never gets a `Self::Item`. unsafe { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 69999d9694492..05c009c6e82db 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -70,11 +70,11 @@ where #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - // SAFE: the iterator is instantly consumed via `any_remaining` and the implementation of - // `QueryIter::any_remaining` never creates any references to the `>::Item`. + // SAFE: the iterator is instantly consumed via `none_remaining` and the implementation of + // `QueryIter::none_remaining` never creates any references to the `>::Item`. unsafe { self.iter_unchecked_manual(world, last_change_tick, change_tick) - .any_remaining() + .none_remaining() } }