ParamSet for conflicting SystemParam:s (bevyengine#2765)

# Objective Add a system parameter `ParamSet` to be used as container for conflicting parameters. ## Solution Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed `get_conflicts` to return every conflicting component instead of breaking on the first conflicting `FilteredAccess`. Co-authored-by: bilsen <[email protected]>
mockersf · Mar 29, 2022 · 63fee25 · 63fee25
1 parent 7ff3d87
commit 63fee25
Show file tree

Hide file tree

Showing 20 changed files with 180 additions and 166 deletions.
diff --git a/.github/contributing/example_style_guide.md b/.github/contributing/example_style_guide.md
@@ -36,7 +36,7 @@ For more advice on writing examples, see the [relevant section](../../CONTRIBUTI
 2. Prefer `for` loops over `.for_each`. The latter is faster (for now), but it is less clear for beginners, less idiomatic, and less flexible.
 3. Use `.single` and `.single_mut` where appropriate.
 4. In Queries, prefer `With<T>` filters over actually fetching unused data with `&T`.
-5. Prefer disjoint queries using `With` and `Without` over query sets when you need more than one query in a single system.
+5. Prefer disjoint queries using `With` and `Without` over param sets when you need more than one query in a single system.
 6. Prefer structs with named fields over tuple structs except in the case of single-field wrapper types.
 7. Use enum-labels over string-labels for system / stage / etc. labels.
 

diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs
@@ -178,88 +178,83 @@ fn get_idents(fmt_string: fn(usize) -> String, count: usize) -> Vec<Ident> {
 }
 
 #[proc_macro]
-pub fn impl_query_set(_input: TokenStream) -> TokenStream {
+pub fn impl_param_set(_input: TokenStream) -> TokenStream {
     let mut tokens = TokenStream::new();
-    let max_queries = 4;
-    let queries = get_idents(|i| format!("Q{}", i), max_queries);
-    let filters = get_idents(|i| format!("F{}", i), max_queries);
-    let mut query_fn_muts = Vec::new();
-    for i in 0..max_queries {
-        let query = &queries[i];
-        let filter = &filters[i];
-        let fn_name = Ident::new(&format!("q{}", i), Span::call_site());
+    let max_params = 8;
+    let params = get_idents(|i| format!("P{}", i), max_params);
+    let params_fetch = get_idents(|i| format!("PF{}", i), max_params);
+    let metas = get_idents(|i| format!("m{}", i), max_params);
+    let mut param_fn_muts = Vec::new();
+    for (i, param) in params.iter().enumerate() {
+        let fn_name = Ident::new(&format!("p{}", i), Span::call_site());
         let index = Index::from(i);
-        query_fn_muts.push(quote! {
-            pub fn #fn_name(&mut self) -> Query<'_, '_, #query, #filter> {
+        param_fn_muts.push(quote! {
+            pub fn #fn_name<'a>(&'a mut self) -> <#param::Fetch as SystemParamFetch<'a, 'a>>::Item {
                 // SAFE: systems run without conflicts with other systems.
-                // Conflicting queries in QuerySet are not accessible at the same time
-                // QuerySets are guaranteed to not conflict with other SystemParams
+                // Conflicting params in ParamSet are not accessible at the same time
+                // ParamSets are guaranteed to not conflict with other SystemParams
                 unsafe {
-                    Query::new(self.world, &self.query_states.#index, self.last_change_tick, self.change_tick)
+                    <#param::Fetch as SystemParamFetch<'a, 'a>>::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick)
                 }
             }
         });
     }
 
-    for query_count in 1..=max_queries {
-        let query = &queries[0..query_count];
-        let filter = &filters[0..query_count];
-        let query_fn_mut = &query_fn_muts[0..query_count];
+    for param_count in 1..=max_params {
+        let param = &params[0..param_count];
+        let param_fetch = &params_fetch[0..param_count];
+        let meta = &metas[0..param_count];
+        let param_fn_mut = &param_fn_muts[0..param_count];
         tokens.extend(TokenStream::from(quote! {
-            impl<'w, 's, #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParam for QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>
-                where #(#filter::Fetch: FilterFetch,)*
+            impl<'w, 's, #(#param: SystemParam,)*> SystemParam for ParamSet<'w, 's, (#(#param,)*)>
             {
-                type Fetch = QuerySetState<(#(QueryState<#query, #filter>,)*)>;
+                type Fetch = ParamSetState<(#(#param::Fetch,)*)>;
             }
 
-            // SAFE: All Queries are constrained to ReadOnlyFetch, so World is only read
-            unsafe impl<#(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> ReadOnlySystemParamFetch for QuerySetState<(#(QueryState<#query, #filter>,)*)>
-            where #(#query::Fetch: ReadOnlyFetch,)* #(#filter::Fetch: FilterFetch,)*
+            // SAFE: All parameters are constrained to ReadOnlyFetch, so World is only read
+
+            unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> ReadOnlySystemParamFetch for ParamSetState<(#(#param_fetch,)*)>
+            where #(#param_fetch: ReadOnlySystemParamFetch,)*
             { }
 
-            // SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any QueryState conflicts
+            // SAFE: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts
             // with any prior access, a panic will occur.
-            unsafe impl<#(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParamState for QuerySetState<(#(QueryState<#query, #filter>,)*)>
-                where #(#filter::Fetch: FilterFetch,)*
+
+            unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> SystemParamState for ParamSetState<(#(#param_fetch,)*)>
             {
                 fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
                     #(
-                        let mut #query = QueryState::<#query, #filter>::new(world);
-                        assert_component_access_compatibility(
-                            &system_meta.name,
-                            std::any::type_name::<#query>(),
-                            std::any::type_name::<#filter>(),
-                            &system_meta.component_access_set,
-                            &#query.component_access,
-                            world,
-                        );
+                        // Pretend to add each param to the system alone, see if it conflicts
+                        let mut #meta = system_meta.clone();
+                        #meta.component_access_set.clear();
+                        #meta.archetype_component_access.clear();
+                        #param_fetch::init(world, &mut #meta);
+                        let #param = #param_fetch::init(world, &mut system_meta.clone());
                     )*
                     #(
                         system_meta
                             .component_access_set
-                            .add(#query.component_access.clone());
+                            .extend(#meta.component_access_set);
                         system_meta
                             .archetype_component_access
-                            .extend(&#query.archetype_component_access);
+                            .extend(&#meta.archetype_component_access);
                     )*
-                    QuerySetState((#(#query,)*))
+                    ParamSetState((#(#param,)*))
                 }
 
                 fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) {
-                    let (#(#query,)*) = &mut self.0;
+                    let (#(#param,)*) = &mut self.0;
                     #(
-                        #query.new_archetype(archetype);
-                        system_meta
-                            .archetype_component_access
-                            .extend(&#query.archetype_component_access);
+                        #param.new_archetype(archetype, system_meta);
                     )*
                 }
             }
 
-            impl<'w, 's, #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParamFetch<'w, 's> for QuerySetState<(#(QueryState<#query, #filter>,)*)>
-                where #(#filter::Fetch: FilterFetch,)*
+
+
+            impl<'w, 's, #(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> SystemParamFetch<'w, 's> for ParamSetState<(#(#param_fetch,)*)>
             {
-                type Item = QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>;
+                type Item = ParamSet<'w, 's, (#(<#param_fetch as SystemParamFetch<'w, 's>>::Item,)*)>;
 
                 #[inline]
                 unsafe fn get_param(
@@ -268,19 +263,19 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream {
                     world: &'w World,
                     change_tick: u32,
                 ) -> Self::Item {
-                    QuerySet {
-                        query_states: &state.0,
+                    ParamSet {
+                        param_states: &mut state.0,
+                        system_meta: system_meta.clone(),
                         world,
-                        last_change_tick: system_meta.last_change_tick,
                         change_tick,
                     }
                 }
             }
 
-            impl<'w, 's, #(#query: WorldQuery,)* #(#filter: WorldQuery,)*> QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>
-                where #(#filter::Fetch: FilterFetch,)*
+            impl<'w, 's, #(#param: SystemParam,)*> ParamSet<'w, 's, (#(#param,)*)>
             {
-                #(#query_fn_mut)*
+
+                #(#param_fn_mut)*
             }
         }));
     }

diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs
@@ -34,7 +34,7 @@ pub mod prelude {
         },
         system::{
             Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend,
-            NonSendMut, Query, QuerySet, RemovedComponents, Res, ResMut, System,
+            NonSendMut, ParamSet, Query, RemovedComponents, Res, ResMut, System,
             SystemParamFunction,
         },
         world::{FromWorld, Mut, World},

diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs
@@ -1,4 +1,5 @@
 use crate::storage::SparseSetIndex;
+use bevy_utils::HashSet;
 use fixedbitset::FixedBitSet;
 use std::marker::PhantomData;
 
@@ -129,7 +130,7 @@ impl<T: SparseSetIndex> Access<T> {
     }
 }
 
-#[derive(Clone, Eq, PartialEq)]
+#[derive(Clone, Eq, PartialEq, Debug)]
 pub struct FilteredAccess<T: SparseSetIndex> {
     access: Access<T>,
     with: FixedBitSet,
@@ -146,6 +147,14 @@ impl<T: SparseSetIndex> Default for FilteredAccess<T> {
     }
 }
 
+impl<T: SparseSetIndex> From<FilteredAccess<T>> for FilteredAccessSet<T> {
+    fn from(filtered_access: FilteredAccess<T>) -> Self {
+        let mut base = FilteredAccessSet::<T>::default();
+        base.add(filtered_access);
+        base
+    }
+}
+
 impl<T: SparseSetIndex> FilteredAccess<T> {
     #[inline]
     pub fn access(&self) -> &Access<T> {
@@ -191,7 +200,7 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
         self.access.read_all();
     }
 }
-
+#[derive(Clone, Debug)]
 pub struct FilteredAccessSet<T: SparseSetIndex> {
     combined_access: Access<T>,
     filtered_accesses: Vec<FilteredAccess<T>>,
@@ -211,22 +220,42 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
     pub fn get_conflicts(&self, filtered_access: &FilteredAccess<T>) -> Vec<T> {
         // if combined unfiltered access is incompatible, check each filtered access for
         // compatibility
+        let mut conflicts = HashSet::<usize>::default();
         if !filtered_access.access.is_compatible(&self.combined_access) {
             for current_filtered_access in &self.filtered_accesses {
                 if !current_filtered_access.is_compatible(filtered_access) {
-                    return current_filtered_access
-                        .access
-                        .get_conflicts(&filtered_access.access);
+                    conflicts.extend(
+                        current_filtered_access
+                            .access
+                            .get_conflicts(&filtered_access.access)
+                            .iter()
+                            .map(|ind| ind.sparse_set_index()),
+                    );
                 }
             }
         }
-        Vec::new()
+        conflicts
+            .iter()
+            .map(|ind| T::get_sparse_set_index(*ind))
+            .collect()
     }
 
     pub fn add(&mut self, filtered_access: FilteredAccess<T>) {
         self.combined_access.extend(&filtered_access.access);
         self.filtered_accesses.push(filtered_access);
     }
+
+    pub fn extend(&mut self, filtered_access_set: FilteredAccessSet<T>) {
+        self.combined_access
+            .extend(&filtered_access_set.combined_access);
+        self.filtered_accesses
+            .extend(filtered_access_set.filtered_accesses);
+    }
+
+    pub fn clear(&mut self) {
+        self.combined_access.clear();
+        self.filtered_accesses.clear();
+    }
 }
 
 impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {

diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs
@@ -13,6 +13,7 @@ use bevy_ecs_macros::all_tuples;
 use std::{borrow::Cow, fmt::Debug, hash::Hash, marker::PhantomData};
 
 /// The metadata of a [`System`].
+#[derive(Clone)]
 pub struct SystemMeta {
     pub(crate) name: Cow<'static, str>,
     pub(crate) component_access_set: FilteredAccessSet<ComponentId>,

diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs
@@ -100,10 +100,10 @@ mod tests {
         bundle::Bundles,
         component::{Component, Components},
         entity::{Entities, Entity},
-        query::{Added, Changed, Or, QueryState, With, Without},
+        query::{Added, Changed, Or, With, Without},
         schedule::{Schedule, Stage, SystemStage},
         system::{
-            IntoExclusiveSystem, IntoSystem, Local, NonSend, NonSendMut, Query, QuerySet,
+            IntoExclusiveSystem, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query,
             RemovedComponents, Res, ResMut, System, SystemState,
         },
         world::{FromWorld, World},
@@ -211,17 +211,17 @@ mod tests {
     }
 
     #[test]
-    fn or_query_set_system() {
+    fn or_param_set_system() {
         // Regression test for issue #762
         fn query_system(
             mut ran: ResMut<bool>,
-            mut set: QuerySet<(
-                QueryState<(), Or<(Changed<A>, Changed<B>)>>,
-                QueryState<(), Or<(Added<A>, Added<B>)>>,
+            mut set: ParamSet<(
+                Query<(), Or<(Changed<A>, Changed<B>)>>,
+                Query<(), Or<(Added<A>, Added<B>)>>,
             )>,
         ) {
-            let changed = set.q0().iter().count();
-            let added = set.q1().iter().count();
+            let changed = set.p0().iter().count();
+            let added = set.p1().iter().count();
 
             assert_eq!(changed, 1);
             assert_eq!(added, 1);
@@ -320,15 +320,15 @@ mod tests {
 
     #[test]
     fn query_set_system() {
-        fn sys(mut _set: QuerySet<(QueryState<&mut A>, QueryState<&A>)>) {}
+        fn sys(mut _set: ParamSet<(Query<&mut A>, Query<&A>)>) {}
         let mut world = World::default();
         run_system(&mut world, sys);
     }
 
     #[test]
     #[should_panic]
     fn conflicting_query_with_query_set_system() {
-        fn sys(_query: Query<&mut A>, _set: QuerySet<(QueryState<&mut A>, QueryState<&B>)>) {}
+        fn sys(_query: Query<&mut A>, _set: ParamSet<(Query<&mut A>, Query<&B>)>) {}
 
         let mut world = World::default();
         run_system(&mut world, sys);
@@ -337,11 +337,7 @@ mod tests {
     #[test]
     #[should_panic]
     fn conflicting_query_sets_system() {
-        fn sys(
-            _set_1: QuerySet<(QueryState<&mut A>,)>,
-            _set_2: QuerySet<(QueryState<&mut A>, QueryState<&B>)>,
-        ) {
-        }
+        fn sys(_set_1: ParamSet<(Query<&mut A>,)>, _set_2: ParamSet<(Query<&mut A>, Query<&B>)>) {}
 
         let mut world = World::default();
         run_system(&mut world, sys);
@@ -674,11 +670,8 @@ mod tests {
         world.insert_resource(A(42));
         world.spawn().insert(B(7));
 
-        let mut system_state: SystemState<(
-            Res<A>,
-            Query<&B>,
-            QuerySet<(QueryState<&C>, QueryState<&D>)>,
-        )> = SystemState::new(&mut world);
+        let mut system_state: SystemState<(Res<A>, Query<&B>, ParamSet<(Query<&C>, Query<&D>)>)> =
+            SystemState::new(&mut world);
         let (a, query, _) = system_state.get(&world);
         assert_eq!(*a, A(42), "returned resource matches initial value");
         assert_eq!(

diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs
@@ -83,7 +83,7 @@ use thiserror::Error;
 ///
 /// Similarly, a system cannot contain two queries that would break Rust's mutability Rules.
 /// If you need such Queries, you can use Filters to make the Queries disjoint or use a
-/// [`QuerySet`](super::QuerySet).
+/// [`ParamSet`](super::ParamSet).
 ///
 /// ## Entity ID access
 ///