diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index bf8717dbe209f..9a9a533da09b5 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -74,6 +74,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, @@ -1389,6 +1390,26 @@ 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() { + let mut world = World::new(); + 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 07fb201a5b979..d90ff04bfedaa 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 bevy_utils::HashSet; use core::fmt; use fixedbitset::FixedBitSet; +use std::fmt::Debug; use std::marker::PhantomData; /// A wrapper struct to make Debug representations of [`FixedBitSet`] easier @@ -484,15 +484,19 @@ 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) -> AccessConflicts { + let mut conflicts = FixedBitSet::new(); if self.reads_all_components { - // QUESTION: How to handle `other.writes_all`? + if other.writes_all_components { + return AccessConflicts::All; + } conflicts.extend(other.component_writes.ones()); } if other.reads_all_components { - // QUESTION: How to handle `self.writes_all`. + if self.writes_all_components { + return AccessConflicts::All; + } conflicts.extend(self.component_writes.ones()); } @@ -504,15 +508,18 @@ 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_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_resources { + return AccessConflicts::All; + } conflicts.extend(self.resource_writes.ones()); } - if self.writes_all_resources { conflicts.extend(other.resource_read_and_writes.ones()); } @@ -537,10 +544,7 @@ impl Access { self.resource_read_and_writes .intersection(&other.resource_writes), ); - conflicts - .ones() - .map(SparseSetIndex::get_sparse_set_index) - .collect() + AccessConflicts::Individual(conflicts) } /// Returns the indices of the components this has access to. @@ -635,6 +639,53 @@ impl From> for FilteredAccessSet { } } +/// Records how two accesses conflict with each other +#[derive(Debug, PartialEq)] +pub enum AccessConflicts { + /// Conflict is for all indices + All, + /// There is a conflict for a subset of indices + Individual(FixedBitSet), +} + +impl AccessConflicts { + fn add(&mut self, other: &Self) { + match (self, other) { + (s, AccessConflicts::All) => { + *s = AccessConflicts::All; + } + (AccessConflicts::Individual(this), AccessConflicts::Individual(other)) => { + this.extend(other.ones()); + } + _ => {} + } + } + + pub(crate) fn is_empty(&self) -> bool { + match self { + 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 AccessConflicts { + fn from(value: FixedBitSet) -> Self { + Self::Individual(value) + } +} + +impl From> for AccessConflicts { + fn from(value: Vec) -> Self { + Self::Individual(value.iter().map(T::sparse_set_index).collect()) + } +} + impl FilteredAccess { /// Returns a `FilteredAccess` which has no access and matches everything. /// This is the equivalent of a `TRUE` logic atom. @@ -752,12 +803,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) -> 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); } - Vec::new() + AccessConflicts::empty() } /// Adds all access and filters from `other`. @@ -948,29 +999,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) -> AccessConflicts { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = HashSet::new(); + 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 { - 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) -> AccessConflicts { // if the unfiltered access is incompatible, must check each pair - let mut conflicts = HashSet::new(); + let mut conflicts = AccessConflicts::empty(); 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. @@ -1030,7 +1081,7 @@ impl Default for FilteredAccessSet { #[cfg(test)] mod tests { use crate::query::access::AccessFilters; - use crate::query::{Access, FilteredAccess, FilteredAccessSet}; + use crate::query::{Access, AccessConflicts, FilteredAccess, FilteredAccessSet}; use fixedbitset::FixedBitSet; use std::marker::PhantomData; @@ -1195,21 +1246,27 @@ mod tests { access_b.add_component_read(0); access_b.add_component_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_component_write(0); access_c.add_component_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_component_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), AccessConflicts::empty()); + assert_eq!(access_d.get_conflicts(&access_b), AccessConflicts::empty()); + assert_eq!(access_d.get_conflicts(&access_c), vec![0_usize].into()); } #[test] @@ -1223,7 +1280,7 @@ mod tests { let conflicts = access_a.get_conflicts_single(&filter_b); assert_eq!( &conflicts, - &[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 c9dc06438f0cf..dad5d99c56f9e 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -23,6 +23,8 @@ use crate::{ world::World, }; +use crate::query::AccessConflicts; +use crate::storage::SparseSetIndex; pub use stepping::Stepping; /// Resource that stores [`Schedule`]s mapped to [`ScheduleLabel`]s excluding the current running [`Schedule`]. @@ -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) { + AccessConflicts::Individual(conflicts) => { + let conflicts: Vec<_> = conflicts + .ones() + .map(ComponentId::get_sparse_set_index) + .filter(|id| !ignored_ambiguities.contains(id)) + .collect(); + if !conflicts.is_empty() { + conflicting_systems.push((a, b, conflicts)); + } + } + 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/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 88faa30cdf4b8..bfb2dcd396cce 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) { @@ -340,10 +340,11 @@ impl std::ops::DerefMut for In { #[cfg(test)] mod tests { - use std::any::TypeId; - use bevy_utils::default; + use std::any::TypeId; + use crate::prelude::EntityRef; + use crate::world::EntityMut; use crate::{ self as bevy_ecs, archetype::{ArchetypeComponentId, Archetypes}, @@ -1102,7 +1103,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, vec![b_id, d_id].into()); } #[test] @@ -1608,6 +1609,33 @@ 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" + )] + 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() { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 1ae9f9a0ae00b..14a258784b908 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::AccessConflicts; +use crate::storage::SparseSetIndex; use crate::{ archetype::{Archetype, Archetypes}, bundle::Bundles, @@ -296,12 +298,22 @@ 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 { + AccessConflicts::All => "", + AccessConflicts::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.