diff --git a/_release-content/migration-guides/access_is_invertible.md b/_release-content/migration-guides/access_is_invertible.md new file mode 100644 index 0000000000000..13157a7782a86 --- /dev/null +++ b/_release-content/migration-guides/access_is_invertible.md @@ -0,0 +1,16 @@ +--- +title: "Reads and writes from `Access` are exposed" +pull_requests: [] +--- + +Removed from [`bevy_ecs::query::Access`] methods that gave `Result>`: + +- `try_reads_and_writes()` +- `try_writes()` + +Added new functions that return a new enum `InvertibleComponentIdSet`: + +- `reads_and_writes()` +- `writes()` + +The `try_` methods would fail for exclusion queries. diff --git a/crates/bevy_dev_tools/src/schedule_data/serde.rs b/crates/bevy_dev_tools/src/schedule_data/serde.rs index 05f1ef6161d74..feea65bad16f0 100644 --- a/crates/bevy_dev_tools/src/schedule_data/serde.rs +++ b/crates/bevy_dev_tools/src/schedule_data/serde.rs @@ -71,6 +71,10 @@ pub struct ScheduleData { pub conflicts: Vec, } +/// A newtype for the index of a component. +#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Copy, PartialOrd, Ord, Hash)] +pub struct ComponentIndex(pub u32); + /// Data about a component type. #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone)] pub struct ComponentData { @@ -89,7 +93,99 @@ pub struct SystemData { pub exclusive: bool, /// Whether this system has deferred buffers to apply. pub deferred: bool, - // TODO: Store the conditions specific to this system. + /// Combined access for the system, summarizes `Self::filtered_accesses` + pub combined_access: AccessData, + /// Filtered accesses for the system, generally 1x per system query + pub filtered_accesses: Vec, + // TODO: Store run conditions specific to this system. +} + +/// A serializable version of [`bevy_ecs::query::Access`]. +/// Tracks read and write access to specific components. +/// +/// All indexes are into [`ScheduleData::components`] +#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Default)] +pub struct AccessData { + /// All accessed components, or forbidden components if + /// `Self::read_and_writes_inverted` is set. + pub read_and_writes: Vec, + /// All exclusively-accessed components, or components that may not be + /// exclusively accessed if `Self::writes_inverted` is set. + pub writes: Vec, + /// Is `true` if this component can read all components *except* those + /// present in `Self::read_and_writes`. + pub read_and_writes_inverted: bool, + /// Is `true` if this component can write to all components *except* those + /// present in `Self::writes`. + pub writes_inverted: bool, + /// Components that are not accessed, but whose presence in an archetype affect query results. + pub archetypal: Vec, +} + +impl AccessData { + fn new(value: &bevy_ecs::query::Access, trace: &mut ComponentTrace) -> Self { + let read_and_writes = value.reads_and_writes(); + let writes = value.writes(); + + let read_and_writes_inverted = read_and_writes.inverted(); + let writes_inverted = writes.inverted(); + + Self { + read_and_writes: trace.get_indexes(read_and_writes.iter()), + writes: trace.get_indexes(writes.iter()), + read_and_writes_inverted, + writes_inverted, + archetypal: trace.get_indexes(value.archetypal().iter()), + } + } +} + +/// A serializable version of [`bevy_ecs::query::AccessFilters`]. +/// A clause in disjunctive normal form that filters entities by their components. +/// An [`AccessFiltersData`] matches entities that have *all* the components in the +/// `with` filters and *none* of the components in the `without` filters. +/// +/// All indexes are into [`ScheduleData::components`] +#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Default)] +pub struct AccessFiltersData { + /// The set of components that must all be present for this [`AccessFiltersData`] to match. + pub with: Vec, + /// The set of components that must all be absent for this [`AccessFiltersData`] to match. + pub without: Vec, +} + +/// A serializable version of [`bevy_ecs::query::FilteredAccess`] (docs copied from there). +/// Corresponds to a query in the system signature. +/// +/// All indexes are into [`ScheduleData::components`] +#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Default)] +pub struct FilteredAccessData { + /// The access of the Query (components that have read/writes) + pub access: AccessData, + /// Required components (summary of read/writes from `Self::access` ). + pub required: Vec, + /// An array of filter sets to express `With` or `Without` clauses. + /// Each filter set is `Or`-d together + pub filter_sets: Vec, +} + +impl FilteredAccessData { + fn new(value: &bevy_ecs::query::FilteredAccess, trace: &mut ComponentTrace) -> Self { + let access = AccessData::new(value.access(), trace); + + Self { + access, + required: trace.get_indexes(value.required().iter()), + filter_sets: value + .filter_sets() + .iter() + .map(|f| AccessFiltersData { + with: trace.get_indexes(f.with().iter()), + without: trace.get_indexes(f.without().iter()), + }) + .collect(), + } + } } /// Data about a particular system set. @@ -145,6 +241,45 @@ pub enum AccessConflict { #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Copy, PartialOrd, Ord)] pub struct SystemSetIndex(pub u32); +/// Helper to build the components used by systems in this schedule +struct ComponentTrace<'a> { + world_components: &'a Components, + component_id_to_index: HashMap, + components: Vec, +} + +impl<'a> ComponentTrace<'a> { + fn with_world_components(world_components: &'a Components) -> Self { + Self { + world_components, + component_id_to_index: HashMap::new(), + components: vec![], + } + } + + fn get_index(&mut self, id: ComponentId) -> u32 { + match self.component_id_to_index.entry(id) { + Entry::Occupied(entry) => *entry.get() as _, + Entry::Vacant(entry) => { + let component = self + .world_components + .get_info(id) + .expect("the component has already been registered by the system"); + + self.components.push(ComponentData { + name: format!("{}", component.name()), + }); + + *entry.insert(self.components.len() - 1) as _ + } + } + } + + fn get_indexes(&mut self, ids: impl Iterator) -> Vec { + ids.map(|id| self.get_index(id)).collect() + } +} + impl ScheduleData { /// Creates the data from the underlying [`Schedule`]. /// @@ -154,6 +289,8 @@ impl ScheduleData { world_components: &Components, build_metadata: Option<&ScheduleBuildMetadata>, ) -> Result { + let mut component_trace = ComponentTrace::with_world_components(world_components); + let graph = schedule.graph(); let mut system_key_to_index = HashMap::new(); @@ -169,14 +306,20 @@ impl ScheduleData { } let systems = schedule - .systems() + .systems_with_access() .map_err(|_| { ExtractAppDataError::ScheduleNotInitialized(format!("{:?}", schedule.label())) })? .enumerate() - .map(|(index, (key, system))| { + .map(|(index, (key, system_with_access))| { system_key_to_index.insert(key, index); + let system = system_with_access.system(); + let access = system_with_access.access(); + + let combined_access = access.combined_access(); + let filtered_accesses = access.filtered_accesses(); + let flags = system.flags(); SystemData { @@ -185,6 +328,11 @@ impl ScheduleData { == core::any::TypeId::of::(), exclusive: flags.contains(SystemStateFlags::EXCLUSIVE), deferred: flags.contains(SystemStateFlags::DEFERRED), + combined_access: AccessData::new(combined_access, &mut component_trace), + filtered_accesses: filtered_accesses + .iter() + .map(|fa| FilteredAccessData::new(fa, &mut component_trace)) + .collect(), } }) .collect(); @@ -258,9 +406,6 @@ impl ScheduleData { ); } - let mut component_id_to_index = HashMap::::new(); - let mut components = vec![]; - let conflicts = graph .conflicting_systems() .iter() @@ -280,28 +425,15 @@ impl ScheduleData { AccessConflict::World } else { AccessConflict::Components( - conflicts - .iter() - .map(|id| match component_id_to_index.entry(*id) { - Entry::Occupied(entry) => *entry.get() as _, - Entry::Vacant(entry) => { - let component = world_components.get_info(*id).expect( - "the component has already been registered by the system", - ); - - components.push(ComponentData { - name: format!("{}", component.name()), - }); - *entry.insert(components.len() - 1) as _ - } - }) - .collect(), + component_trace.get_indexes(conflicts.iter().copied()), ) }, } }) .collect(); + let components = component_trace.components; + Ok(Self { name: format!("{:?}", schedule.label()), components, @@ -337,8 +469,9 @@ pub mod tests { use bevy_platform::collections::HashMap; use crate::schedule_data::serde::{ - AccessConflict, AppData, ComponentData, ExtractAppDataError, ScheduleData, ScheduleIndex, - SystemConflict, SystemData, SystemSetData, SystemSetIndex, + AccessConflict, AccessData, AccessFiltersData, AppData, ComponentData, ExtractAppDataError, + FilteredAccessData, ScheduleData, ScheduleIndex, SystemConflict, SystemData, SystemSetData, + SystemSetIndex, }; fn app_data_from_app(app: &mut App) -> Result { @@ -465,6 +598,19 @@ pub mod tests { .unwrap() as u32; }; + let reindex_component_vec = |components: &mut Vec| { + for component in components.iter_mut() { + reindex_component(component); + } + components.sort(); + }; + + let reindex_access = |access: &mut AccessData| { + reindex_component_vec(&mut access.archetypal); + reindex_component_vec(&mut access.read_and_writes); + reindex_component_vec(&mut access.writes); + }; + // Sort the conditions in a system set. for set in schedule.system_sets.iter_mut() { set.conditions @@ -485,6 +631,21 @@ pub mod tests { } schedule.dependency.sort(); + // Reindex access in systems + for system in schedule.systems.iter_mut() { + reindex_access(&mut system.combined_access); + + for filtered_access in system.filtered_accesses.iter_mut() { + reindex_access(&mut filtered_access.access); + + reindex_component_vec(&mut filtered_access.required); + for filter_set in filtered_access.filter_sets.iter_mut() { + reindex_component_vec(&mut filter_set.with); + reindex_component_vec(&mut filter_set.without); + } + } + } + // Reindex the conflicts. for conflict in schedule.conflicts.iter_mut() { reindex_system(&mut conflict.system_1); @@ -517,6 +678,58 @@ pub mod tests { apply_deferred: false, exclusive: false, deferred: false, + combined_access: AccessData::default(), + filtered_accesses: vec![], + } + } + + /// Convenience to create a [`SystemData`] for more detailed case. + pub fn full_system( + name: &str, + needs: Vec, + writes: Vec, + filter: Option>, + reject: Option>, + ) -> SystemData { + // +1 on inputted components as 0 = Disabled + let offset_needs: Vec = needs.iter().map(|n| n + 1).collect(); + let offset_writes: Vec = writes.iter().map(|n| n + 1).collect(); + + let offset_filter: Vec = filter + .map(|f| f.iter().map(|n| n + 1).collect()) + .unwrap_or(offset_needs.clone()); + + let mut reject_filter: Vec = reject + .map(|f| f.iter().map(|n| n + 1).collect()) + .unwrap_or(vec![]); + reject_filter.insert(0, 0); + + SystemData { + name: name.into(), + apply_deferred: false, + exclusive: false, + deferred: false, + combined_access: AccessData { + read_and_writes: offset_needs.clone(), + writes: offset_writes.clone(), + read_and_writes_inverted: false, + writes_inverted: false, + archetypal: vec![], + }, + filtered_accesses: vec![FilteredAccessData { + access: AccessData { + read_and_writes: offset_needs.clone(), + writes: offset_writes.clone(), + read_and_writes_inverted: false, + writes_inverted: false, + archetypal: vec![], + }, + required: offset_needs.clone(), + filter_sets: vec![AccessFiltersData { + with: offset_filter.clone(), + without: reject_filter.clone(), + }], + }], } } @@ -704,18 +917,24 @@ pub mod tests { apply_deferred: false, exclusive: false, deferred: true, + combined_access: AccessData::default(), + filtered_accesses: vec![], }, SystemData { name: "a1".into(), apply_deferred: false, exclusive: false, deferred: true, + combined_access: AccessData::default(), + filtered_accesses: vec![], }, SystemData { name: "apply_deferred".into(), apply_deferred: true, exclusive: true, deferred: false, + combined_access: AccessData::default(), + filtered_accesses: vec![], }, simple_system("b0"), simple_system("b1"), @@ -812,21 +1031,39 @@ pub mod tests { assert_eq!(data.schedules.len(), 1); let schedule = &data.schedules[0]; assert_eq!(schedule.name, "Update"); + assert_eq!( + schedule.components, + [ + simple_component("Disabled"), + simple_component("MyComponent<0>"), + simple_component("MyComponent<1>"), + simple_component("MyComponent<2>"), + simple_component("MyComponent<3>"), + simple_component("MyComponent<4>"), + simple_component("MyComponent<5>"), + simple_component("MyComponent<6>"), + simple_component("MyComponent<7>"), + simple_component("MyComponent<8>"), + simple_component("MyComponent<9>"), + ] + ); + assert_eq!( schedule.systems, [ - simple_system("a0"), - simple_system("a1"), - simple_system("b0"), - simple_system("b1"), - simple_system("c0"), - simple_system("c1"), - simple_system("d0"), - simple_system("d1"), - simple_system("e0"), - simple_system("e1"), + full_system("a0", vec![0], vec![], None, None), + full_system("a1", vec![0], vec![], None, None), + full_system("b0", vec![1], vec![], None, None), + full_system("b1", vec![1], vec![1], None, None), + full_system("c0", vec![2, 3, 4, 5], vec![3], None, None), + full_system("c1", vec![2, 3, 4, 6], vec![2], None, None), + full_system("d0", vec![7], vec![7], Some(vec![7, 8]), None), + full_system("d1", vec![7], vec![7], None, Some(vec![8])), + full_system("e0", vec![9], vec![9], None, None), + full_system("e1", vec![9], vec![9], None, None), ] ); + assert_eq!( schedule.system_sets, [ @@ -864,19 +1101,15 @@ pub mod tests { (ScheduleIndex::System(8), ScheduleIndex::System(9)), ] ); - assert_eq!( - schedule.components, - [ - simple_component("MyComponent<1>"), - simple_component("MyComponent<2>"), - simple_component("MyComponent<3>"), - ] - ); assert_eq!( schedule.conflicts, [ - conflict(2, 3, AccessConflict::Components(vec![0])), - conflict(4, 5, AccessConflict::Components(vec![1, 2])) + // +1 on components for 0 = Disabled + + // b0, b1 conflict on 1 + conflict(2, 3, AccessConflict::Components(vec![2])), + // c0, c1 conflict on 2, 3 + conflict(4, 5, AccessConflict::Components(vec![3, 4])) ] ); } diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 3e00230bd5e56..a6d1860ec220e 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -407,30 +407,22 @@ impl Access { &self.archetypal } - /// Returns the set of components with read or write access, - /// or an error if the access is unbounded. - pub fn try_reads_and_writes(&self) -> Result<&ComponentIdSet, UnboundedAccessError> { + /// Returns the set of components with read or write access + pub fn reads_and_writes(&self) -> InvertibleComponentIdSet<'_> { // writes_inverted is only ever true when read_and_writes_inverted is // also true. Therefore it is sufficient to check just read_and_writes_inverted. if self.read_and_writes_inverted { - return Err(UnboundedAccessError { - writes_inverted: self.writes_inverted, - read_and_writes_inverted: self.read_and_writes_inverted, - }); + return InvertibleComponentIdSet::Excluded(&self.read_and_writes); } - Ok(&self.read_and_writes) + InvertibleComponentIdSet::Included(&self.read_and_writes) } - /// Returns the set of components with write access, - /// or an error if the access is unbounded. - pub fn try_writes(&self) -> Result<&ComponentIdSet, UnboundedAccessError> { + /// Returns the set of components with write access + pub fn writes(&self) -> InvertibleComponentIdSet<'_> { if self.writes_inverted { - return Err(UnboundedAccessError { - writes_inverted: self.writes_inverted, - read_and_writes_inverted: self.read_and_writes_inverted, - }); + return InvertibleComponentIdSet::Excluded(&self.writes); } - Ok(&self.writes) + InvertibleComponentIdSet::Included(&self.writes) } /// Returns an iterator over the component IDs and their [`ComponentAccessKind`]. @@ -466,7 +458,16 @@ impl Access { pub fn try_iter_access( &self, ) -> Result + '_, UnboundedAccessError> { - let reads_and_writes = self.try_reads_and_writes()?.iter().map(|index| { + let invertible_set = self.reads_and_writes(); + + let InvertibleComponentIdSet::Included(component_set) = invertible_set else { + return Err(UnboundedAccessError { + writes_inverted: self.writes_inverted, + read_and_writes_inverted: self.read_and_writes_inverted, + }); + }; + + let reads_and_writes = component_set.iter().map(|index| { if self.writes.contains(index) { ComponentAccessKind::Exclusive(index) } else { @@ -483,6 +484,34 @@ impl Access { } } +/// Either all or nothing in a [`ComponentIdSet`] +#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy)] +pub enum InvertibleComponentIdSet<'a> { + /// All components in the set + Included(&'a ComponentIdSet), + /// All components *not* in the set + Excluded(&'a ComponentIdSet), +} + +impl InvertibleComponentIdSet<'_> { + /// Returns true if this is Excluded, otherwise false + pub fn inverted(&self) -> bool { + match self { + InvertibleComponentIdSet::Included(_) => false, + InvertibleComponentIdSet::Excluded(_) => true, + } + } + + /// Iterate the underlying component ids + pub fn iter(&self) -> ComponentIdIter> { + match self { + InvertibleComponentIdSet::Included(b) | InvertibleComponentIdSet::Excluded(b) => { + b.iter() + } + } + } +} + /// Performs an in-place union of `other` into `self`, where either set may be inverted. /// /// Each set corresponds to a `FixedBitSet` if `inverted` is `false`, diff --git a/crates/bevy_ecs/src/schedule/node.rs b/crates/bevy_ecs/src/schedule/node.rs index b98815dc0b86d..e7db3e90a0dc5 100644 --- a/crates/bevy_ecs/src/schedule/node.rs +++ b/crates/bevy_ecs/src/schedule/node.rs @@ -55,6 +55,11 @@ impl SystemWithAccess { pub fn system(&self) -> &ScheduleSystem { &self.system } + + /// Returns the underlying [`FilteredAccessSet`] + pub fn access(&self) -> &FilteredAccessSet { + &self.access + } } impl System for SystemWithAccess { diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 9c851a1210461..1f9f4fc1d0bff 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -708,6 +708,28 @@ impl Schedule { Ok(iter) } + /// Returns an iterator over all systems with access in this schedule. + /// + /// Note: this method will return [`ScheduleNotInitialized`] if the + /// schedule has never been initialized or run. + pub fn systems_with_access( + &self, + ) -> Result + Sized, ScheduleNotInitialized> + { + if !self.executor_initialized { + return Err(ScheduleNotInitialized); + } + + let iter = self + .executable + .system_ids + .iter() + .zip(&self.executable.systems) + .map(|(&node_id, system)| (node_id, system)); + + Ok(iter) + } + /// Returns the number of systems in this schedule. pub fn systems_len(&self) -> usize { if !self.executor_initialized { diff --git a/crates/bevy_remote/src/builtin_methods.rs b/crates/bevy_remote/src/builtin_methods.rs index 2f10c3397d6a0..a7d22d5ef4810 100644 --- a/crates/bevy_remote/src/builtin_methods.rs +++ b/crates/bevy_remote/src/builtin_methods.rs @@ -2413,8 +2413,7 @@ mod tests { assert_eq!(res3.schedule_data.system_sets.len(), 6); assert_eq!(res3.schedule_data.hierarchy.len(), 6); assert_eq!(res3.schedule_data.dependency.len(), 4); - // Components are only currently recorded for conflicts - this may change in the future. - assert_eq!(res3.schedule_data.components.len(), 0); + assert_eq!(res3.schedule_data.components.len(), 2); assert_eq!(res3.schedule_data.conflicts.len(), 0); }