From fbfee24e1921dcc28f2b2fba4becac82fd861ebc Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 11:08:02 -0700 Subject: [PATCH 1/3] rebase - add `MaybeUnsafeCell` (in the future `&T -> &UnsafeCell` may be OK) - add `WorldAccessLevel` - add concept of "all exclusive access" to `Access` - update system_param.rs - update system.rs - update function_system.rs - update system_chaining.rs - remove ExclusiveSystem* internals - add tests - fix stage tests - in ambiguity_detection, the else branch in find_ambiguities is no longer taken because exclusive systems have a component access now. And obv two empty systems will always be compatible. - update fixed_timestep.rs - update extract_param.rs Co-Authored-By: Mike --- .../bevy_ecs/scheduling/run_criteria.rs | 2 +- crates/bevy_animation/src/lib.rs | 2 +- crates/bevy_app/src/app.rs | 4 +- crates/bevy_ecs/macros/src/lib.rs | 32 +- crates/bevy_ecs/src/entity/mod.rs | 2 +- crates/bevy_ecs/src/lib.rs | 11 +- crates/bevy_ecs/src/query/access.rs | 80 +++- crates/bevy_ecs/src/schedule/executor.rs | 10 +- .../src/schedule/executor_parallel.rs | 15 +- crates/bevy_ecs/src/schedule/stage.rs | 422 ++++++++---------- .../bevy_ecs/src/schedule/system_container.rs | 92 +--- .../src/schedule/system_descriptor.rs | 347 ++++++-------- crates/bevy_ecs/src/schedule/system_set.rs | 15 +- .../src/system/commands/parallel_scope.rs | 10 +- .../bevy_ecs/src/system/exclusive_system.rs | 177 -------- crates/bevy_ecs/src/system/function_system.rs | 76 +++- crates/bevy_ecs/src/system/mod.rs | 95 ++-- crates/bevy_ecs/src/system/system.rs | 8 +- crates/bevy_ecs/src/system/system_chaining.rs | 8 +- crates/bevy_ecs/src/system/system_param.rs | 290 ++++++++++-- crates/bevy_gilrs/src/lib.rs | 2 +- crates/bevy_input/src/lib.rs | 2 +- crates/bevy_pbr/src/lib.rs | 4 +- crates/bevy_pbr/src/material.rs | 2 +- crates/bevy_ptr/src/lib.rs | 59 +++ crates/bevy_render/src/extract_param.rs | 10 +- crates/bevy_render/src/lib.rs | 2 +- crates/bevy_scene/src/lib.rs | 7 +- crates/bevy_sprite/src/lib.rs | 2 +- crates/bevy_sprite/src/mesh2d/material.rs | 2 +- crates/bevy_text/src/lib.rs | 2 +- crates/bevy_time/src/fixed_timestep.rs | 8 +- crates/bevy_time/src/lib.rs | 5 +- crates/bevy_ui/src/lib.rs | 2 +- crates/bevy_window/src/lib.rs | 2 +- examples/ecs/ecs_guide.rs | 8 +- examples/scene/scene.rs | 2 +- 37 files changed, 948 insertions(+), 871 deletions(-) delete mode 100644 crates/bevy_ecs/src/system/exclusive_system.rs diff --git a/benches/benches/bevy_ecs/scheduling/run_criteria.rs b/benches/benches/bevy_ecs/scheduling/run_criteria.rs index 7fa1cb1adbb01..b8b5b834a456a 100644 --- a/benches/benches/bevy_ecs/scheduling/run_criteria.rs +++ b/benches/benches/bevy_ecs/scheduling/run_criteria.rs @@ -1,6 +1,6 @@ use bevy_ecs::{ component::Component, - prelude::{ParallelSystemDescriptorCoercion, Res, Resource, RunCriteriaDescriptorCoercion}, + prelude::{IntoSystemDescriptor, Res, Resource, RunCriteriaDescriptorCoercion}, schedule::{RunCriteriaLabel, ShouldRun, Stage, SystemStage}, system::Query, world::World, diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index ee70d801ef952..8dbf71e7faadc 100644 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -12,7 +12,7 @@ use bevy_ecs::{ entity::Entity, prelude::Component, reflect::ReflectComponent, - schedule::ParallelSystemDescriptorCoercion, + schedule::IntoSystemDescriptor, system::{Query, Res}, }; use bevy_hierarchy::Children; diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 428dac0fd54ca..00c43bb0aa575 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -3,7 +3,7 @@ pub use bevy_derive::AppLabel; use bevy_derive::{Deref, DerefMut}; use bevy_ecs::{ event::{Event, Events}, - prelude::{FromWorld, IntoExclusiveSystem}, + prelude::FromWorld, schedule::{ IntoSystemDescriptor, Schedule, ShouldRun, Stage, StageLabel, State, StateData, SystemSet, SystemStage, @@ -84,7 +84,7 @@ impl Default for App { app.add_default_stages() .add_event::() - .add_system_to_stage(CoreStage::Last, World::clear_trackers.exclusive_system()); + .add_system_to_stage(CoreStage::Last, World::clear_trackers.at_end()); #[cfg(feature = "bevy_ci_testing")] { diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index ed4391d7e0041..b70ec097f3f8c 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -227,6 +227,30 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { fn apply(&mut self, world: &mut World) { self.0.apply(world) } + + fn world_access_level() -> WorldAccessLevel { + let mut exclusive = false; + let mut shared = false; + #( + match #param_fetch::world_access_level() { + WorldAccessLevel::Exclusive => { + exclusive = true; + } + WorldAccessLevel::Shared => { + shared = true; + } + WorldAccessLevel::None => (), + } + )* + + if exclusive { + WorldAccessLevel::Exclusive + } else if shared { + WorldAccessLevel::Shared + } else { + WorldAccessLevel::None + } + } } @@ -239,7 +263,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: MaybeUnsafeCell<'w, World>, change_tick: u32, ) -> Self::Item { ParamSet { @@ -377,6 +401,10 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { fn apply(&mut self, world: &mut #path::world::World) { self.state.apply(world) } + + fn world_access_level() -> #path::system::WorldAccessLevel { + TSystemParamState::world_access_level() + } } impl #impl_generics #path::system::SystemParamFetch<'w, 's> for FetchState <(#(<#field_types as #path::system::SystemParam>::Fetch,)*), #punctuated_generic_idents> #where_clause { @@ -384,7 +412,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { unsafe fn get_param( state: &'s mut Self, system_meta: &#path::system::SystemMeta, - world: &'w #path::world::World, + world: #path::system::MaybeUnsafeCell<'w, #path::world::World>, change_tick: u32, ) -> Self::Item { #struct_name { diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index b4f54bac3637a..1c56e1b40e15b 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -79,7 +79,7 @@ type IdCursor = isize; /// } /// # /// # bevy_ecs::system::assert_is_system(setup); -/// # bevy_ecs::system::IntoExclusiveSystem::exclusive_system(exclusive_system); +/// # bevy_ecs::system::assert_is_system(exclusive_system); /// ``` /// /// It can be used to refer to a specific entity to apply [`EntityCommands`], or to call [`Query::get`] (or similar methods) to access its components. diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 102df951f89a0..56f9e938e5864 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -34,14 +34,13 @@ pub mod prelude { event::{EventReader, EventWriter, Events}, query::{Added, AnyOf, ChangeTrackers, Changed, Or, QueryState, With, Without}, schedule::{ - ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, RunCriteria, - RunCriteriaDescriptorCoercion, RunCriteriaLabel, Schedule, Stage, StageLabel, State, - SystemLabel, SystemSet, SystemStage, + IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel, + Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, SystemStage, }, system::{ - adapter as system_adapter, Commands, In, IntoChainSystem, IntoExclusiveSystem, - IntoSystem, Local, NonSend, NonSendMut, ParallelCommands, ParamSet, Query, - RemovedComponents, Res, ResMut, Resource, System, SystemParamFunction, + adapter as system_adapter, Commands, In, IntoChainSystem, IntoSystem, Local, NonSend, + NonSendMut, ParallelCommands, ParamSet, Query, RemovedComponents, Res, ResMut, + Resource, System, SystemParamFunction, }, world::{FromWorld, Mut, World}, }; diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index d9580bd4eb507..a4c47f5b48cca 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -13,9 +13,12 @@ pub struct Access { reads_and_writes: FixedBitSet, /// The exclusively-accessed elements. writes: FixedBitSet, - /// Is `true` if this has access to all elements in the collection? + /// Is `true` if this has access to all elements in the collection. /// This field is a performance optimization for `&World` (also harder to mess up for soundness). reads_all: bool, + /// Is `true` if this has exclusive access to all elements in the collection. + /// This field is a performance optimization for `&mut World` (also harder to mess up for soundness). + writes_all: bool, marker: PhantomData, } @@ -23,6 +26,7 @@ impl Default for Access { fn default() -> Self { Self { reads_all: false, + writes_all: false, reads_and_writes: Default::default(), writes: Default::default(), marker: PhantomData, @@ -64,7 +68,11 @@ impl Access { /// Returns `true` if this can exclusively access the element given by `index`. pub fn has_write(&self, index: T) -> bool { - self.writes.contains(index.sparse_set_index()) + if self.writes_all { + true + } else { + self.writes.contains(index.sparse_set_index()) + } } /// Sets this as having access to all indexed elements (i.e. `&World`). @@ -77,9 +85,21 @@ impl Access { self.reads_all } + /// Sets this as having exclusive access to all indexed elements (i.e. `&mut World`). + pub fn write_all(&mut self) { + self.reads_all = true; + self.writes_all = true; + } + + /// Returns `true` if this has exclusive access to all indexed elements (i.e. `&mut World`). + pub fn has_write_all(&self) -> bool { + self.writes_all + } + /// Removes all accesses. pub fn clear(&mut self) { self.reads_all = false; + self.writes_all = false; self.reads_and_writes.clear(); self.writes.clear(); } @@ -87,6 +107,7 @@ impl Access { /// Adds all access from `other`. pub fn extend(&mut self, other: &Access) { self.reads_all = self.reads_all || other.reads_all; + self.writes_all = self.writes_all || other.writes_all; self.reads_and_writes.union_with(&other.reads_and_writes); self.writes.union_with(&other.writes); } @@ -96,6 +117,12 @@ impl Access { /// `Access` instances are incompatible if one can write /// an element that the other can read or write. pub fn is_compatible(&self, other: &Access) -> bool { + // All systems make a `&World` reference before running to update change detection info. + // Since exclusive systems produce a `&mut World`, we cannot let other systems run. + if self.writes_all || other.writes_all { + return false; + } + // Only systems that do not write data are compatible with systems that operate on `&World`. if self.reads_all { return other.writes.count_ones(..) == 0; @@ -112,15 +139,31 @@ 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(); - if self.reads_all { - conflicts.extend(other.writes.ones()); + + if self.writes_all { + conflicts.extend(other.reads_and_writes.ones()); } - if other.reads_all { - conflicts.extend(self.writes.ones()); + if other.writes_all { + conflicts.extend(self.reads_and_writes.ones()); + } + + if !(self.writes_all || other.writes_all) { + match (self.reads_all, other.reads_all) { + (false, false) => { + conflicts.extend(self.writes.intersection(&other.reads_and_writes)); + conflicts.extend(self.reads_and_writes.intersection(&other.writes)); + } + (false, true) => { + conflicts.extend(self.writes.ones()); + } + (true, false) => { + conflicts.extend(other.writes.ones()); + } + (true, true) => (), + } } - 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) @@ -266,6 +309,11 @@ impl FilteredAccess { pub fn read_all(&mut self) { self.access.read_all(); } + + /// Sets the underlying unfiltered access as having exclusive access to all indexed elements. + pub fn write_all(&mut self) { + self.access.write_all(); + } } /// A collection of [`FilteredAccess`] instances. @@ -306,6 +354,20 @@ impl FilteredAccessSet { true } + /// Returns `true` if this and `filtered_access` can be active at the same time. + pub fn is_compatible_single(&self, filtered_access: &FilteredAccess) -> bool { + if self.combined_access.is_compatible(filtered_access.access()) { + return true; + } + for filtered in &self.filtered_accesses { + if !filtered.is_compatible(filtered_access) { + return false; + } + } + + true + } + /// Returns a vector of elements that this set and `other` cannot access at the same time. pub fn get_conflicts(&self, other: &FilteredAccessSet) -> Vec { // if the unfiltered access is incompatible, must check each pair @@ -320,7 +382,7 @@ impl FilteredAccessSet { conflicts.into_iter().collect() } - /// Returns a vector of elements that this set and `other` cannot access at the same time. + /// Returns a vector of elements that this set and `filtered_access` cannot access at the same time. pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> Vec { // if the unfiltered access is incompatible, must check each pair let mut conflicts = HashSet::new(); diff --git a/crates/bevy_ecs/src/schedule/executor.rs b/crates/bevy_ecs/src/schedule/executor.rs index 06db889ec2d6c..edf6fcf5c6ab3 100644 --- a/crates/bevy_ecs/src/schedule/executor.rs +++ b/crates/bevy_ecs/src/schedule/executor.rs @@ -1,11 +1,11 @@ -use crate::{schedule::ParallelSystemContainer, world::World}; +use crate::{schedule::FunctionSystemContainer, world::World}; use downcast_rs::{impl_downcast, Downcast}; pub trait ParallelSystemExecutor: Downcast + Send + Sync { /// Called by `SystemStage` whenever `systems` have been changed. - fn rebuild_cached_data(&mut self, systems: &[ParallelSystemContainer]); + fn rebuild_cached_data(&mut self, systems: &[FunctionSystemContainer]); - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World); + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World); } impl_downcast!(ParallelSystemExecutor); @@ -14,9 +14,9 @@ impl_downcast!(ParallelSystemExecutor); pub struct SingleThreadedExecutor; impl ParallelSystemExecutor for SingleThreadedExecutor { - fn rebuild_cached_data(&mut self, _: &[ParallelSystemContainer]) {} + fn rebuild_cached_data(&mut self, _: &[FunctionSystemContainer]) {} - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World) { + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World) { for system in systems { if system.should_run() { #[cfg(feature = "trace")] diff --git a/crates/bevy_ecs/src/schedule/executor_parallel.rs b/crates/bevy_ecs/src/schedule/executor_parallel.rs index c0eb24a2bae56..e0b34434ffb24 100644 --- a/crates/bevy_ecs/src/schedule/executor_parallel.rs +++ b/crates/bevy_ecs/src/schedule/executor_parallel.rs @@ -1,7 +1,8 @@ use crate::{ archetype::ArchetypeComponentId, query::Access, - schedule::{ParallelSystemContainer, ParallelSystemExecutor}, + schedule::{FunctionSystemContainer, ParallelSystemExecutor}, + system::MaybeUnsafeCell, world::World, }; use async_channel::{Receiver, Sender}; @@ -77,7 +78,7 @@ impl Default for ParallelExecutor { } impl ParallelSystemExecutor for ParallelExecutor { - fn rebuild_cached_data(&mut self, systems: &[ParallelSystemContainer]) { + fn rebuild_cached_data(&mut self, systems: &[FunctionSystemContainer]) { self.system_metadata.clear(); self.queued.grow(systems.len()); self.running.grow(systems.len()); @@ -85,6 +86,10 @@ impl ParallelSystemExecutor for ParallelExecutor { // Construct scheduling data for systems. for container in systems { + if container.system().is_exclusive() { + panic!("executor cannot run exclusive systems"); + } + let dependencies_total = container.dependencies().len(); let system = container.system(); self.system_metadata.push(SystemSchedulingMetadata { @@ -104,7 +109,7 @@ impl ParallelSystemExecutor for ParallelExecutor { } } - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World) { + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World) { #[cfg(test)] if self.events_sender.is_none() { let (sender, receiver) = async_channel::unbounded::(); @@ -167,7 +172,7 @@ impl ParallelExecutor { fn prepare_systems<'scope>( &mut self, scope: &mut Scope<'scope, ()>, - systems: &'scope mut [ParallelSystemContainer], + systems: &'scope mut [FunctionSystemContainer], world: &'scope World, ) { // These are used as a part of a unit test. @@ -215,7 +220,7 @@ impl ParallelExecutor { #[cfg(feature = "trace")] let _system_guard = system_span.enter(); // SAFETY: the executor prevents two systems with conflicting access from running simultaneously. - unsafe { system.run_unsafe((), world) }; + unsafe { system.run_unsafe((), MaybeUnsafeCell::from_ref(world)) }; }; if can_start { diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index 414ebb5e615fa..103c34d35c777 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -5,11 +5,11 @@ use crate::{ prelude::IntoSystem, schedule::{ graph_utils::{self, DependencyGraphError}, - BoxedRunCriteria, DuplicateLabelStrategy, ExclusiveSystemContainer, GraphNode, - InsertionPoint, ParallelExecutor, ParallelSystemContainer, ParallelSystemExecutor, - RunCriteriaContainer, RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, - RunCriteriaInner, RunCriteriaLabelId, ShouldRun, SingleThreadedExecutor, SystemContainer, - SystemDescriptor, SystemLabelId, SystemSet, + BoxedRunCriteria, DuplicateLabelStrategy, FunctionSystemContainer, GraphNode, + InsertionPoint, ParallelExecutor, ParallelSystemExecutor, RunCriteriaContainer, + RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, RunCriteriaInner, RunCriteriaLabelId, + ShouldRun, SingleThreadedExecutor, SystemContainer, SystemDescriptor, SystemLabelId, + SystemSet, SystemType, }, world::{World, WorldId}, }; @@ -62,14 +62,14 @@ pub struct SystemStage { /// Topologically sorted run criteria of systems. run_criteria: Vec, /// Topologically sorted exclusive systems that want to be run at the start of the stage. - pub(super) exclusive_at_start: Vec, + pub(super) exclusive_at_start: Vec, /// Topologically sorted exclusive systems that want to be run after parallel systems but /// before the application of their command buffers. - pub(super) exclusive_before_commands: Vec, + pub(super) exclusive_before_commands: Vec, /// Topologically sorted exclusive systems that want to be run at the end of the stage. - pub(super) exclusive_at_end: Vec, + pub(super) exclusive_at_end: Vec, /// Topologically sorted parallel systems. - pub(super) parallel: Vec, + pub(super) parallel: Vec, /// Determines if the stage was modified and needs to rebuild its graphs and orders. pub(super) systems_modified: bool, /// Determines if the stage's executor was changed. @@ -77,13 +77,7 @@ pub struct SystemStage { /// Newly inserted run criteria that will be initialized at the next opportunity. uninitialized_run_criteria: Vec<(usize, DuplicateLabelStrategy)>, /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_at_start: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_before_commands: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_at_end: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_parallel: Vec, + uninitialized_systems: Vec<(FunctionSystemContainer, SystemType)>, /// Saves the value of the World change_tick during the last tick check last_tick_check: u32, /// If true, buffers will be automatically applied at the end of the stage. If false, buffers must be manually applied. @@ -99,16 +93,13 @@ impl SystemStage { stage_run_criteria: Default::default(), run_criteria: vec![], uninitialized_run_criteria: vec![], + uninitialized_systems: vec![], exclusive_at_start: Default::default(), exclusive_before_commands: Default::default(), exclusive_at_end: Default::default(), parallel: vec![], systems_modified: true, executor_modified: true, - uninitialized_parallel: vec![], - uninitialized_at_start: vec![], - uninitialized_before_commands: vec![], - uninitialized_at_end: vec![], last_tick_check: Default::default(), apply_buffers: true, must_read_resource: None, @@ -156,64 +147,29 @@ impl SystemStage { self } - fn add_system_inner(&mut self, system: SystemDescriptor, default_run_criteria: Option) { + fn add_system_inner( + &mut self, + mut descriptor: SystemDescriptor, + default_run_criteria: Option, + ) { self.systems_modified = true; - match system { - SystemDescriptor::Exclusive(mut descriptor) => { - let insertion_point = descriptor.insertion_point; - let criteria = descriptor.run_criteria.take(); - let mut container = ExclusiveSystemContainer::from_descriptor(descriptor); - match criteria { - Some(RunCriteriaDescriptorOrLabel::Label(label)) => { - container.run_criteria_label = Some(label); - } - Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { - container.run_criteria_label = criteria_descriptor.label; - container.run_criteria_index = - Some(self.add_run_criteria_internal(criteria_descriptor)); - } - None => { - container.run_criteria_index = default_run_criteria; - } - } - match insertion_point { - InsertionPoint::AtStart => { - let index = self.exclusive_at_start.len(); - self.uninitialized_at_start.push(index); - self.exclusive_at_start.push(container); - } - InsertionPoint::BeforeCommands => { - let index = self.exclusive_before_commands.len(); - self.uninitialized_before_commands.push(index); - self.exclusive_before_commands.push(container); - } - InsertionPoint::AtEnd => { - let index = self.exclusive_at_end.len(); - self.uninitialized_at_end.push(index); - self.exclusive_at_end.push(container); - } - } + let system_type = descriptor.system_type; + let criteria = descriptor.run_criteria.take(); + let mut container = FunctionSystemContainer::from_descriptor(descriptor); + match criteria { + Some(RunCriteriaDescriptorOrLabel::Label(label)) => { + container.run_criteria_label = Some(label); } - SystemDescriptor::Parallel(mut descriptor) => { - let criteria = descriptor.run_criteria.take(); - let mut container = ParallelSystemContainer::from_descriptor(descriptor); - match criteria { - Some(RunCriteriaDescriptorOrLabel::Label(label)) => { - container.run_criteria_label = Some(label); - } - Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { - container.run_criteria_label = criteria_descriptor.label; - container.run_criteria_index = - Some(self.add_run_criteria_internal(criteria_descriptor)); - } - None => { - container.run_criteria_index = default_run_criteria; - } - } - self.uninitialized_parallel.push(self.parallel.len()); - self.parallel.push(container); + Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { + container.run_criteria_label = criteria_descriptor.label; + container.run_criteria_index = + Some(self.add_run_criteria_internal(criteria_descriptor)); + } + None => { + container.run_criteria_index = default_run_criteria; } } + self.uninitialized_systems.push((container, system_type)); } pub fn apply_buffers(&mut self, world: &mut World) { @@ -267,53 +223,41 @@ impl SystemStage { pub fn add_system_set(&mut self, system_set: SystemSet) -> &mut Self { self.systems_modified = true; - let (run_criteria, mut systems) = system_set.bake(); - let set_run_criteria_index = run_criteria.and_then(|criteria| { - // validate that no systems have criteria - for system in &mut systems { - if let Some(name) = match system { - SystemDescriptor::Exclusive(descriptor) => descriptor - .run_criteria - .is_some() - .then(|| descriptor.system.name()), - SystemDescriptor::Parallel(descriptor) => descriptor - .run_criteria - .is_some() - .then(|| descriptor.system.name()), - } { - panic!( - "The system {} has a run criteria, but its `SystemSet` also has a run \ - criteria. This is not supported. Consider moving the system into a \ - different `SystemSet` or calling `add_system()` instead.", - name - ) - } + let (run_criteria, mut descriptors) = system_set.bake(); + // verify that none of the systems in the set have their own run criteria + for descriptor in &descriptors { + if let Some(name) = descriptor + .run_criteria + .as_ref() + .map(|_| descriptor.system.name()) + { + panic!( + "The system {} has a run criteria, but its `SystemSet` also has a run \ + criteria. This is not supported. Consider moving the system into a \ + different `SystemSet` or calling `add_system()` instead.", + name + ) } - match criteria { - RunCriteriaDescriptorOrLabel::Descriptor(descriptor) => { - Some(self.add_run_criteria_internal(descriptor)) - } - RunCriteriaDescriptorOrLabel::Label(label) => { - for system in &mut systems { - match system { - SystemDescriptor::Exclusive(descriptor) => { - descriptor.run_criteria = - Some(RunCriteriaDescriptorOrLabel::Label(label)); - } - SystemDescriptor::Parallel(descriptor) => { - descriptor.run_criteria = - Some(RunCriteriaDescriptorOrLabel::Label(label)); - } - } - } + } - None + // add system set run criteria + let set_run_criteria_index = run_criteria.and_then(|criteria| match criteria { + RunCriteriaDescriptorOrLabel::Descriptor(descriptor) => { + Some(self.add_run_criteria_internal(descriptor)) + } + RunCriteriaDescriptorOrLabel::Label(label) => { + for descriptor in &mut descriptors { + descriptor.run_criteria = Some(RunCriteriaDescriptorOrLabel::Label(label)); } + None } }); - for system in systems.drain(..) { - self.add_system_inner(system, set_run_criteria_index); + + // set every system to use the set's run criteria + for descriptor in descriptors.drain(..) { + self.add_system_inner(descriptor, set_run_criteria_index); } + self } @@ -360,11 +304,11 @@ impl SystemStage { let mut criteria_labels = HashMap::default(); let uninitialized_criteria: HashMap<_, _> = self.uninitialized_run_criteria.drain(..).collect(); - // track the number of filtered criteria to correct run criteria indices + + // remove duplicate run criteria and point systems to the corrected run criteria indices let mut filtered_criteria = 0; let mut new_indices = Vec::new(); - self.run_criteria = self - .run_criteria + self.run_criteria = self.run_criteria .drain(..) .enumerate() .filter_map(|(index, mut container)| { @@ -375,9 +319,8 @@ impl SystemStage { if let Some(duplicate_index) = criteria_labels.get(label) { match strategy { DuplicateLabelStrategy::Panic => panic!( - "Run criteria {} is labelled with {:?}, which \ - is already in use. Consider using \ - `RunCriteriaDescriptorCoercion::label_discard_if_duplicate().", + "Run criteria {} has label {:?}, which is already in use. \ + Consider using `RunCriteriaDescriptorCoercion::label_discard_if_duplicate().", container.name(), container.label ), @@ -399,33 +342,31 @@ impl SystemStage { }) .collect(); - for index in self.uninitialized_at_start.drain(..) { - let container = &mut self.exclusive_at_start[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); - } - container.system_mut().initialize(world); - } - for index in self.uninitialized_before_commands.drain(..) { - let container = &mut self.exclusive_before_commands[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); - } + // initialize the systems and sort them into the appropriate vectors + for (mut container, system_type) in self.uninitialized_systems.drain(..) { container.system_mut().initialize(world); - } - for index in self.uninitialized_at_end.drain(..) { - let container = &mut self.exclusive_at_end[index]; + if let Some(index) = container.run_criteria() { container.set_run_criteria(new_indices[index]); } - container.system_mut().initialize(world); - } - for index in self.uninitialized_parallel.drain(..) { - let container = &mut self.parallel[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); + + match system_type { + SystemType::Parallel => { + assert!(!container.system().is_exclusive()); + self.parallel.push(container); + } + SystemType::Exclusive(insertion_point) => match insertion_point { + InsertionPoint::AtStart => { + self.exclusive_at_start.push(container); + } + InsertionPoint::BeforeCommands => { + self.exclusive_before_commands.push(container); + } + InsertionPoint::AtEnd => { + self.exclusive_at_end.push(container); + } + }, } - container.system_mut().initialize(world); } } @@ -442,11 +383,7 @@ impl SystemStage { < (CHECK_TICK_THRESHOLD as usize) ); debug_assert!( - self.uninitialized_run_criteria.is_empty() - && self.uninitialized_parallel.is_empty() - && self.uninitialized_at_start.is_empty() - && self.uninitialized_before_commands.is_empty() - && self.uninitialized_at_end.is_empty() + self.uninitialized_run_criteria.is_empty() && self.uninitialized_systems.is_empty() ); fn unwrap_dependency_cycle_error( result: Result>, @@ -717,13 +654,25 @@ impl Stage for SystemStage { // Run systems that want to be at the start of stage. for container in &mut self.exclusive_at_start { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -738,13 +687,25 @@ impl Stage for SystemStage { // Run systems that want to be between parallel systems and their command buffers. for container in &mut self.exclusive_before_commands { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -766,13 +727,25 @@ impl Stage for SystemStage { // Run systems that want to be at the end of stage. for container in &mut self.exclusive_at_end { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -826,11 +799,10 @@ mod tests { use crate::{ schedule::{ - ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, RunCriteria, - RunCriteriaDescriptorCoercion, ShouldRun, SingleThreadedExecutor, Stage, SystemLabel, - SystemSet, SystemStage, + IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, ShouldRun, + SingleThreadedExecutor, Stage, SystemLabel, SystemLabelId, SystemSet, SystemStage, }, - system::{In, IntoExclusiveSystem, Local, Query, ResMut}, + system::{In, Local, Query, ResMut}, world::World, }; @@ -868,10 +840,10 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(0).exclusive_system().at_start()) + .with_system(make_exclusive(0).at_start()) .with_system(make_parallel(1)) - .with_system(make_exclusive(2).exclusive_system().before_commands()) - .with_system(make_exclusive(3).exclusive_system().at_end()); + .with_system(make_exclusive(2).before_commands()) + .with_system(make_exclusive(3).at_end()); stage.run(&mut world); assert_eq!(world.resource_mut::().0, vec![0, 1, 2, 3]); stage.set_executor(Box::new(SingleThreadedExecutor::default())); @@ -883,10 +855,10 @@ mod tests { world.resource_mut::().0.clear(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(2).exclusive_system().before_commands()) - .with_system(make_exclusive(3).exclusive_system().at_end()) + .with_system(make_exclusive(2).before_commands()) + .with_system(make_exclusive(3).at_end()) .with_system(make_parallel(1)) - .with_system(make_exclusive(0).exclusive_system().at_start()); + .with_system(make_exclusive(0).at_start()); stage.run(&mut world); assert_eq!(world.resource::().0, vec![0, 1, 2, 3]); stage.set_executor(Box::new(SingleThreadedExecutor::default())); @@ -898,10 +870,10 @@ mod tests { world.resource_mut::().0.clear(); let mut stage = SystemStage::parallel() - .with_system(make_parallel(2).exclusive_system().before_commands()) - .with_system(make_parallel(3).exclusive_system().at_end()) + .with_system(make_parallel(2).before_commands()) + .with_system(make_parallel(3).at_end()) .with_system(make_parallel(1)) - .with_system(make_parallel(0).exclusive_system().at_start()); + .with_system(make_parallel(0).at_start()); stage.run(&mut world); assert_eq!(world.resource::().0, vec![0, 1, 2, 3]); stage.set_executor(Box::new(SingleThreadedExecutor::default())); @@ -930,9 +902,9 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(1).exclusive_system().label(L1).after(L0)) - .with_system(make_exclusive(2).exclusive_system().after(L1)) - .with_system(make_exclusive(0).exclusive_system().label(L0)); + .with_system(make_exclusive(1).label(L1).after(L0)) + .with_system(make_exclusive(2).after(L1)) + .with_system(make_exclusive(0).label(L0)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -944,9 +916,9 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(1).exclusive_system().label(L1).before(L2)) - .with_system(make_exclusive(2).exclusive_system().label(L2)) - .with_system(make_exclusive(0).exclusive_system().before(L1)); + .with_system(make_exclusive(1).label(L1).before(L2)) + .with_system(make_exclusive(2).label(L2)) + .with_system(make_exclusive(0).before(L1)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -958,11 +930,11 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(2).exclusive_system().label(L2)) - .with_system(make_exclusive(1).exclusive_system().after(L0).before(L2)) - .with_system(make_exclusive(0).exclusive_system().label(L0)) - .with_system(make_exclusive(4).exclusive_system().label(L4)) - .with_system(make_exclusive(3).exclusive_system().after(L2).before(L4)); + .with_system(make_exclusive(2).label(L2)) + .with_system(make_exclusive(1).after(L0).before(L2)) + .with_system(make_exclusive(0).label(L0)) + .with_system(make_exclusive(4).label(L4)) + .with_system(make_exclusive(3).after(L2).before(L4)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -977,9 +949,9 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(1).exclusive_system().label(First).after(L0)) - .with_system(make_exclusive(2).exclusive_system().after(First)) - .with_system(make_exclusive(0).exclusive_system().label(First).label(L0)); + .with_system(make_exclusive(1).label(First).after(L0)) + .with_system(make_exclusive(2).after(First)) + .with_system(make_exclusive(0).label(First).label(L0)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -987,11 +959,11 @@ mod tests { world.resource_mut::().0.clear(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(2).exclusive_system().after(L01).label(L2)) - .with_system(make_exclusive(1).exclusive_system().label(L01).after(L0)) - .with_system(make_exclusive(0).exclusive_system().label(L01).label(L0)) - .with_system(make_exclusive(4).exclusive_system().label(L4)) - .with_system(make_exclusive(3).exclusive_system().after(L2).before(L4)); + .with_system(make_exclusive(2).after(L01).label(L2)) + .with_system(make_exclusive(1).label(L01).after(L0)) + .with_system(make_exclusive(0).label(L01).label(L0)) + .with_system(make_exclusive(4).label(L4)) + .with_system(make_exclusive(3).after(L2).before(L4)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -1002,13 +974,13 @@ mod tests { world.resource_mut::().0.clear(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(2).exclusive_system().label(L234).label(L2)) - .with_system(make_exclusive(1).exclusive_system().before(L234).after(L0)) - .with_system(make_exclusive(0).exclusive_system().label(L0)) - .with_system(make_exclusive(4).exclusive_system().label(L234).label(L4)) + .with_system(make_exclusive(2).label(L234).label(L2)) + .with_system(make_exclusive(1).before(L234).after(L0)) + .with_system(make_exclusive(0).label(L0)) + .with_system(make_exclusive(4).label(L234).label(L4)) .with_system( make_exclusive(3) - .exclusive_system() + .label(L234) .after(L2) .before(L4), @@ -1029,7 +1001,7 @@ mod tests { let mut stage = SystemStage::parallel() .with_system( make_exclusive(2) - .exclusive_system() + .label(L2) .after(L1) .before(L3) @@ -1037,17 +1009,17 @@ mod tests { ) .with_system( make_exclusive(1) - .exclusive_system() + .label(L1) .after(L0) .after(L0) .before(L2), ) - .with_system(make_exclusive(0).exclusive_system().label(L0).before(L1)) - .with_system(make_exclusive(4).exclusive_system().label(L4).after(L3)) + .with_system(make_exclusive(0).label(L0).before(L1)) + .with_system(make_exclusive(4).label(L4).after(L3)) .with_system( make_exclusive(3) - .exclusive_system() + .label(L3) .after(L2) .before(L4), @@ -1066,14 +1038,14 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(2).exclusive_system().label(L2)) + .with_system(make_exclusive(2).label(L2)) .with_system_set( SystemSet::new() - .with_system(make_exclusive(0).exclusive_system().label(L0)) - .with_system(make_exclusive(4).exclusive_system().label(L4)) - .with_system(make_exclusive(3).exclusive_system().after(L2).before(L4)), + .with_system(make_exclusive(0).label(L0)) + .with_system(make_exclusive(4).label(L4)) + .with_system(make_exclusive(3).after(L2).before(L4)), ) - .with_system(make_exclusive(1).exclusive_system().after(L0).before(L2)); + .with_system(make_exclusive(1).after(L0).before(L2)); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); stage.run(&mut world); @@ -1088,13 +1060,13 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(0).exclusive_system().before(L1)) + .with_system(make_exclusive(0).before(L1)) .with_system_set( SystemSet::new() .with_run_criteria(every_other_time) - .with_system(make_exclusive(1).exclusive_system().label(L1)), + .with_system(make_exclusive(1).label(L1)), ) - .with_system(make_exclusive(2).exclusive_system().after(L1)); + .with_system(make_exclusive(2).after(L1)); stage.run(&mut world); stage.run(&mut world); stage.set_executor(Box::new(SingleThreadedExecutor::default())); @@ -1112,7 +1084,7 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(0).exclusive_system().label(L0).after(L0)); + .with_system(make_exclusive(0).label(L0).after(L0)); stage.run(&mut world); } @@ -1122,8 +1094,8 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(0).exclusive_system().label(L0).after(L1)) - .with_system(make_exclusive(1).exclusive_system().label(L1).after(L0)); + .with_system(make_exclusive(0).label(L0).after(L1)) + .with_system(make_exclusive(1).label(L1).after(L0)); stage.run(&mut world); } @@ -1133,9 +1105,9 @@ mod tests { let mut world = World::new(); world.init_resource::(); let mut stage = SystemStage::parallel() - .with_system(make_exclusive(0).exclusive_system().label(L0)) - .with_system(make_exclusive(1).exclusive_system().after(L0).before(L2)) - .with_system(make_exclusive(2).exclusive_system().label(L2).before(L0)); + .with_system(make_exclusive(0).label(L0)) + .with_system(make_exclusive(1).after(L0).before(L2)) + .with_system(make_exclusive(2).label(L2).before(L0)); stage.run(&mut world); } diff --git a/crates/bevy_ecs/src/schedule/system_container.rs b/crates/bevy_ecs/src/schedule/system_container.rs index 2318d65ca7b13..6b86d11b65a5a 100644 --- a/crates/bevy_ecs/src/schedule/system_container.rs +++ b/crates/bevy_ecs/src/schedule/system_container.rs @@ -2,10 +2,9 @@ use crate::{ component::ComponentId, query::Access, schedule::{ - ExclusiveSystemDescriptor, GraphNode, ParallelSystemDescriptor, RunCriteriaLabelId, - SystemLabelId, + GraphNode, RunCriteriaLabelId, SystemDescriptor, SystemLabelId, }, - system::{ExclusiveSystem, System}, + system::System, }; use std::borrow::Cow; @@ -23,82 +22,7 @@ pub trait SystemContainer: GraphNode