From 0b9da999678567addc621f454c139cd63713085d Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 17:05:56 -0400 Subject: [PATCH 1/8] wip --- crates/bevy_ecs/src/lib.rs | 8 ++ crates/bevy_ecs/src/query/access.rs | 129 ++++++++++++++++++++++------ crates/bevy_ecs/src/system/mod.rs | 14 ++- 3 files changed, 124 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 406beb92d9593..d9237aab2882e 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -90,6 +90,7 @@ mod tests { Arc, Mutex, }, }; + use crate::world::EntityMut; #[derive(Component, Resource, Debug, PartialEq, Eq, Clone, Copy)] struct A(usize); @@ -1360,6 +1361,13 @@ mod tests { world.query::<(&mut A, EntityRef)>(); } + #[test] + #[should_panic] + fn entity_mut_and_entity_mut_query_panic() { + let mut world = World::new(); + world.query::<(EntityMut, EntityMut)>(); + } + #[test] #[should_panic] fn mut_and_mut_query_panic() { diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 54a70690a126f..25246ee68edd8 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,6 +1,7 @@ use crate::storage::SparseSetIndex; use bevy_utils::HashSet; use core::fmt; +use std::fmt::{Debug, Formatter}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; @@ -269,32 +270,42 @@ impl Access { } /// Returns a vector of elements that the access and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &Access) -> Vec { - let mut conflicts = FixedBitSet::default(); + pub fn get_conflicts(&self, other: &Access) -> AccessConflict { + let mut conflicts = FixedBitSet::new(); if self.reads_all { - // QUESTION: How to handle `other.writes_all`? - conflicts.extend(other.writes.ones()); + if other.writes_all { + return AccessConflict::All; + } else { + conflicts.extend(other.writes.ones()); + } } if other.reads_all { - // QUESTION: How to handle `self.writes_all`. - conflicts.extend(self.writes.ones()); + if self.writes_all { + return AccessConflict::All; + } else { + conflicts.extend(self.writes.ones()); + } } if self.writes_all { - conflicts.extend(other.reads_and_writes.ones()); + if other.reads_all { + return AccessConflict::All; + } else { + conflicts.extend(other.reads_and_writes.ones()); + } } if other.writes_all { - conflicts.extend(self.reads_and_writes.ones()); + if self.reads_all { + return AccessConflict::All; + } else { + conflicts.extend(self.reads_and_writes.ones()); + } } - conflicts.extend(self.writes.intersection(&other.reads_and_writes)); conflicts.extend(self.reads_and_writes.intersection(&other.writes)); - conflicts - .ones() - .map(SparseSetIndex::get_sparse_set_index) - .collect() + AccessConflict::Individual(conflicts) } /// Returns the indices of the elements this has access to. @@ -379,6 +390,7 @@ impl Default for FilteredAccess { } } + impl From> for FilteredAccessSet { fn from(filtered_access: FilteredAccess) -> Self { let mut base = FilteredAccessSet::::default(); @@ -387,6 +399,70 @@ impl From> for FilteredAccessSet { } } +/// Records how two accesses conflict with each other +pub enum AccessConflict { + /// Conflict is for all indices + All, + /// Conflict is only for a subset of indices + Individual(FixedBitSet) +} + +impl Debug for AccessConflict { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + AccessConflict::All => { + write!(f, "All") + } + AccessConflict::Individual(set) => { + write!(f, "{:?}", FormattedBitSet::::new(set)) + } + } + } +} + +impl PartialEq for AccessConflict { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (AccessConflict::All, AccessConflict::All) => true, + (AccessConflict::Individual(this), AccessConflict::Individual(other)) => this == other, + _ => false + } + } +} + + +impl AccessConflict { + fn add(&mut self, other: &self) { + match (self, other) { + (AccessConflict::All, _) | (_, AccessConflict::All) => { + *self = AccessConflict::All; + } + (AccessConflict::Individual(this), AccessConflict::Individual(other)) => { + this.extend(other); + } + } + } + + pub(crate) fn is_empty(&self) -> bool { + match self { + AccessConflict::All => false, + AccessConflict::Individual(set) => set.is_empty(), + } + } +} + +impl From for AccessConflict { + fn from(value: FixedBitSet) -> Self { + Self::Individual(value) + } +} + +impl Default for AccessConflict { + fn default() -> Self { + AccessConflict::Individual(FixedBitSet::new()) + } +} + impl FilteredAccess { /// Returns a `FilteredAccess` which has no access and matches everything. /// This is the equivalent of a `TRUE` logic atom. @@ -494,12 +570,12 @@ impl FilteredAccess { } /// Returns a vector of elements that this and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccess) -> Vec { + pub fn get_conflicts(&self, other: &FilteredAccess) -> AccessConflict { if !self.is_compatible(other) { // filters are disjoint, so we can just look at the unfiltered intersection return self.access.get_conflicts(&other.access); } - Vec::new() + AccessConflict::default() } /// Adds all access and filters from `other`. @@ -680,29 +756,29 @@ impl FilteredAccessSet { } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccessSet) -> Vec { + pub fn get_conflicts(&self, other: &FilteredAccessSet) -> AccessConflict { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = HashSet::new(); + let mut conflicts = AccessConflict::default(); if !self.combined_access.is_compatible(other.combined_access()) { for filtered in &self.filtered_accesses { for other_filtered in &other.filtered_accesses { - conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); + conflicts.add(&filtered.get_conflicts(other_filtered)); } } } - conflicts.into_iter().collect() + conflicts } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> Vec { + pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> AccessConflict { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = HashSet::new(); + let mut conflicts = AccessConflict::default(); if !self.combined_access.is_compatible(filtered_access.access()) { for filtered in &self.filtered_accesses { - conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); + conflicts.add(&filtered.get_conflicts(filtered_access)); } } - conflicts.into_iter().collect() + conflicts } /// Adds the filtered access to the set. @@ -762,9 +838,10 @@ impl Default for FilteredAccessSet { #[cfg(test)] mod tests { use crate::query::access::AccessFilters; - use crate::query::{Access, FilteredAccess, FilteredAccessSet}; + use crate::query::{Access, AccessConflict, FilteredAccess, FilteredAccessSet}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; + use crate::storage::SparseSetIndex; fn create_sample_access() -> Access { let mut access = Access::::default(); @@ -953,9 +1030,11 @@ mod tests { filter_b.add_write(1); let conflicts = access_a.get_conflicts_single(&filter_b); + let mut raw_conflicts = FixedBitSet::new(); + raw_conflicts.insert(1); assert_eq!( &conflicts, - &[1_usize], + &AccessConflict::Individual(raw_conflicts), "access_a: {access_a:?}, filter_b: {filter_b:?}" ); } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 071e02f9d3ca4..4e209b06f52dc 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -341,7 +341,7 @@ impl std::ops::DerefMut for In { #[cfg(test)] mod tests { use std::any::TypeId; - + use fixedbitset::FixedBitSet; use bevy_utils::default; use crate::{ @@ -364,6 +364,9 @@ mod tests { }, world::{FromWorld, World}, }; + use crate::query::AccessConflict; + use crate::storage::SparseSetIndex; + use crate::world::EntityMut; #[derive(Resource, PartialEq, Debug)] enum SystemRan { @@ -1102,7 +1105,7 @@ mod tests { .get_resource_id(TypeId::of::()) .unwrap(); let d_id = world.components().get_id(TypeId::of::()).unwrap(); - assert_eq!(conflicts, vec![b_id, d_id]); + assert_eq!(conflicts, AccessConflict::Individual(vec![b_id.sparse_set_index(), d_id.sparse_set_index()].iter().copied().collect())); } #[test] @@ -1608,6 +1611,13 @@ mod tests { super::assert_system_does_not_conflict(system); } + #[test] + #[should_panic] + fn assert_entity_mut_system_does_conflict() { + fn system(_query: Query, _q2: Query) {} + super::assert_system_does_not_conflict(system); + } + #[test] #[should_panic] fn panic_inside_system() { From 74d0339245142ed2ca9e21e9a663d4c6f7396c6b Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 19:39:59 -0400 Subject: [PATCH 2/8] fix EntityMut soundness issue --- crates/bevy_ecs/src/query/access.rs | 71 ++++++++-------------- crates/bevy_ecs/src/schedule/schedule.rs | 26 +++++--- crates/bevy_ecs/src/system/mod.rs | 7 +-- crates/bevy_ecs/src/system/system_param.rs | 15 +++-- 4 files changed, 56 insertions(+), 63 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 25246ee68edd8..5d14c332cc527 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,7 +1,6 @@ use crate::storage::SparseSetIndex; -use bevy_utils::HashSet; use core::fmt; -use std::fmt::{Debug, Formatter}; +use std::fmt::{Debug}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; @@ -270,7 +269,7 @@ impl Access { } /// Returns a vector of elements that the access and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &Access) -> AccessConflict { + pub fn get_conflicts(&self, other: &Access) -> AccessConflict { let mut conflicts = FixedBitSet::new(); if self.reads_all { if other.writes_all { @@ -400,46 +399,25 @@ impl From> for FilteredAccessSet { } /// Records how two accesses conflict with each other -pub enum AccessConflict { +#[derive(Debug, PartialEq)] +pub enum AccessConflict { /// Conflict is for all indices All, /// Conflict is only for a subset of indices Individual(FixedBitSet) } -impl Debug for AccessConflict { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - AccessConflict::All => { - write!(f, "All") - } - AccessConflict::Individual(set) => { - write!(f, "{:?}", FormattedBitSet::::new(set)) - } - } - } -} -impl PartialEq for AccessConflict { - fn eq(&self, other: &Self) -> bool { +impl AccessConflict { + fn add(&mut self, other: &Self) { match (self, other) { - (AccessConflict::All, AccessConflict::All) => true, - (AccessConflict::Individual(this), AccessConflict::Individual(other)) => this == other, - _ => false - } - } -} - - -impl AccessConflict { - fn add(&mut self, other: &self) { - match (self, other) { - (AccessConflict::All, _) | (_, AccessConflict::All) => { - *self = AccessConflict::All; + (s, AccessConflict::All) => { + *s = AccessConflict::All; } (AccessConflict::Individual(this), AccessConflict::Individual(other)) => { - this.extend(other); + this.extend(other.ones()); } + _ => {} } } @@ -451,13 +429,19 @@ impl AccessConflict { } } -impl From for AccessConflict { +impl From for AccessConflict { fn from(value: FixedBitSet) -> Self { Self::Individual(value) } } -impl Default for AccessConflict { +impl From> for AccessConflict { + fn from(value: Vec) -> Self { + Self::Individual(value.iter().map(T::sparse_set_index).collect()) + } +} + +impl Default for AccessConflict { fn default() -> Self { AccessConflict::Individual(FixedBitSet::new()) } @@ -570,7 +554,7 @@ impl FilteredAccess { } /// Returns a vector of elements that this and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccess) -> AccessConflict { + pub fn get_conflicts(&self, other: &FilteredAccess) -> AccessConflict { if !self.is_compatible(other) { // filters are disjoint, so we can just look at the unfiltered intersection return self.access.get_conflicts(&other.access); @@ -756,7 +740,7 @@ impl FilteredAccessSet { } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccessSet) -> AccessConflict { + pub fn get_conflicts(&self, other: &FilteredAccessSet) -> AccessConflict { // if the unfiltered access is incompatible, must check each pair let mut conflicts = AccessConflict::default(); if !self.combined_access.is_compatible(other.combined_access()) { @@ -770,7 +754,7 @@ impl FilteredAccessSet { } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> AccessConflict { + pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> AccessConflict { // if the unfiltered access is incompatible, must check each pair let mut conflicts = AccessConflict::default(); if !self.combined_access.is_compatible(filtered_access.access()) { @@ -841,7 +825,6 @@ mod tests { use crate::query::{Access, AccessConflict, FilteredAccess, FilteredAccessSet}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; - use crate::storage::SparseSetIndex; fn create_sample_access() -> Access { let mut access = Access::::default(); @@ -1004,21 +987,21 @@ mod tests { access_b.add_read(0); access_b.add_write(1); - assert_eq!(access_a.get_conflicts(&access_b), vec![1]); + assert_eq!(access_a.get_conflicts(&access_b), vec![1_usize].into()); let mut access_c = Access::::default(); access_c.add_write(0); access_c.add_write(1); - assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]); - assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]); + assert_eq!(access_a.get_conflicts(&access_c), vec![0_usize, 1_usize].into()); + assert_eq!(access_b.get_conflicts(&access_c), vec![0_usize, 1_usize].into()); let mut access_d = Access::::default(); access_d.add_read(0); - assert_eq!(access_d.get_conflicts(&access_a), vec![]); - assert_eq!(access_d.get_conflicts(&access_b), vec![]); - assert_eq!(access_d.get_conflicts(&access_c), vec![0]); + assert_eq!(access_d.get_conflicts(&access_a), AccessConflict::default()); + assert_eq!(access_d.get_conflicts(&access_b), AccessConflict::default()); + assert_eq!(access_d.get_conflicts(&access_c), vec![0_usize].into()); } #[test] diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index c9dc06438f0cf..4bdd92df22ccd 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -24,6 +24,8 @@ use crate::{ }; pub use stepping::Stepping; +use crate::query::AccessConflict; +use crate::storage::SparseSetIndex; /// Resource that stores [`Schedule`]s mapped to [`ScheduleLabel`]s excluding the current running [`Schedule`]. #[derive(Default, Resource)] @@ -1363,14 +1365,22 @@ impl ScheduleGraph { let access_a = system_a.component_access(); let access_b = system_b.component_access(); if !access_a.is_compatible(access_b) { - let conflicts: Vec<_> = access_a - .get_conflicts(access_b) - .into_iter() - .filter(|id| !ignored_ambiguities.contains(id)) - .collect(); - - if !conflicts.is_empty() { - conflicting_systems.push((a, b, conflicts)); + match access_a.get_conflicts(access_b) { + AccessConflict::Individual(conflicts) => { + let conflicts: Vec<_> = conflicts + .ones() + .map(|i| ComponentId::get_sparse_set_index(i)) + .filter(|id| !ignored_ambiguities.contains(id)) + .collect(); + if !conflicts.is_empty() { + conflicting_systems.push((a, b, conflicts)); + } + } + AccessConflict::All => { + // there is no specific component conflicting, but the systems are overall incompatible + // for example 2 systems with `Query` + conflicting_systems.push((a, b, Vec::new())); + } } } } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 4e209b06f52dc..b73a0b1da53a0 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -341,7 +341,6 @@ impl std::ops::DerefMut for In { #[cfg(test)] mod tests { use std::any::TypeId; - use fixedbitset::FixedBitSet; use bevy_utils::default; use crate::{ @@ -364,8 +363,6 @@ mod tests { }, world::{FromWorld, World}, }; - use crate::query::AccessConflict; - use crate::storage::SparseSetIndex; use crate::world::EntityMut; #[derive(Resource, PartialEq, Debug)] @@ -1105,7 +1102,7 @@ mod tests { .get_resource_id(TypeId::of::()) .unwrap(); let d_id = world.components().get_id(TypeId::of::()).unwrap(); - assert_eq!(conflicts, AccessConflict::Individual(vec![b_id.sparse_set_index(), d_id.sparse_set_index()].iter().copied().collect())); + assert_eq!(conflicts, vec![b_id, d_id].into()); } #[test] @@ -1612,7 +1609,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001")] fn assert_entity_mut_system_does_conflict() { fn system(_query: Query, _q2: Query) {} super::assert_system_does_not_conflict(system); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 8898e34d092e1..893f8922cd930 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -25,6 +25,8 @@ use std::{ marker::PhantomData, ops::{Deref, DerefMut}, }; +use crate::query::AccessConflict; +use crate::storage::SparseSetIndex; /// A parameter that can be used in a [`System`](super::System). /// @@ -296,12 +298,13 @@ fn assert_component_access_compatibility( if conflicts.is_empty() { return; } - let conflicting_components = conflicts - .into_iter() - .map(|component_id| world.components.get_info(component_id).unwrap().name()) - .collect::>(); - let accesses = conflicting_components.join(", "); - panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s) {accesses} in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"); + let accesses = match conflicts { + AccessConflict::All => {""} + AccessConflict::Individual(indices) => { + &format!(" {}", indices.ones().map(|index| world.components.get_info(ComponentId::get_sparse_set_index(index)).unwrap().name()).collect::>().join(", ")) + } + }; + panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s){accesses} in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"); } /// A collection of potentially conflicting [`SystemParam`]s allowed by disjoint access. From 517640e798277973d50801a812a9251bfc055cb4 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 19:47:31 -0400 Subject: [PATCH 3/8] cargo fmt --- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_ecs/src/query/access.rs | 20 +++++++++++--------- crates/bevy_ecs/src/schedule/schedule.rs | 2 +- crates/bevy_ecs/src/system/mod.rs | 8 +++++--- crates/bevy_ecs/src/system/system_param.rs | 21 +++++++++++++++------ 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index d9237aab2882e..acf40c852c997 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -71,6 +71,7 @@ pub mod prelude { mod tests { use crate as bevy_ecs; use crate::prelude::Or; + use crate::world::EntityMut; use crate::{ bundle::Bundle, change_detection::Ref, @@ -90,7 +91,6 @@ mod tests { Arc, Mutex, }, }; - use crate::world::EntityMut; #[derive(Component, Resource, Debug, PartialEq, Eq, Clone, Copy)] struct A(usize); diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 5d14c332cc527..1463adf02a449 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,7 +1,7 @@ use crate::storage::SparseSetIndex; use core::fmt; -use std::fmt::{Debug}; use fixedbitset::FixedBitSet; +use std::fmt::Debug; use std::marker::PhantomData; /// A wrapper struct to make Debug representations of [`FixedBitSet`] easier @@ -389,7 +389,6 @@ impl Default for FilteredAccess { } } - impl From> for FilteredAccessSet { fn from(filtered_access: FilteredAccess) -> Self { let mut base = FilteredAccessSet::::default(); @@ -404,10 +403,9 @@ pub enum AccessConflict { /// Conflict is for all indices All, /// Conflict is only for a subset of indices - Individual(FixedBitSet) + Individual(FixedBitSet), } - impl AccessConflict { fn add(&mut self, other: &Self) { match (self, other) { @@ -993,8 +991,14 @@ mod tests { access_c.add_write(0); access_c.add_write(1); - assert_eq!(access_a.get_conflicts(&access_c), vec![0_usize, 1_usize].into()); - assert_eq!(access_b.get_conflicts(&access_c), vec![0_usize, 1_usize].into()); + assert_eq!( + access_a.get_conflicts(&access_c), + vec![0_usize, 1_usize].into() + ); + assert_eq!( + access_b.get_conflicts(&access_c), + vec![0_usize, 1_usize].into() + ); let mut access_d = Access::::default(); access_d.add_read(0); @@ -1013,11 +1017,9 @@ mod tests { filter_b.add_write(1); let conflicts = access_a.get_conflicts_single(&filter_b); - let mut raw_conflicts = FixedBitSet::new(); - raw_conflicts.insert(1); assert_eq!( &conflicts, - &AccessConflict::Individual(raw_conflicts), + &AccessConflict::from(vec![1_usize]), "access_a: {access_a:?}, filter_b: {filter_b:?}" ); } diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 4bdd92df22ccd..ec0514d9afbfa 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -23,9 +23,9 @@ use crate::{ world::World, }; -pub use stepping::Stepping; use crate::query::AccessConflict; use crate::storage::SparseSetIndex; +pub use stepping::Stepping; /// Resource that stores [`Schedule`]s mapped to [`ScheduleLabel`]s excluding the current running [`Schedule`]. #[derive(Default, Resource)] diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index b73a0b1da53a0..cd4fff26095f2 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -340,9 +340,10 @@ impl std::ops::DerefMut for In { #[cfg(test)] mod tests { - use std::any::TypeId; use bevy_utils::default; + use std::any::TypeId; + use crate::world::EntityMut; use crate::{ self as bevy_ecs, archetype::{ArchetypeComponentId, Archetypes}, @@ -363,7 +364,6 @@ mod tests { }, world::{FromWorld, World}, }; - use crate::world::EntityMut; #[derive(Resource, PartialEq, Debug)] enum SystemRan { @@ -1609,7 +1609,9 @@ mod tests { } #[test] - #[should_panic(expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001")] + #[should_panic( + expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" + )] fn assert_entity_mut_system_does_conflict() { fn system(_query: Query, _q2: Query) {} super::assert_system_does_not_conflict(system); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 893f8922cd930..30c9ac1a86e3f 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1,4 +1,6 @@ pub use crate::change_detection::{NonSendMut, Res, ResMut}; +use crate::query::AccessConflict; +use crate::storage::SparseSetIndex; use crate::{ archetype::{Archetype, Archetypes}, bundle::Bundles, @@ -25,8 +27,6 @@ use std::{ marker::PhantomData, ops::{Deref, DerefMut}, }; -use crate::query::AccessConflict; -use crate::storage::SparseSetIndex; /// A parameter that can be used in a [`System`](super::System). /// @@ -299,10 +299,19 @@ fn assert_component_access_compatibility( return; } let accesses = match conflicts { - AccessConflict::All => {""} - AccessConflict::Individual(indices) => { - &format!(" {}", indices.ones().map(|index| world.components.get_info(ComponentId::get_sparse_set_index(index)).unwrap().name()).collect::>().join(", ")) - } + AccessConflict::All => "", + AccessConflict::Individual(indices) => &format!( + " {}", + indices + .ones() + .map(|index| world + .components + .get_info(ComponentId::get_sparse_set_index(index)) + .unwrap() + .name()) + .collect::>() + .join(", ") + ), }; panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s){accesses} in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"); } From e07acbff5ccfb97eb88e5750282a958ef6bfdfb7 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 20:05:22 -0400 Subject: [PATCH 4/8] clippy --- crates/bevy_ecs/src/query/access.rs | 12 ++++-------- crates/bevy_ecs/src/schedule/schedule.rs | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 1463adf02a449..76d04b4652766 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -274,33 +274,29 @@ impl Access { if self.reads_all { if other.writes_all { return AccessConflict::All; - } else { - conflicts.extend(other.writes.ones()); } + conflicts.extend(other.writes.ones()); } if other.reads_all { if self.writes_all { return AccessConflict::All; - } else { - conflicts.extend(self.writes.ones()); } + conflicts.extend(self.writes.ones()); } if self.writes_all { if other.reads_all { return AccessConflict::All; - } else { - conflicts.extend(other.reads_and_writes.ones()); } + conflicts.extend(other.reads_and_writes.ones()); } if other.writes_all { if self.reads_all { return AccessConflict::All; - } else { - conflicts.extend(self.reads_and_writes.ones()); } + conflicts.extend(self.reads_and_writes.ones()); } conflicts.extend(self.writes.intersection(&other.reads_and_writes)); conflicts.extend(self.reads_and_writes.intersection(&other.writes)); diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index ec0514d9afbfa..00118665f9e2c 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1369,7 +1369,7 @@ impl ScheduleGraph { AccessConflict::Individual(conflicts) => { let conflicts: Vec<_> = conflicts .ones() - .map(|i| ComponentId::get_sparse_set_index(i)) + .map(ComponentId::get_sparse_set_index) .filter(|id| !ignored_ambiguities.contains(id)) .collect(); if !conflicts.is_empty() { From a721ecba4e3dc850e7aae2c083b45530de8fcf36 Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 22:07:13 -0400 Subject: [PATCH 5/8] add extra tests --- crates/bevy_ecs/src/lib.rs | 7 +++++++ crates/bevy_ecs/src/system/mod.rs | 23 +++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index acf40c852c997..27a7f6e670beb 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1361,6 +1361,13 @@ mod tests { world.query::<(&mut A, EntityRef)>(); } + #[test] + #[should_panic] + fn entity_ref_and_entity_mut_query_panic() { + let mut world = World::new(); + world.query::<(EntityRef, EntityMut)>(); + } + #[test] #[should_panic] fn entity_mut_and_entity_mut_query_panic() { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index cd4fff26095f2..e95d1b0407ad8 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -312,9 +312,9 @@ where assert_is_system(system); } -/// Ensures that the provided system doesn't with itself. +/// Ensures that the provided system doesn't conflict with itself. /// -/// This function will panic if the provided system conflict with itself. +/// This function will panic if the provided system conflict with itself. /// /// Note: this will run the system on an empty world. pub fn assert_system_does_not_conflict>(sys: S) { @@ -364,6 +364,7 @@ mod tests { }, world::{FromWorld, World}, }; + use crate::prelude::EntityRef; #[derive(Resource, PartialEq, Debug)] enum SystemRan { @@ -1608,6 +1609,24 @@ mod tests { super::assert_system_does_not_conflict(system); } + #[test] + #[should_panic( + expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_world_and_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" + )] + fn assert_world_and_entity_mut_system_does_conflict() { + fn system(_query: &World, _q2: Query) {} + super::assert_system_does_not_conflict(system); + } + + #[test] + #[should_panic( + expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_entity_ref_and_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" + )] + fn assert_entity_ref_and_entity_mut_system_does_conflict() { + fn system(_query: Query, _q2: Query) {} + super::assert_system_does_not_conflict(system); + } + #[test] #[should_panic( expected = "error[B0001]: Query in system bevy_ecs::system::tests::assert_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" From 944cc182d2265df2165ac03372d681d718e1723d Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Thu, 1 Aug 2024 22:08:53 -0400 Subject: [PATCH 6/8] fmt --- crates/bevy_ecs/src/system/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index e95d1b0407ad8..c727e8912b319 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -343,6 +343,7 @@ mod tests { use bevy_utils::default; use std::any::TypeId; + use crate::prelude::EntityRef; use crate::world::EntityMut; use crate::{ self as bevy_ecs, @@ -364,7 +365,6 @@ mod tests { }, world::{FromWorld, World}, }; - use crate::prelude::EntityRef; #[derive(Resource, PartialEq, Debug)] enum SystemRan { From 378eb032eed171c67597056f5d6ee262596c66ac Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Sat, 3 Aug 2024 08:52:14 -0400 Subject: [PATCH 7/8] address comments --- crates/bevy_ecs/src/lib.rs | 6 +++ crates/bevy_ecs/src/query/access.rs | 63 +++++++++++----------- crates/bevy_ecs/src/schedule/schedule.rs | 6 +-- crates/bevy_ecs/src/system/system_param.rs | 6 +-- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 27a7f6e670beb..4aa27eca67221 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1375,6 +1375,12 @@ mod tests { world.query::<(EntityMut, EntityMut)>(); } + #[test] + fn entity_ref_and_entity_ref_query_no_panic() { + let mut world = World::new(); + world.query::<(EntityRef, EntityRef)>(); + } + #[test] #[should_panic] fn mut_and_mut_query_panic() { diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 76d04b4652766..69791703e725f 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -269,38 +269,38 @@ impl Access { } /// Returns a vector of elements that the access and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &Access) -> AccessConflict { + pub fn get_conflicts(&self, other: &Access) -> AccessConflicts { let mut conflicts = FixedBitSet::new(); if self.reads_all { if other.writes_all { - return AccessConflict::All; + return AccessConflicts::All; } conflicts.extend(other.writes.ones()); } if other.reads_all { if self.writes_all { - return AccessConflict::All; + return AccessConflicts::All; } conflicts.extend(self.writes.ones()); } if self.writes_all { if other.reads_all { - return AccessConflict::All; + return AccessConflicts::All; } conflicts.extend(other.reads_and_writes.ones()); } if other.writes_all { if self.reads_all { - return AccessConflict::All; + return AccessConflicts::All; } conflicts.extend(self.reads_and_writes.ones()); } conflicts.extend(self.writes.intersection(&other.reads_and_writes)); conflicts.extend(self.reads_and_writes.intersection(&other.writes)); - AccessConflict::Individual(conflicts) + AccessConflicts::Individual(conflicts) } /// Returns the indices of the elements this has access to. @@ -395,20 +395,20 @@ impl From> for FilteredAccessSet { /// Records how two accesses conflict with each other #[derive(Debug, PartialEq)] -pub enum AccessConflict { +pub enum AccessConflicts { /// Conflict is for all indices All, - /// Conflict is only for a subset of indices + /// There is a conflict for a subset of indices Individual(FixedBitSet), } -impl AccessConflict { +impl AccessConflicts { fn add(&mut self, other: &Self) { match (self, other) { - (s, AccessConflict::All) => { - *s = AccessConflict::All; + (s, AccessConflicts::All) => { + *s = AccessConflicts::All; } - (AccessConflict::Individual(this), AccessConflict::Individual(other)) => { + (AccessConflicts::Individual(this), AccessConflicts::Individual(other)) => { this.extend(other.ones()); } _ => {} @@ -417,30 +417,29 @@ impl AccessConflict { pub(crate) fn is_empty(&self) -> bool { match self { - AccessConflict::All => false, - AccessConflict::Individual(set) => set.is_empty(), + Self::All => false, + Self::Individual(set) => set.is_empty(), } } + + /// An [`AccessConflicts`] which represents the absence of any conflict + pub(crate) fn empty() -> Self { + Self::Individual(FixedBitSet::new()) + } } -impl From for AccessConflict { +impl From for AccessConflicts { fn from(value: FixedBitSet) -> Self { Self::Individual(value) } } -impl From> for AccessConflict { +impl From> for AccessConflicts { fn from(value: Vec) -> Self { Self::Individual(value.iter().map(T::sparse_set_index).collect()) } } -impl Default for AccessConflict { - fn default() -> Self { - AccessConflict::Individual(FixedBitSet::new()) - } -} - impl FilteredAccess { /// Returns a `FilteredAccess` which has no access and matches everything. /// This is the equivalent of a `TRUE` logic atom. @@ -548,12 +547,12 @@ impl FilteredAccess { } /// Returns a vector of elements that this and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccess) -> AccessConflict { + pub fn get_conflicts(&self, other: &FilteredAccess) -> AccessConflicts { if !self.is_compatible(other) { // filters are disjoint, so we can just look at the unfiltered intersection return self.access.get_conflicts(&other.access); } - AccessConflict::default() + AccessConflicts::empty() } /// Adds all access and filters from `other`. @@ -734,9 +733,9 @@ impl FilteredAccessSet { } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts(&self, other: &FilteredAccessSet) -> AccessConflict { + pub fn get_conflicts(&self, other: &FilteredAccessSet) -> AccessConflicts { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = AccessConflict::default(); + let mut conflicts = AccessConflicts::empty(); if !self.combined_access.is_compatible(other.combined_access()) { for filtered in &self.filtered_accesses { for other_filtered in &other.filtered_accesses { @@ -748,9 +747,9 @@ impl FilteredAccessSet { } /// Returns a vector of elements that this set and `other` cannot access at the same time. - pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> AccessConflict { + pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> AccessConflicts { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = AccessConflict::default(); + let mut conflicts = AccessConflicts::empty(); if !self.combined_access.is_compatible(filtered_access.access()) { for filtered in &self.filtered_accesses { conflicts.add(&filtered.get_conflicts(filtered_access)); @@ -816,7 +815,7 @@ impl Default for FilteredAccessSet { #[cfg(test)] mod tests { use crate::query::access::AccessFilters; - use crate::query::{Access, AccessConflict, FilteredAccess, FilteredAccessSet}; + use crate::query::{Access, AccessConflicts, FilteredAccess, FilteredAccessSet}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; @@ -999,8 +998,8 @@ mod tests { let mut access_d = Access::::default(); access_d.add_read(0); - assert_eq!(access_d.get_conflicts(&access_a), AccessConflict::default()); - assert_eq!(access_d.get_conflicts(&access_b), AccessConflict::default()); + assert_eq!(access_d.get_conflicts(&access_a), AccessConflicts::empty()); + assert_eq!(access_d.get_conflicts(&access_b), AccessConflicts::empty()); assert_eq!(access_d.get_conflicts(&access_c), vec![0_usize].into()); } @@ -1015,7 +1014,7 @@ mod tests { let conflicts = access_a.get_conflicts_single(&filter_b); assert_eq!( &conflicts, - &AccessConflict::from(vec![1_usize]), + &AccessConflicts::from(vec![1_usize]), "access_a: {access_a:?}, filter_b: {filter_b:?}" ); } diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 00118665f9e2c..dad5d99c56f9e 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -23,7 +23,7 @@ use crate::{ world::World, }; -use crate::query::AccessConflict; +use crate::query::AccessConflicts; use crate::storage::SparseSetIndex; pub use stepping::Stepping; @@ -1366,7 +1366,7 @@ impl ScheduleGraph { let access_b = system_b.component_access(); if !access_a.is_compatible(access_b) { match access_a.get_conflicts(access_b) { - AccessConflict::Individual(conflicts) => { + AccessConflicts::Individual(conflicts) => { let conflicts: Vec<_> = conflicts .ones() .map(ComponentId::get_sparse_set_index) @@ -1376,7 +1376,7 @@ impl ScheduleGraph { conflicting_systems.push((a, b, conflicts)); } } - AccessConflict::All => { + AccessConflicts::All => { // there is no specific component conflicting, but the systems are overall incompatible // for example 2 systems with `Query` conflicting_systems.push((a, b, Vec::new())); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 30c9ac1a86e3f..ab52f1a976346 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1,5 +1,5 @@ pub use crate::change_detection::{NonSendMut, Res, ResMut}; -use crate::query::AccessConflict; +use crate::query::AccessConflicts; use crate::storage::SparseSetIndex; use crate::{ archetype::{Archetype, Archetypes}, @@ -299,8 +299,8 @@ fn assert_component_access_compatibility( return; } let accesses = match conflicts { - AccessConflict::All => "", - AccessConflict::Individual(indices) => &format!( + AccessConflicts::All => "", + AccessConflicts::Individual(indices) => &format!( " {}", indices .ones() From 4a092d3558e3d6b31f28f801c92df311c0b1ce5a Mon Sep 17 00:00:00 2001 From: cBournhonesque Date: Tue, 6 Aug 2024 00:20:25 -0400 Subject: [PATCH 8/8] fix --- crates/bevy_ecs/src/query/access.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 2dc8f2b6360e6..d90ff04bfedaa 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -487,14 +487,14 @@ impl Access { pub fn get_conflicts(&self, other: &Access) -> AccessConflicts { let mut conflicts = FixedBitSet::new(); if self.reads_all_components { - if other.writes_all { + if other.writes_all_components { return AccessConflicts::All; } conflicts.extend(other.component_writes.ones()); } if other.reads_all_components { - if self.writes_all { + if self.writes_all_components { return AccessConflicts::All; } conflicts.extend(self.component_writes.ones()); @@ -508,21 +508,19 @@ impl Access { conflicts.extend(self.component_read_and_writes.ones()); } if self.reads_all_resources { - // QUESTION: How to handle `other.writes_all`? - if other.reads_all { + if other.reads_all_resources { return AccessConflicts::All; } conflicts.extend(other.resource_writes.ones()); } if other.reads_all_resources { - // QUESTION: How to handle `self.writes_all`. - if self.reads_all { + if self.reads_all_resources { return AccessConflicts::All; } conflicts.extend(self.resource_writes.ones()); } -if self.writes_all_resources { + if self.writes_all_resources { conflicts.extend(other.resource_read_and_writes.ones()); }