diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 3b16ca0326840..26f65911bec8b 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -5,6 +5,7 @@ use crate::{ condition::{BoxedCondition, Condition}, graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo}, set::{InternedSystemSet, IntoSystemSet, SystemSet}, + Chain, }, system::{BoxedSystem, IntoSystem, System}, }; @@ -67,8 +68,8 @@ pub enum NodeConfigs { configs: Vec>, /// Run conditions applied to everything in the tuple. collective_conditions: Vec, - /// If `true`, adds `before -> after` ordering constraints between the successive elements. - chained: bool, + /// See [`Chain`] for usage. + chained: Chain, }, } @@ -137,6 +138,38 @@ impl NodeConfigs { } } + fn before_ignore_deferred_inner(&mut self, set: InternedSystemSet) { + match self { + Self::NodeConfig(config) => { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::BeforeNoSync, set)); + } + Self::Configs { configs, .. } => { + for config in configs { + config.before_ignore_deferred_inner(set.intern()); + } + } + } + } + + fn after_ignore_deferred_inner(&mut self, set: InternedSystemSet) { + match self { + Self::NodeConfig(config) => { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::AfterNoSync, set)); + } + Self::Configs { configs, .. } => { + for config in configs { + config.after_ignore_deferred_inner(set.intern()); + } + } + } + } + fn distributive_run_if_inner(&mut self, condition: impl Condition + Clone) { match self { Self::NodeConfig(config) => { @@ -198,7 +231,17 @@ impl NodeConfigs { match &mut self { Self::NodeConfig(_) => { /* no op */ } Self::Configs { chained, .. } => { - *chained = true; + *chained = Chain::Yes; + } + } + self + } + + fn chain_ignore_deferred_inner(mut self) -> Self { + match &mut self { + Self::NodeConfig(_) => { /* no op */ } + Self::Configs { chained, .. } => { + *chained = Chain::YesIgnoreDeferred; } } self @@ -252,7 +295,11 @@ where self.into_configs().in_set(set) } - /// Run before all systems in `set`. + /// Runs before all systems in `set`. If `self` has any systems that produce [`Commands`](crate::system::Commands) + /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `set` will see their effect. + /// + /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like + /// this isn't desired, use [`before_ignore_deferred`](Self::before_ignore_deferred) instead. /// /// Note: The given set is not implicitly added to the schedule when this system set is added. /// It is safe, but no dependencies will be created. @@ -260,7 +307,11 @@ where self.into_configs().before(set) } - /// Run after all systems in `set`. + /// Run after all systems in `set`. If `set` has any systems that produce [`Commands`](crate::system::Commands) + /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `self` will see their effect. + /// + /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like + /// this isn't desired, use [`after_ignore_deferred`](Self::after_ignore_deferred) instead. /// /// Note: The given set is not implicitly added to the schedule when this system set is added. /// It is safe, but no dependencies will be created. @@ -268,6 +319,22 @@ where self.into_configs().after(set) } + /// Run before all systems in `set`. + /// + /// Unlike [`before`](Self::before), this will not cause the systems in + /// `set` to wait for the deferred effects of `self` to be applied. + fn before_ignore_deferred(self, set: impl IntoSystemSet) -> SystemConfigs { + self.into_configs().before_ignore_deferred(set) + } + + /// Run after all systems in `set`. + /// + /// Unlike [`after`](Self::after), this will not wait for the deferred + /// effects of systems in `set` to be applied. + fn after_ignore_deferred(self, set: impl IntoSystemSet) -> SystemConfigs { + self.into_configs().after_ignore_deferred(set) + } + /// Add a run condition to each contained system. /// /// Each system will receive its own clone of the [`Condition`] and will only run @@ -351,9 +418,22 @@ where /// Treat this collection as a sequence of systems. /// /// Ordering constraints will be applied between the successive elements. + /// + /// If the preceeding node on a edge has deferred parameters, a [`apply_deferred`](crate::schedule::apply_deferred) + /// will be inserted on the edge. If this behavior is not desired consider using + /// [`chain_ignore_deferred`](Self::chain_ignore_deferred) instead. fn chain(self) -> SystemConfigs { self.into_configs().chain() } + + /// Treat this collection as a sequence of systems. + /// + /// Ordering constraints will be applied between the successive elements. + /// + /// Unlike [`chain`](Self::chain) this will **not** add [`apply_deferred`](crate::schedule::apply_deferred) on the edges. + fn chain_ignore_deferred(self) -> SystemConfigs { + self.into_configs().chain_ignore_deferred() + } } impl IntoSystemConfigs<()> for SystemConfigs { @@ -385,6 +465,18 @@ impl IntoSystemConfigs<()> for SystemConfigs { self } + fn before_ignore_deferred(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + self.before_ignore_deferred_inner(set.intern()); + self + } + + fn after_ignore_deferred(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + self.after_ignore_deferred_inner(set.intern()); + self + } + fn distributive_run_if(mut self, condition: impl Condition + Clone) -> SystemConfigs { self.distributive_run_if_inner(condition); self @@ -409,6 +501,10 @@ impl IntoSystemConfigs<()> for SystemConfigs { fn chain(self) -> Self { self.chain_inner() } + + fn chain_ignore_deferred(self) -> Self { + self.chain_ignore_deferred_inner() + } } #[doc(hidden)] @@ -426,7 +522,7 @@ macro_rules! impl_system_collection { SystemConfigs::Configs { configs: vec![$($sys.into_configs(),)*], collective_conditions: Vec::new(), - chained: false, + chained: Chain::No, } } } @@ -474,16 +570,40 @@ where self.into_configs().in_set(set) } - /// Run before all systems in `set`. + /// Runs before all systems in `set`. If `self` has any systems that produce [`Commands`](crate::system::Commands) + /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `set` will see their effect. + /// + /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like + /// this isn't desired, use [`before_ignore_deferred`](Self::before_ignore_deferred) instead. fn before(self, set: impl IntoSystemSet) -> SystemSetConfigs { self.into_configs().before(set) } - /// Run after all systems in `set`. + /// Runs before all systems in `set`. If `set` has any systems that produce [`Commands`](crate::system::Commands) + /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `self` will see their effect. + /// + /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like + /// this isn't desired, use [`after_ignore_deferred`](Self::after_ignore_deferred) instead. fn after(self, set: impl IntoSystemSet) -> SystemSetConfigs { self.into_configs().after(set) } + /// Run before all systems in `set`. + /// + /// Unlike [`before`](Self::before), this will not cause the systems in `set` to wait for the + /// deferred effects of `self` to be applied. + fn before_ignore_deferred(self, set: impl IntoSystemSet) -> SystemSetConfigs { + self.into_configs().before_ignore_deferred(set) + } + + /// Run after all systems in `set`. + /// + /// Unlike [`after`](Self::after), this may not see the deferred + /// effects of systems in `set` to be applied. + fn after_ignore_deferred(self, set: impl IntoSystemSet) -> SystemSetConfigs { + self.into_configs().after_ignore_deferred(set) + } + /// Run the systems in this set(s) only if the [`Condition`] is `true`. /// /// The `Condition` will be evaluated at most once (per schedule run), @@ -510,6 +630,15 @@ where fn chain(self) -> SystemSetConfigs { self.into_configs().chain() } + + /// Treat this collection as a sequence of systems. + /// + /// Ordering constraints will be applied between the successive elements. + /// + /// Unlike [`chain`](Self::chain) this will **not** add [`apply_deferred`](crate::schedule::apply_deferred) on the edges. + fn chain_ignore_deferred(self) -> SystemConfigs { + self.into_configs().chain_ignore_deferred() + } } impl IntoSystemSetConfigs for SystemSetConfigs { @@ -542,6 +671,20 @@ impl IntoSystemSetConfigs for SystemSetConfigs { self } + fn before_ignore_deferred(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + self.before_ignore_deferred_inner(set.intern()); + + self + } + + fn after_ignore_deferred(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + self.after_ignore_deferred_inner(set.intern()); + + self + } + fn run_if(mut self, condition: impl Condition) -> SystemSetConfigs { self.run_if_dyn(new_condition(condition)); @@ -588,7 +731,7 @@ macro_rules! impl_system_set_collection { SystemSetConfigs::Configs { configs: vec![$($set.into_configs(),)*], collective_conditions: Vec::new(), - chained: false, + chained: Chain::No, } } } diff --git a/crates/bevy_ecs/src/schedule/graph_utils.rs b/crates/bevy_ecs/src/schedule/graph_utils.rs index ee35212464b7d..04fb9a9ddda92 100644 --- a/crates/bevy_ecs/src/schedule/graph_utils.rs +++ b/crates/bevy_ecs/src/schedule/graph_utils.rs @@ -45,6 +45,10 @@ pub(crate) enum DependencyKind { Before, /// A node that should be succeeded. After, + /// A node that should be preceded and will **not** automatically insert an instance of `apply_deferred` on the edge. + BeforeNoSync, + /// A node that should be succeeded and will **not** automatically insert an instance of `apply_deferred` on the edge. + AfterNoSync, } /// An edge to be added to the dependency graph. diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 5b042590d5cf2..3c7885c03f10d 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -21,7 +21,7 @@ use crate::{ component::{ComponentId, Components, Tick}, prelude::Component, schedule::*, - system::{BoxedSystem, Resource, System}, + system::{BoxedSystem, IntoSystem, Resource, System}, world::World, }; @@ -156,6 +156,18 @@ fn make_executor(kind: ExecutorKind) -> Box { } } +/// Chain systems into dependencies +#[derive(PartialEq)] +pub enum Chain { + /// Run nodes in order. If there are deferred parameters in preceeding systems a + /// [`apply_deferred`] will be added on the edge. + Yes, + /// Run nodes in order. This will not add [`apply_deferred`] between nodes. + YesIgnoreDeferred, + /// Nodes are allowed to run in any order. + No, +} + /// A collection of systems, and the metadata and executor needed to run them /// in a certain order under certain conditions. /// @@ -452,6 +464,8 @@ pub struct ScheduleGraph { anonymous_sets: usize, changed: bool, settings: ScheduleBuildSettings, + no_sync_edges: BTreeSet<(NodeId, NodeId)>, + auto_sync_node_ids: HashMap, } impl ScheduleGraph { @@ -472,6 +486,8 @@ impl ScheduleGraph { anonymous_sets: 0, changed: false, settings: default(), + no_sync_edges: BTreeSet::new(), + auto_sync_node_ids: HashMap::new(), } } @@ -614,7 +630,7 @@ impl ScheduleGraph { let mut config_iter = configs.into_iter(); let mut nodes_in_scope = Vec::new(); let mut densely_chained = true; - if chained { + if chained == Chain::Yes || chained == Chain::YesIgnoreDeferred { let Some(prev) = config_iter.next() else { return ProcessConfigsResult { nodes: Vec::new(), @@ -640,6 +656,11 @@ impl ScheduleGraph { *first_in_current, (), ); + + if chained == Chain::YesIgnoreDeferred { + self.no_sync_edges + .insert((*last_in_prev, *first_in_current)); + } } // The previous group is "densely" chained, so we can simplify the graph by only // chaining the last item from the previous list to every item in the current list @@ -651,6 +672,10 @@ impl ScheduleGraph { *current_node, (), ); + + if chained == Chain::YesIgnoreDeferred { + self.no_sync_edges.insert((*last_in_prev, *current_node)); + } } } // The current list is currently "densely" chained, so we can simplify the graph by @@ -663,6 +688,11 @@ impl ScheduleGraph { *first_in_current, (), ); + + if chained == Chain::YesIgnoreDeferred { + self.no_sync_edges + .insert((*previous_node, *first_in_current)); + } } } // Neither of the lists are "densely" chained, so we must chain every item in the first @@ -675,6 +705,11 @@ impl ScheduleGraph { *current_node, (), ); + + if chained == Chain::YesIgnoreDeferred { + self.no_sync_edges + .insert((*previous_node, *current_node)); + } } } } @@ -858,7 +893,15 @@ impl ScheduleGraph { { let (lhs, rhs) = match kind { DependencyKind::Before => (id, set), + DependencyKind::BeforeNoSync => { + self.no_sync_edges.insert((id, set)); + (id, set) + } DependencyKind::After => (set, id), + DependencyKind::AfterNoSync => { + self.no_sync_edges.insert((set, id)); + (set, id) + } }; self.dependency.graph.add_edge(lhs, rhs, ()); @@ -941,7 +984,12 @@ impl ScheduleGraph { // check that there are no edges to system-type sets that have multiple instances self.check_system_type_set_ambiguity(&set_systems)?; - let dependency_flattened = self.get_dependency_flattened(&set_systems); + let mut dependency_flattened = self.get_dependency_flattened(&set_systems); + + // modify graph with auto sync points + if self.settings.auto_insert_apply_deferred { + dependency_flattened = self.auto_insert_apply_deferred(&mut dependency_flattened)?; + } // topsort let mut dependency_flattened_dag = Dag { @@ -973,6 +1021,84 @@ impl ScheduleGraph { Ok(self.build_schedule_inner(dependency_flattened_dag, hier_results.reachable)) } + // modify the graph to have sync nodes for any dependants after a system with deferred system params + fn auto_insert_apply_deferred( + &mut self, + dependency_flattened: &mut GraphMap, + ) -> Result, ScheduleBuildError> { + let mut sync_point_graph = dependency_flattened.clone(); + let topo = self.topsort_graph(dependency_flattened, ReportCycles::Dependency)?; + + // calculate the number of sync points each sync point is from the beginning of the graph + // use the same sync point if the distance is the same + let mut distances: HashMap> = HashMap::with_capacity(topo.len()); + for node in &topo { + let add_sync_after = self.systems[node.index()].get().unwrap().has_deferred(); + + for target in dependency_flattened.neighbors_directed(*node, Outgoing) { + let add_sync_on_edge = add_sync_after + && !is_apply_deferred(self.systems[target.index()].get().unwrap()) + && !self.no_sync_edges.contains(&(*node, target)); + + let weight = if add_sync_on_edge { 1 } else { 0 }; + + let distance = distances + .get(&target.index()) + .unwrap_or(&None) + .or(Some(0)) + .map(|distance| { + distance.max( + distances.get(&node.index()).unwrap_or(&None).unwrap_or(0) + weight, + ) + }); + + distances.insert(target.index(), distance); + + if add_sync_on_edge { + let sync_point = self.get_sync_point(distances[&target.index()].unwrap()); + sync_point_graph.add_edge(*node, sync_point, ()); + sync_point_graph.add_edge(sync_point, target, ()); + + // edge is now redundant + sync_point_graph.remove_edge(*node, target); + } + } + } + + Ok(sync_point_graph) + } + + /// add an [`apply_deferred`] system with no config + fn add_auto_sync(&mut self) -> NodeId { + let id = NodeId::System(self.systems.len()); + + self.systems + .push(SystemNode::new(Box::new(IntoSystem::into_system( + apply_deferred, + )))); + self.system_conditions.push(Vec::new()); + + // ignore ambiguities with auto sync points + // They aren't under user control, so no one should know or care. + self.ambiguous_with_all.insert(id); + + id + } + + /// Returns the `NodeId` of the cached auto sync point. Will create + /// a new one if needed. + fn get_sync_point(&mut self, distance: u32) -> NodeId { + self.auto_sync_node_ids + .get(&distance) + .copied() + .or_else(|| { + let node_id = self.add_auto_sync(); + self.auto_sync_node_ids.insert(distance, node_id); + Some(node_id) + }) + .unwrap() + } + fn map_sets_to_systems( &self, hierarchy_topsort: &[NodeId], @@ -1011,7 +1137,7 @@ impl ScheduleGraph { } fn get_dependency_flattened( - &self, + &mut self, set_systems: &HashMap>, ) -> GraphMap { // flatten: combine `in_set` with `before` and `after` information @@ -1020,20 +1146,33 @@ impl ScheduleGraph { let mut temp = Vec::new(); for (&set, systems) in set_systems { if systems.is_empty() { + // collapse dependencies for empty sets for a in dependency_flattened.neighbors_directed(set, Incoming) { for b in dependency_flattened.neighbors_directed(set, Outgoing) { + if self.no_sync_edges.contains(&(a, set)) + && self.no_sync_edges.contains(&(set, b)) + { + self.no_sync_edges.insert((a, b)); + } + temp.push((a, b)); } } } else { for a in dependency_flattened.neighbors_directed(set, Incoming) { for &sys in systems { + if self.no_sync_edges.contains(&(a, set)) { + self.no_sync_edges.insert((a, sys)); + } temp.push((a, sys)); } } for b in dependency_flattened.neighbors_directed(set, Outgoing) { for &sys in systems { + if self.no_sync_edges.contains(&(set, b)) { + self.no_sync_edges.insert((sys, b)); + } temp.push((sys, b)); } } @@ -1599,9 +1738,9 @@ impl ScheduleGraph { let n_ambiguities = ambiguities.len(); let mut message = format!( - "{n_ambiguities} pairs of systems with conflicting data access have indeterminate execution order. \ - Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n", - ); + "{n_ambiguities} pairs of systems with conflicting data access have indeterminate execution order. \ + Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n", + ); for (name_a, name_b, conflicts) in self.conflicts_to_string(ambiguities, components) { writeln!(message, " -- {name_a} and {name_b}").unwrap(); @@ -1729,6 +1868,16 @@ pub struct ScheduleBuildSettings { /// /// Defaults to [`LogLevel::Warn`]. pub hierarchy_detection: LogLevel, + /// Auto insert [`apply_deferred`] systems into the schedule, + /// when there are [`Deferred`](crate::prelude::Deferred) + /// in one system and there are ordering dependencies on that system. [`Commands`](crate::system::Commands) is one + /// such deferred buffer. + /// + /// You may want to disable this if you only want to sync deferred params at the end of the schedule, + /// or want to manually insert all your sync points. + /// + /// Defaults to `true` + pub auto_insert_apply_deferred: bool, /// If set to true, node names will be shortened instead of the fully qualified type path. /// /// Defaults to `true`. @@ -1752,6 +1901,7 @@ impl ScheduleBuildSettings { Self { ambiguity_detection: LogLevel::Ignore, hierarchy_detection: LogLevel::Warn, + auto_insert_apply_deferred: true, use_shortnames: true, report_sets: true, } @@ -1762,10 +1912,20 @@ impl ScheduleBuildSettings { mod tests { use crate::{ self as bevy_ecs, - schedule::{IntoSystemConfigs, IntoSystemSetConfigs, Schedule, SystemSet}, + prelude::{Res, Resource}, + schedule::{ + IntoSystemConfigs, IntoSystemSetConfigs, Schedule, ScheduleBuildSettings, SystemSet, + }, + system::Commands, world::World, }; + #[derive(Resource)] + struct Resource1; + + #[derive(Resource)] + struct Resource2; + // regression test for https://github.com/bevyengine/bevy/issues/9114 #[test] fn ambiguous_with_not_breaking_run_conditions() { @@ -1783,4 +1943,438 @@ mod tests { ); schedule.run(&mut world); } + + #[test] + fn inserts_a_sync_point() { + let mut schedule = Schedule::default(); + let mut world = World::default(); + schedule.add_systems( + ( + |mut commands: Commands| commands.insert_resource(Resource1), + |_: Res| {}, + ) + .chain(), + ); + schedule.run(&mut world); + + // inserted a sync point + assert_eq!(schedule.executable.systems.len(), 3); + } + + #[test] + fn merges_sync_points_into_one() { + let mut schedule = Schedule::default(); + let mut world = World::default(); + // insert two parallel command systems, it should only create one sync point + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Resource1), + |mut commands: Commands| commands.insert_resource(Resource2), + ), + |_: Res, _: Res| {}, + ) + .chain(), + ); + schedule.run(&mut world); + + // inserted sync points + assert_eq!(schedule.executable.systems.len(), 4); + + // merges sync points on rebuild + schedule.add_systems((( + ( + |mut commands: Commands| commands.insert_resource(Resource1), + |mut commands: Commands| commands.insert_resource(Resource2), + ), + |_: Res, _: Res| {}, + ) + .chain(),)); + schedule.run(&mut world); + + assert_eq!(schedule.executable.systems.len(), 7); + } + + #[test] + fn adds_multiple_consecutive_syncs() { + let mut schedule = Schedule::default(); + let mut world = World::default(); + // insert two consecutive command systems, it should create two sync points + schedule.add_systems( + ( + |mut commands: Commands| commands.insert_resource(Resource1), + |mut commands: Commands| commands.insert_resource(Resource2), + |_: Res, _: Res| {}, + ) + .chain(), + ); + schedule.run(&mut world); + + assert_eq!(schedule.executable.systems.len(), 5); + } + + #[test] + fn disable_auto_sync_points() { + let mut schedule = Schedule::default(); + schedule.set_build_settings(ScheduleBuildSettings { + auto_insert_apply_deferred: false, + ..Default::default() + }); + let mut world = World::default(); + schedule.add_systems( + ( + |mut commands: Commands| commands.insert_resource(Resource1), + |res: Option>| assert!(res.is_none()), + ) + .chain(), + ); + schedule.run(&mut world); + + assert_eq!(schedule.executable.systems.len(), 2); + } + + mod no_sync_edges { + use super::*; + + fn insert_resource(mut commands: Commands) { + commands.insert_resource(Resource1); + } + + fn resource_does_not_exist(res: Option>) { + assert!(res.is_none()); + } + + #[derive(SystemSet, Hash, PartialEq, Eq, Debug, Clone)] + enum Sets { + A, + B, + } + + fn check_no_sync_edges(add_systems: impl FnOnce(&mut Schedule)) { + let mut schedule = Schedule::default(); + let mut world = World::default(); + add_systems(&mut schedule); + + schedule.run(&mut world); + + assert_eq!(schedule.executable.systems.len(), 2); + } + + #[test] + fn system_to_system_after() { + check_no_sync_edges(|schedule| { + schedule.add_systems(( + insert_resource, + resource_does_not_exist.after_ignore_deferred(insert_resource), + )); + }); + } + + #[test] + fn system_to_system_before() { + check_no_sync_edges(|schedule| { + schedule.add_systems(( + insert_resource.before_ignore_deferred(resource_does_not_exist), + resource_does_not_exist, + )); + }); + } + + #[test] + fn set_to_system_after() { + check_no_sync_edges(|schedule| { + schedule + .add_systems((insert_resource, resource_does_not_exist.in_set(Sets::A))) + .configure_sets(Sets::A.after_ignore_deferred(insert_resource)); + }); + } + + #[test] + fn set_to_system_before() { + check_no_sync_edges(|schedule| { + schedule + .add_systems((insert_resource.in_set(Sets::A), resource_does_not_exist)) + .configure_sets(Sets::A.before_ignore_deferred(resource_does_not_exist)); + }); + } + + #[test] + fn set_to_set_after() { + check_no_sync_edges(|schedule| { + schedule + .add_systems(( + insert_resource.in_set(Sets::A), + resource_does_not_exist.in_set(Sets::B), + )) + .configure_sets(Sets::B.after_ignore_deferred(Sets::A)); + }); + } + + #[test] + fn set_to_set_before() { + check_no_sync_edges(|schedule| { + schedule + .add_systems(( + insert_resource.in_set(Sets::A), + resource_does_not_exist.in_set(Sets::B), + )) + .configure_sets(Sets::A.before_ignore_deferred(Sets::B)); + }); + } + } + + mod no_sync_chain { + use super::*; + + #[derive(Resource)] + struct Ra; + + #[derive(Resource)] + struct Rb; + + #[derive(Resource)] + struct Rc; + + fn run_schedule(expected_num_systems: usize, add_systems: impl FnOnce(&mut Schedule)) { + let mut schedule = Schedule::default(); + let mut world = World::default(); + add_systems(&mut schedule); + + schedule.run(&mut world); + + assert_eq!(schedule.executable.systems.len(), expected_num_systems); + } + + #[test] + fn only_chain_outside() { + run_schedule(5, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands| commands.insert_resource(Rb), + ), + ( + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + ), + ) + .chain(), + ); + }); + + run_schedule(4, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands| commands.insert_resource(Rb), + ), + ( + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_none()); + assert!(res_b.is_none()); + }, + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_none()); + assert!(res_b.is_none()); + }, + ), + ) + .chain_ignore_deferred(), + ); + }); + } + + #[test] + fn chain_first() { + run_schedule(6, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands, res_a: Option>| { + commands.insert_resource(Rb); + assert!(res_a.is_some()); + }, + ) + .chain(), + ( + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + ), + ) + .chain(), + ); + }); + + run_schedule(5, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands, res_a: Option>| { + commands.insert_resource(Rb); + assert!(res_a.is_some()); + }, + ) + .chain(), + ( + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_none()); + }, + |res_a: Option>, res_b: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_none()); + }, + ), + ) + .chain_ignore_deferred(), + ); + }); + } + + #[test] + fn chain_second() { + run_schedule(6, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands| commands.insert_resource(Rb), + ), + ( + |mut commands: Commands, + res_a: Option>, + res_b: Option>| { + commands.insert_resource(Rc); + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + |res_a: Option>, + res_b: Option>, + res_c: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + assert!(res_c.is_some()); + }, + ) + .chain(), + ) + .chain(), + ); + }); + + run_schedule(5, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands| commands.insert_resource(Rb), + ), + ( + |mut commands: Commands, + res_a: Option>, + res_b: Option>| { + commands.insert_resource(Rc); + assert!(res_a.is_none()); + assert!(res_b.is_none()); + }, + |res_a: Option>, + res_b: Option>, + res_c: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + assert!(res_c.is_some()); + }, + ) + .chain(), + ) + .chain_ignore_deferred(), + ); + }); + } + + #[test] + fn chain_all() { + run_schedule(7, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands, res_a: Option>| { + commands.insert_resource(Rb); + assert!(res_a.is_some()); + }, + ) + .chain(), + ( + |mut commands: Commands, + res_a: Option>, + res_b: Option>| { + commands.insert_resource(Rc); + assert!(res_a.is_some()); + assert!(res_b.is_some()); + }, + |res_a: Option>, + res_b: Option>, + res_c: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + assert!(res_c.is_some()); + }, + ) + .chain(), + ) + .chain(), + ); + }); + + run_schedule(6, |schedule: &mut Schedule| { + schedule.add_systems( + ( + ( + |mut commands: Commands| commands.insert_resource(Ra), + |mut commands: Commands, res_a: Option>| { + commands.insert_resource(Rb); + assert!(res_a.is_some()); + }, + ) + .chain(), + ( + |mut commands: Commands, + res_a: Option>, + res_b: Option>| { + commands.insert_resource(Rc); + assert!(res_a.is_some()); + assert!(res_b.is_none()); + }, + |res_a: Option>, + res_b: Option>, + res_c: Option>| { + assert!(res_a.is_some()); + assert!(res_b.is_some()); + assert!(res_c.is_some()); + }, + ) + .chain(), + ) + .chain_ignore_deferred(), + ); + }); + } + } } diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 8e087e15bff33..0ccd2cf0991bd 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -104,6 +104,10 @@ where self.system.is_exclusive() } + fn has_deferred(&self) -> bool { + self.system.has_deferred() + } + #[inline] unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { // SAFETY: `system.run_unsafe` has the same invariants as `self.run_unsafe`. diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 5461e503a47b1..5a7a403c47169 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -162,6 +162,10 @@ where self.a.is_exclusive() || self.b.is_exclusive() } + fn has_deferred(&self) -> bool { + self.a.has_deferred() || self.b.has_deferred() + } + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { Func::combine( input, diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index e9090b6f30152..1d9e1241d2232 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -93,6 +93,12 @@ where true } + #[inline] + fn has_deferred(&self) -> bool { + // exclusive systems have no deferred system params + false + } + #[inline] unsafe fn run_unsafe(&mut self, _input: Self::In, _world: UnsafeWorldCell) -> Self::Out { panic!("Cannot run exclusive systems with a shared World reference"); diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index e327288685f5f..431809ac9e08f 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -25,6 +25,7 @@ pub struct SystemMeta { // NOTE: this must be kept private. making a SystemMeta non-send is irreversible to prevent // SystemParams from overriding each other is_send: bool, + has_deferred: bool, pub(crate) last_run: Tick, #[cfg(feature = "trace")] pub(crate) system_span: Span, @@ -40,6 +41,7 @@ impl SystemMeta { archetype_component_access: Access::default(), component_access_set: FilteredAccessSet::default(), is_send: true, + has_deferred: false, last_run: Tick::new(0), #[cfg(feature = "trace")] system_span: info_span!("system", name = name), @@ -67,6 +69,18 @@ impl SystemMeta { pub fn set_non_send(&mut self) { self.is_send = false; } + + /// Returns true if the system has deferred [`SystemParam`]'s + #[inline] + pub fn has_deferred(&self) -> bool { + self.has_deferred + } + + /// Marks the system as having deferred buffers like [`Commands`](`super::Commands`) + /// This lets the scheduler insert [`apply_deferred`](`crate::prelude::apply_deferred`) systems automatically. + pub fn set_has_deferred(&mut self) { + self.has_deferred = true; + } } // TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference @@ -464,6 +478,11 @@ where false } + #[inline] + fn has_deferred(&self) -> bool { + self.system_meta.has_deferred + } + #[inline] unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { #[cfg(feature = "trace")] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index d31336b165d94..073c93085937c 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -42,6 +42,9 @@ pub trait System: Send + Sync + 'static { /// Returns true if the system must be run exclusively. fn is_exclusive(&self) -> bool; + /// Returns true if system as deferred buffers + fn has_deferred(&self) -> bool; + /// Runs the system with the given input in the world. Unlike [`System::run`], this function /// can be called in parallel with other systems and may break Rust's aliasing rules /// if used incorrectly, making it unsafe to call. diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 673b8229d6ede..25706c30fd2c6 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -899,7 +899,8 @@ unsafe impl SystemParam for Deferred<'_, T> { type State = SyncCell; type Item<'w, 's> = Deferred<'s, T>; - fn init_state(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { + fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { + system_meta.set_has_deferred(); SyncCell::new(T::from_world(world)) } diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index 6427b99dab477..ce104502c8b10 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -263,7 +263,6 @@ impl Plugin for PbrPlugin { PostUpdate, ( SimulationLightSystems::AddClusters, - SimulationLightSystems::AddClustersFlush, SimulationLightSystems::AssignLightsToClusters, ) .chain(), @@ -272,7 +271,6 @@ impl Plugin for PbrPlugin { PostUpdate, ( add_clusters.in_set(SimulationLightSystems::AddClusters), - apply_deferred.in_set(SimulationLightSystems::AddClustersFlush), assign_lights_to_clusters .in_set(SimulationLightSystems::AssignLightsToClusters) .after(TransformSystem::TransformPropagate) @@ -306,7 +304,7 @@ impl Plugin for PbrPlugin { .after(SimulationLightSystems::AssignLightsToClusters), check_light_mesh_visibility .in_set(SimulationLightSystems::CheckLightVisibility) - .after(VisibilitySystems::CalculateBoundsFlush) + .after(VisibilitySystems::CalculateBounds) .after(TransformSystem::TransformPropagate) .after(SimulationLightSystems::UpdateLightFrusta) // NOTE: This MUST be scheduled AFTER the core renderer visibility check diff --git a/crates/bevy_pbr/src/light.rs b/crates/bevy_pbr/src/light.rs index 179f7cf646703..bcded3f475fc1 100644 --- a/crates/bevy_pbr/src/light.rs +++ b/crates/bevy_pbr/src/light.rs @@ -631,7 +631,6 @@ pub enum ShadowFilteringMethod { #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] pub enum SimulationLightSystems { AddClusters, - AddClustersFlush, AssignLightsToClusters, UpdateDirectionalLightCascades, UpdateLightFrusta, diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index f6782d058f011..a8243422d607d 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -71,23 +71,17 @@ pub struct RenderPlugin { /// The labels of the default App rendering sets. /// -/// The sets run in the order listed, with [`apply_deferred`] inserted between each set. -/// -/// The `*Flush` sets are assigned to the copy of [`apply_deferred`] /// that runs immediately after the matching system set. /// These can be useful for ordering, but you almost never want to add your systems to these sets. #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] pub enum RenderSet { - /// The copy of [`apply_deferred`] that runs at the beginning of this schedule. /// This is used for applying the commands from the [`ExtractSchedule`] ExtractCommands, /// Prepare assets that have been created/modified/removed this frame. PrepareAssets, /// Create any additional views such as those used for shadow mapping. ManageViews, - /// The copy of [`apply_deferred`] that runs immediately after [`ManageViews`](RenderSet::ManageViews). - ManageViewsFlush, - /// Queue drawable entities as phase items in [`RenderPhase`](render_phase::RenderPhase)s + /// Queue drawable entities as phase items in [`RenderPhase`](crate::render_phase::RenderPhase)s /// ready for sorting Queue, /// A sub-set within [`Queue`](RenderSet::Queue) where mesh entity queue systems are executed. Ensures `prepare_assets::` is completed. @@ -100,21 +94,15 @@ pub enum RenderSet { Prepare, /// A sub-set within [`Prepare`](RenderSet::Prepare) for initializing buffers, textures and uniforms for use in bind groups. PrepareResources, - /// The copy of [`apply_deferred`] that runs between [`PrepareResources`](RenderSet::PrepareResources) and ['PrepareBindGroups'](RenderSet::PrepareBindGroups). + /// Flush buffers after [`PrepareResources`](RenderSet::PrepareResources), but before ['PrepareBindGroups'](RenderSet::PrepareBindGroups). PrepareResourcesFlush, /// A sub-set within [`Prepare`](RenderSet::Prepare) for constructing bind groups, or other data that relies on render resources prepared in [`PrepareResources`](RenderSet::PrepareResources). PrepareBindGroups, - /// The copy of [`apply_deferred`] that runs immediately after [`Prepare`](RenderSet::Prepare). - PrepareFlush, /// Actual rendering happens here. /// In most cases, only the render backend should insert resources here. Render, - /// The copy of [`apply_deferred`] that runs immediately after [`Render`](RenderSet::Render). - RenderFlush, /// Cleanup render resources here. Cleanup, - /// The copy of [`apply_deferred`] that runs immediately after [`Cleanup`](RenderSet::Cleanup). - CleanupFlush, } /// The main render schedule. @@ -124,35 +112,21 @@ pub struct Render; impl Render { /// Sets up the base structure of the rendering [`Schedule`]. /// - /// The sets defined in this enum are configured to run in order, - /// and a copy of [`apply_deferred`] is inserted into each [`*Flush` set](RenderSet). + /// The sets defined in this enum are configured to run in order. pub fn base_schedule() -> Schedule { use RenderSet::*; let mut schedule = Schedule::new(Self); - // Create "stage-like" structure using buffer flushes + ordering - schedule.add_systems(( - apply_deferred.in_set(ManageViewsFlush), - apply_deferred.in_set(PrepareResourcesFlush), - apply_deferred.in_set(RenderFlush), - apply_deferred.in_set(PrepareFlush), - apply_deferred.in_set(CleanupFlush), - )); - schedule.configure_sets( ( ExtractCommands, ManageViews, - ManageViewsFlush, Queue, PhaseSort, Prepare, - PrepareFlush, Render, - RenderFlush, Cleanup, - CleanupFlush, ) .chain(), ); @@ -175,7 +149,7 @@ impl Render { /// running the next frame while rendering the current frame. /// /// This schedule is run on the main world, but its buffers are not applied -/// via [`Schedule::apply_deferred`](Schedule) until it is returned to the render world. +/// until it is returned to the render world. #[derive(ScheduleLabel, PartialEq, Eq, Debug, Clone, Hash)] pub struct ExtractSchedule; diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 8311268640bb1..bc0c889c2a8d3 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -191,8 +191,6 @@ pub enum VisibilitySystems { /// Label for the [`calculate_bounds`] and `calculate_bounds_2d` systems, /// calculating and inserting an [`Aabb`] to relevant entities. CalculateBounds, - /// Label for the [`apply_deferred`] call after [`VisibilitySystems::CalculateBounds`] - CalculateBoundsFlush, /// Label for the [`update_frusta`] system. UpdateOrthographicFrusta, /// Label for the [`update_frusta`] system. @@ -213,47 +211,42 @@ impl Plugin for VisibilityPlugin { fn build(&self, app: &mut bevy_app::App) { use VisibilitySystems::*; - app - // We add an AABB component in CalculateBounds, which must be ready on the same frame. - .add_systems(PostUpdate, apply_deferred.in_set(CalculateBoundsFlush)) - .configure_sets(PostUpdate, CalculateBoundsFlush.after(CalculateBounds)) - .add_systems( - PostUpdate, - ( - calculate_bounds.in_set(CalculateBounds), - update_frusta:: - .in_set(UpdateOrthographicFrusta) - .after(camera_system::) - .after(TransformSystem::TransformPropagate) - // We assume that no camera will have more than one projection component, - // so these systems will run independently of one another. - // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. - .ambiguous_with(update_frusta::) - .ambiguous_with(update_frusta::), - update_frusta:: - .in_set(UpdatePerspectiveFrusta) - .after(camera_system::) - .after(TransformSystem::TransformPropagate) - // We assume that no camera will have more than one projection component, - // so these systems will run independently of one another. - // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. - .ambiguous_with(update_frusta::), - update_frusta:: - .in_set(UpdateProjectionFrusta) - .after(camera_system::) - .after(TransformSystem::TransformPropagate), - (visibility_propagate_system, reset_view_visibility) - .in_set(VisibilityPropagate), - check_visibility - .in_set(CheckVisibility) - .after(CalculateBoundsFlush) - .after(UpdateOrthographicFrusta) - .after(UpdatePerspectiveFrusta) - .after(UpdateProjectionFrusta) - .after(VisibilityPropagate) - .after(TransformSystem::TransformPropagate), - ), - ); + app.add_systems( + PostUpdate, + ( + calculate_bounds.in_set(CalculateBounds), + update_frusta:: + .in_set(UpdateOrthographicFrusta) + .after(camera_system::) + .after(TransformSystem::TransformPropagate) + // We assume that no camera will have more than one projection component, + // so these systems will run independently of one another. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(update_frusta::) + .ambiguous_with(update_frusta::), + update_frusta:: + .in_set(UpdatePerspectiveFrusta) + .after(camera_system::) + .after(TransformSystem::TransformPropagate) + // We assume that no camera will have more than one projection component, + // so these systems will run independently of one another. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(update_frusta::), + update_frusta:: + .in_set(UpdateProjectionFrusta) + .after(camera_system::) + .after(TransformSystem::TransformPropagate), + (visibility_propagate_system, reset_view_visibility).in_set(VisibilityPropagate), + check_visibility + .in_set(CheckVisibility) + .after(CalculateBounds) + .after(UpdateOrthographicFrusta) + .after(UpdatePerspectiveFrusta) + .after(UpdateProjectionFrusta) + .after(VisibilityPropagate) + .after(TransformSystem::TransformPropagate), + ), + ); } }