From 0ba596122b5f352e0b041803bdeb66f555b971f0 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 14:26:57 -0400 Subject: [PATCH 1/8] wip --- crates/bevy_ecs/src/query/access.rs | 25 ++++++++++++++++++++++--- crates/bevy_ecs/src/query/fetch.rs | 15 ++++----------- crates/bevy_ecs/src/query/filter.rs | 15 ++++----------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 27a8c9532e660..d7cbf815e36b8 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -339,7 +339,7 @@ impl Default for FilteredAccess { Self { access: Access::default(), required: FixedBitSet::default(), - filter_sets: vec![AccessFilters::default()], + filter_sets: vec![], } } } @@ -388,6 +388,9 @@ impl FilteredAccess { /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. pub fn and_with(&mut self, index: T) { + if self.filter_sets.is_empty() { + self.filter_sets.push(AccessFilters::default()); + } for filter in &mut self.filter_sets { filter.with.grow_and_insert(index.sparse_set_index()); } @@ -398,6 +401,9 @@ impl FilteredAccess { /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. pub fn and_without(&mut self, index: T) { + if self.filter_sets.is_empty() { + self.filter_sets.push(AccessFilters::default()); + } for filter in &mut self.filter_sets { filter.without.grow_and_insert(index.sparse_set_index()); } @@ -409,7 +415,12 @@ impl FilteredAccess { /// where each element (`AccessFilters`) represents a conjunction, /// we can simply append to the array. pub fn append_or(&mut self, other: &FilteredAccess) { - self.filter_sets.append(&mut other.filter_sets.clone()); + let mut other_filter_sets = if other.filter_sets.is_empty() { + vec![AccessFilters::default()] + } else { + other.filter_sets.clone() + }; + self.filter_sets.append(&mut other_filter_sets); } /// Adds all of the accesses from `other` to `self`. @@ -457,6 +468,14 @@ impl FilteredAccess { self.access.extend(&other.access); self.required.union_with(&other.required); + if other.filter_sets.is_empty() { + return; + } + if self.filter_sets.is_empty() { + self.filter_sets = other.filter_sets.clone(); + return + } + // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: // in this case we can short-circuit by performing an in-place union for each bitset. if other.filter_sets.len() == 1 { @@ -579,7 +598,7 @@ impl FilteredAccessSet { /// mutually exclusive. The fine grained phase iterates over all filters in /// the `self` set and compares it to all the filters in the `other` set, /// making sure they are all mutually compatible. - pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { + pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { if self.combined_access.is_compatible(other.combined_access()) { return true; } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 2ebf95cdea787..258b2995a4a22 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1865,18 +1865,11 @@ macro_rules! impl_anytuple_fetch { let ($($name,)*) = state; let mut _new_access = _access.clone(); - let mut _not_first = false; $( - if _not_first { - let mut intermediate = _access.clone(); - $name::update_component_access($name, &mut intermediate); - _new_access.append_or(&intermediate); - _new_access.extend_access(&intermediate); - } else { - $name::update_component_access($name, &mut _new_access); - _new_access.required = _access.required.clone(); - _not_first = true; - } + let mut intermediate = _access.clone(); + $name::update_component_access($name, &mut intermediate); + _new_access.append_or(&intermediate); + _new_access.extend_access(&intermediate); )* *_access = _new_access; diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 316aed862bc44..5b3f9dce130dc 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -444,18 +444,11 @@ macro_rules! impl_or_query_filter { let ($($filter,)*) = state; let mut _new_access = access.clone(); - let mut _not_first = false; $( - if _not_first { - let mut intermediate = access.clone(); - $filter::update_component_access($filter, &mut intermediate); - _new_access.append_or(&intermediate); - _new_access.extend_access(&intermediate); - } else { - $filter::update_component_access($filter, &mut _new_access); - _new_access.required = access.required.clone(); - _not_first = true; - } + let mut intermediate = access.clone(); + $filter::update_component_access($filter, &mut intermediate); + _new_access.append_or(&intermediate); + _new_access.extend_access(&intermediate); )* *access = _new_access; From 73954ca3066af3f9822f50fadb8769340b2e6ab4 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 19:38:22 -0400 Subject: [PATCH 2/8] update queries to match correctly for () filter --- crates/bevy_ecs/src/query/access.rs | 1 + crates/bevy_ecs/src/query/state.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index d7cbf815e36b8..3f21ac6f1880c 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -416,6 +416,7 @@ impl FilteredAccess { /// we can simply append to the array. pub fn append_or(&mut self, other: &FilteredAccess) { let mut other_filter_sets = if other.filter_sets.is_empty() { + // we want to explicit append with an empty filter set vec![AccessFilters::default()] } else { other.filter_sets.clone() diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index dcb20250c2466..622d1832a2442 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -434,7 +434,7 @@ impl QueryState { /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. pub fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { - self.component_access.filter_sets.iter().any(|set| { + self.component_access.filter_sets.is_empty() || self.component_access.filter_sets.iter().any(|set| { set.with .ones() .all(|index| set_contains_id(ComponentId::get_sparse_set_index(index))) From 7b2881a96ca8203f9a0a55be025e49c136a7a3bf Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 20:21:12 -0400 Subject: [PATCH 3/8] fix is_compatible check --- crates/bevy_ecs/src/query/access.rs | 17 +++++++++-------- crates/bevy_ecs/src/query/state.rs | 17 +++++++++-------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 3f21ac6f1880c..fb8ab8bfb5962 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -442,12 +442,13 @@ impl FilteredAccess { // // For example, `Query<&mut C, Or<(With, Without)>>` is compatible `Query<&mut C, (With, Without)>`, // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. - self.filter_sets.iter().all(|filter| { - other - .filter_sets - .iter() - .all(|other_filter| filter.is_ruled_out_by(other_filter)) - }) + !self.filter_sets.is_empty() + && self.filter_sets.iter().all(|filter| { + other + .filter_sets + .iter() + .all(|other_filter| filter.is_ruled_out_by(other_filter)) + }) } /// Returns a vector of elements that this and `other` cannot access at the same time. @@ -474,7 +475,7 @@ impl FilteredAccess { } if self.filter_sets.is_empty() { self.filter_sets = other.filter_sets.clone(); - return + return; } // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: @@ -599,7 +600,7 @@ impl FilteredAccessSet { /// mutually exclusive. The fine grained phase iterates over all filters in /// the `self` set and compares it to all the filters in the `other` set, /// making sure they are all mutually compatible. - pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { + pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { if self.combined_access.is_compatible(other.combined_access()) { return true; } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 622d1832a2442..a58de17fbc875 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -434,15 +434,16 @@ impl QueryState { /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. pub fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { - self.component_access.filter_sets.is_empty() || self.component_access.filter_sets.iter().any(|set| { - set.with - .ones() - .all(|index| set_contains_id(ComponentId::get_sparse_set_index(index))) - && set - .without + self.component_access.filter_sets.is_empty() + || self.component_access.filter_sets.iter().any(|set| { + set.with .ones() - .all(|index| !set_contains_id(ComponentId::get_sparse_set_index(index))) - }) + .all(|index| set_contains_id(ComponentId::get_sparse_set_index(index))) + && set + .without + .ones() + .all(|index| !set_contains_id(ComponentId::get_sparse_set_index(index))) + }) } /// For the given `archetype`, adds any component accessed used by this query's underlying [`FilteredAccess`] to `access`. From de490278f548a0308199991f8f535eed0626ddb8 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 20:40:35 -0400 Subject: [PATCH 4/8] add comment --- crates/bevy_ecs/src/query/access.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index fb8ab8bfb5962..3146005e1109b 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -416,7 +416,7 @@ impl FilteredAccess { /// we can simply append to the array. pub fn append_or(&mut self, other: &FilteredAccess) { let mut other_filter_sets = if other.filter_sets.is_empty() { - // we want to explicit append with an empty filter set + // we want to explicit append with an empty filter set, for example in Or<((), With)> vec![AccessFilters::default()] } else { other.filter_sets.clone() From 849f735f3ebb51dd9647f1d64f26ecad7a064257 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 21:14:24 -0400 Subject: [PATCH 5/8] fix --- crates/bevy_ecs/src/query/fetch.rs | 4 +--- crates/bevy_ecs/src/query/filter.rs | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 0cd88413306a8..7736517e9ba6e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1871,10 +1871,8 @@ macro_rules! impl_anytuple_fetch { // we use an intermediate empty access because we only want to update the filters, not the access let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); - _new_access.append_or(&intermediate); + _access.append_or(&intermediate); )* - - _access.filter_sets = _new_access.filter_sets; } #[allow(unused_variables)] fn init_state(world: &mut World) -> Self::State { diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 5b3f9dce130dc..0a7376edae03f 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -448,7 +448,6 @@ macro_rules! impl_or_query_filter { let mut intermediate = access.clone(); $filter::update_component_access($filter, &mut intermediate); _new_access.append_or(&intermediate); - _new_access.extend_access(&intermediate); )* *access = _new_access; From a7b5cd4a789dd4736061d4f92c059aef868fcf15 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 21:45:01 -0400 Subject: [PATCH 6/8] clippy --- crates/bevy_ecs/src/query/access.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 3146005e1109b..33b6ec6fecbc9 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -474,7 +474,7 @@ impl FilteredAccess { return; } if self.filter_sets.is_empty() { - self.filter_sets = other.filter_sets.clone(); + self.filter_sets.clone_from(&other.filter_sets); return; } From 2aebf38d235b9a0adf355bc3f02f8348c6455974 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 21:46:30 -0400 Subject: [PATCH 7/8] typo --- crates/bevy_ecs/src/query/access.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 33b6ec6fecbc9..caa9db3e8568f 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -416,7 +416,7 @@ impl FilteredAccess { /// we can simply append to the array. pub fn append_or(&mut self, other: &FilteredAccess) { let mut other_filter_sets = if other.filter_sets.is_empty() { - // we want to explicit append with an empty filter set, for example in Or<((), With)> + // we want to explicitly append_or with an empty filter set, for example in Or<((), With)> vec![AccessFilters::default()] } else { other.filter_sets.clone() From 8020fd44ff0ff1d92191b4046ff3e176e811e917 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 25 Jun 2024 23:00:14 -0400 Subject: [PATCH 8/8] reduce the number of duplicate accesses for Or and AnyOf --- crates/bevy_ecs/src/query/fetch.rs | 11 ++++++----- crates/bevy_ecs/src/query/filter.rs | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 7736517e9ba6e..601ec5433ff88 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1862,17 +1862,18 @@ macro_rules! impl_anytuple_fetch { } fn update_component_access(state: &Self::State, _access: &mut FilteredAccess) { - // update the access (add the read/writes) - <($(Option<$name>,)*)>::update_component_access(state, _access); - // update the filters (Or<(With<$name>,)>) let ($($name,)*) = state; + let mut _new_access = FilteredAccess::default(); $( // we use an intermediate empty access because we only want to update the filters, not the access - let mut intermediate = FilteredAccess::default(); + let mut intermediate = _access.clone(); $name::update_component_access($name, &mut intermediate); - _access.append_or(&intermediate); + _new_access.append_or(&intermediate); )* + _access.filter_sets = _new_access.filter_sets; + + <($(Option<$name>,)*)>::update_component_access(state, _access); } #[allow(unused_variables)] fn init_state(world: &mut World) -> Self::State { diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 0a7376edae03f..417f180605834 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -443,7 +443,7 @@ macro_rules! impl_or_query_filter { fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { let ($($filter,)*) = state; - let mut _new_access = access.clone(); + let mut _new_access = FilteredAccess::default(); $( let mut intermediate = access.clone(); $filter::update_component_access($filter, &mut intermediate);