From cc5b79c1b680b854d3dcbbdb8fc7c91196e3ffea Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Thu, 30 Mar 2023 21:44:31 +0200 Subject: [PATCH 1/5] `collective_run_if` for `SystemConfigs` via anonymous system sets --- crates/bevy_app/src/app.rs | 2 + crates/bevy_ecs/src/schedule/config.rs | 55 ++++++++++++++++++++ crates/bevy_ecs/src/schedule/schedule.rs | 65 +++++++++++++++++++++--- crates/bevy_ecs/src/schedule/set.rs | 28 ++++++++++ 4 files changed, 144 insertions(+), 6 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index d7a4da63cd1b0..487ad31165216 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -380,8 +380,10 @@ impl App { /// # fn system_a() {} /// # fn system_b() {} /// # fn system_c() {} + /// # fn should_run() -> bool { true } /// # /// app.add_systems(Update, (system_a, system_b, system_c)); + /// app.add_systems(Update, (system_a, system_b).collective_run_if(should_run)); /// ``` pub fn add_systems( &mut self, diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 50aa7bfe9010a..b2648f7497219 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -59,6 +59,7 @@ pub enum SystemConfigs { SystemConfig(SystemConfig), Configs { configs: Vec, + collective_conditions: Vec, /// If `true`, adds `before -> after` ordering constraints between the successive elements. chained: bool, }, @@ -123,6 +124,20 @@ impl SystemConfigs { } } + fn collective_run_if_inner(&mut self, condition: BoxedCondition) { + match self { + SystemConfigs::SystemConfig(config) => { + config.conditions.push(condition); + } + SystemConfigs::Configs { + collective_conditions, + .. + } => { + collective_conditions.push(condition); + } + } + } + fn distributive_run_if_inner(&mut self, condition: impl Condition + Clone) { match self { SystemConfigs::SystemConfig(config) => { @@ -199,6 +214,40 @@ where self.into_configs().after(set) } + /// Add a single run condition for all contained systems. + /// + /// The [`Condition`] will be evaluated at most once (per schedule run), + /// when one of the contained systems is first run. + /// + /// This is equivalent to adding each system to an common set and configuring + /// the run condition on that set, as shown below: + /// + /// # Examples + /// + /// ``` + /// # use bevy_ecs::prelude::*; + /// # let mut app = Schedule::new(); + /// # fn a() {} + /// # fn b() {} + /// # fn condition() -> bool { true } + /// # #[derive(SystemSet, Debug, Eq, PartialEq, Hash, Clone, Copy)] + /// # struct C; + /// app.add_systems((a, b).collective_run_if(condition)); + /// app.add_systems((a, b).in_set(C)).configure_set(C.run_if(condition)); + /// ``` + /// + /// # Note + /// + /// Because the condition will only be evaluated once, there is no guarantee that the condition + /// is upheld after the first system has run. You need to make sure that no other systems that + /// could invalidate the condition are scheduled inbetween the first and last run system. + /// + /// Use [`distributive_run_if`](IntoSystemConfigs::distributive_run_if) if you want the + /// condition to be evaluated for each individual system, right before one is run. + fn collective_run_if

(self, condition: impl Condition

) -> SystemConfigs { + self.into_configs().collective_run_if(condition) + } + /// Add a run condition to each contained system. /// /// Each system will receive its own clone of the [`Condition`] and will only run @@ -319,6 +368,11 @@ impl IntoSystemConfigs<()> for SystemConfigs { self } + fn collective_run_if(mut self, condition: impl Condition) -> Self { + self.collective_run_if_inner(new_condition(condition)); + self + } + fn distributive_run_if(mut self, condition: impl Condition + Clone) -> SystemConfigs { self.distributive_run_if_inner(condition); self @@ -364,6 +418,7 @@ macro_rules! impl_system_collection { let ($($sys,)*) = self; SystemConfigs::Configs { configs: vec![$($sys.into_configs(),)*], + collective_conditions: Vec::new(), chained: false, } } diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 303ac8dd29479..f373580c02ebb 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -349,6 +349,10 @@ impl SystemSetNode { pub fn is_system_type(&self) -> bool { self.inner.system_type().is_some() } + + pub fn is_anonymous(&self) -> bool { + self.inner.is_anonymous() + } } /// A [`BoxedSystem`] with metadata, stored in a [`ScheduleGraph`]. @@ -525,7 +529,11 @@ impl ScheduleGraph { } } } - SystemConfigs::Configs { configs, chained } => { + SystemConfigs::Configs { + configs, + collective_conditions, + chained, + } => { let mut config_iter = configs.into_iter(); let mut nodes_in_scope = Vec::new(); let mut densely_chained = true; @@ -536,9 +544,26 @@ impl ScheduleGraph { densely_chained: true } }; + let more_than_one_entry = config_iter.len() > 0; + let anonymous_set = if more_than_one_entry && !collective_conditions.is_empty() + { + Some(AnonymousSet::new()) + } else { + None + }; + let prev = if let Some(anonymous_set) = anonymous_set { + prev.in_set(anonymous_set) + } else { + prev + }; let mut previous_result = self.add_systems_inner(prev, true); densely_chained = previous_result.densely_chained; for current in config_iter { + let current = if let Some(anonymous_set) = anonymous_set { + current.in_set(anonymous_set) + } else { + current + }; let current_result = self.add_systems_inner(current, true); densely_chained = densely_chained && current_result.densely_chained; match ( @@ -608,7 +633,18 @@ impl ScheduleGraph { } } else { let more_than_one_entry = config_iter.len() > 1; + let anonymous_set = if more_than_one_entry && !collective_conditions.is_empty() + { + Some(AnonymousSet::new()) + } else { + None + }; for config in config_iter { + let config = if let Some(anonymous_set) = anonymous_set { + config.in_set(anonymous_set) + } else { + config + }; let result = self.add_systems_inner(config, ancestor_chained); densely_chained = densely_chained && result.densely_chained; if ancestor_chained { @@ -705,8 +741,7 @@ impl ScheduleGraph { match self.system_set_ids.get(set) { Some(set_id) => { if id == set_id { - let string = format!("{:?}", &set); - return Err(ScheduleBuildError::HierarchyLoop(string)); + return Err(ScheduleBuildError::HierarchyLoop(self.get_node_name(id))); } } None => { @@ -742,8 +777,7 @@ impl ScheduleGraph { match self.system_set_ids.get(set) { Some(set_id) => { if id == set_id { - let string = format!("{:?}", &set); - return Err(ScheduleBuildError::DependencyLoop(string)); + return Err(ScheduleBuildError::DependencyLoop(self.get_node_name(id))); } } None => { @@ -1257,7 +1291,14 @@ impl ScheduleGraph { name } } - NodeId::Set(_) => self.system_sets[id.index()].name(), + NodeId::Set(_) => { + let set = &self.system_sets[id.index()]; + if set.is_anonymous() { + self.anonymous_set_name(id) + } else { + set.name() + } + } }; if self.settings.use_shortnames { name = bevy_utils::get_short_name(&name); @@ -1265,6 +1306,18 @@ impl ScheduleGraph { name } + fn anonymous_set_name(&self, id: &NodeId) -> String { + format!( + "({})", + self.hierarchy + .graph + .edges_directed(*id, Direction::Outgoing) + .map(|(_, member_id, _)| self.get_node_name(&member_id)) + .reduce(|a, b| format!("{a}, {b}")) + .unwrap_or_default() + ) + } + fn get_node_kind(&self, id: &NodeId) -> &'static str { match id { NodeId::System(_) => "system", diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index 56deb5c398ae8..5e2797a969111 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -2,6 +2,7 @@ use std::any::TypeId; use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::marker::PhantomData; +use std::sync::atomic::{AtomicUsize, Ordering}; pub use bevy_ecs_macros::{ScheduleLabel, SystemSet}; use bevy_utils::define_boxed_label; @@ -23,6 +24,10 @@ pub trait SystemSet: DynHash + Debug + Send + Sync + 'static { None } + /// Returns `true` if this system set is an [`AnonymousSet`]. + fn is_anonymous(&self) -> bool { + false + } /// Creates a boxed clone of the label corresponding to this system set. fn dyn_clone(&self) -> Box; } @@ -102,6 +107,29 @@ impl SystemSet for SystemTypeSet { } } +/// A [`SystemSet`] implicitly created when using +/// [`Schedule::add_systems`](super::Schedule::add_systems). +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +pub struct AnonymousSet(usize); + +static NEXT_ANONYMOUS_SET_ID: AtomicUsize = AtomicUsize::new(0); + +impl AnonymousSet { + pub(crate) fn new() -> Self { + Self(NEXT_ANONYMOUS_SET_ID.fetch_add(1, Ordering::Relaxed)) + } +} + +impl SystemSet for AnonymousSet { + fn is_anonymous(&self) -> bool { + true + } + + fn dyn_clone(&self) -> Box { + Box::new(*self) + } +} + /// Types that can be converted into a [`SystemSet`]. pub trait IntoSystemSet: Sized { type Set: SystemSet; From 0c4310e79a5cccaab366f71d4b3ef8ef1edb0a0c Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Thu, 30 Mar 2023 22:42:54 +0200 Subject: [PATCH 2/5] Replace `collective_run_if` with `run_if` --- crates/bevy_app/src/app.rs | 2 +- crates/bevy_ecs/src/schedule/config.rs | 90 ++++++++++---------------- 2 files changed, 34 insertions(+), 58 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 487ad31165216..8fa2d36c94039 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -383,7 +383,7 @@ impl App { /// # fn should_run() -> bool { true } /// # /// app.add_systems(Update, (system_a, system_b, system_c)); - /// app.add_systems(Update, (system_a, system_b).collective_run_if(should_run)); + /// app.add_systems(Update, (system_a, system_b).run_if(should_run)); /// ``` pub fn add_systems( &mut self, diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index b2648f7497219..6445458155158 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -124,20 +124,6 @@ impl SystemConfigs { } } - fn collective_run_if_inner(&mut self, condition: BoxedCondition) { - match self { - SystemConfigs::SystemConfig(config) => { - config.conditions.push(condition); - } - SystemConfigs::Configs { - collective_conditions, - .. - } => { - collective_conditions.push(condition); - } - } - } - fn distributive_run_if_inner(&mut self, condition: impl Condition + Clone) { match self { SystemConfigs::SystemConfig(config) => { @@ -182,8 +168,11 @@ impl SystemConfigs { SystemConfigs::SystemConfig(config) => { config.conditions.push(condition); } - SystemConfigs::Configs { .. } => { - todo!("run_if is not implemented for groups of systems yet") + SystemConfigs::Configs { + collective_conditions, + .. + } => { + collective_conditions.push(condition); } } } @@ -214,40 +203,6 @@ where self.into_configs().after(set) } - /// Add a single run condition for all contained systems. - /// - /// The [`Condition`] will be evaluated at most once (per schedule run), - /// when one of the contained systems is first run. - /// - /// This is equivalent to adding each system to an common set and configuring - /// the run condition on that set, as shown below: - /// - /// # Examples - /// - /// ``` - /// # use bevy_ecs::prelude::*; - /// # let mut app = Schedule::new(); - /// # fn a() {} - /// # fn b() {} - /// # fn condition() -> bool { true } - /// # #[derive(SystemSet, Debug, Eq, PartialEq, Hash, Clone, Copy)] - /// # struct C; - /// app.add_systems((a, b).collective_run_if(condition)); - /// app.add_systems((a, b).in_set(C)).configure_set(C.run_if(condition)); - /// ``` - /// - /// # Note - /// - /// Because the condition will only be evaluated once, there is no guarantee that the condition - /// is upheld after the first system has run. You need to make sure that no other systems that - /// could invalidate the condition are scheduled inbetween the first and last run system. - /// - /// Use [`distributive_run_if`](IntoSystemConfigs::distributive_run_if) if you want the - /// condition to be evaluated for each individual system, right before one is run. - fn collective_run_if

(self, condition: impl Condition

) -> SystemConfigs { - self.into_configs().collective_run_if(condition) - } - /// Add a run condition to each contained system. /// /// Each system will receive its own clone of the [`Condition`] and will only run @@ -265,8 +220,8 @@ where /// # fn a() {} /// # fn b() {} /// # fn condition() -> bool { true } - /// app.add_systems((a, b).distributive_run_if(condition)); - /// app.add_systems((a.run_if(condition), b.run_if(condition))); + /// app.add_systems(Update, (a, b).distributive_run_if(condition)); + /// app.add_systems(Update, (a.run_if(condition), b.run_if(condition))); /// ``` /// /// # Note @@ -286,6 +241,32 @@ where /// /// The `Condition` will be evaluated at most once (per schedule run), /// the first time a system in this set prepares to run. + /// + /// If this set contains more than one system, calling `run_if` is equivalent to adding each + /// system to an common set and configuring the run condition on that set, as shown below: + /// + /// # Examples + /// + /// ``` + /// # use bevy_ecs::prelude::*; + /// # let mut app = Schedule::new(); + /// # fn a() {} + /// # fn b() {} + /// # fn condition() -> bool { true } + /// # #[derive(SystemSet, Debug, Eq, PartialEq, Hash, Clone, Copy)] + /// # struct C; + /// app.add_systems(Update, (a, b).run_if(condition)); + /// app.add_systems(Update, (a, b).in_set(C)).configure_set(C.run_if(condition)); + /// ``` + /// + /// # Note + /// + /// Because the condition will only be evaluated once, there is no guarantee that the condition + /// is upheld after the first system has run. You need to make sure that no other systems that + /// could invalidate the condition are scheduled inbetween the first and last run system. + /// + /// Use [`distributive_run_if`](IntoSystemConfigs::distributive_run_if) if you want the + /// condition to be evaluated for each individual system, right before one is run. fn run_if(self, condition: impl Condition) -> SystemConfigs { self.into_configs().run_if(condition) } @@ -368,11 +349,6 @@ impl IntoSystemConfigs<()> for SystemConfigs { self } - fn collective_run_if(mut self, condition: impl Condition) -> Self { - self.collective_run_if_inner(new_condition(condition)); - self - } - fn distributive_run_if(mut self, condition: impl Condition + Clone) -> SystemConfigs { self.distributive_run_if_inner(condition); self From 39034fb58ce4aae16a578a1edbbefab7e31def96 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Thu, 30 Mar 2023 22:52:49 +0200 Subject: [PATCH 3/5] FIx doc-tests --- crates/bevy_ecs/src/schedule/config.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 6445458155158..44c7c4576df40 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -216,12 +216,12 @@ where /// /// ``` /// # use bevy_ecs::prelude::*; - /// # let mut app = Schedule::new(); + /// # let mut schedule = Schedule::new(); /// # fn a() {} /// # fn b() {} /// # fn condition() -> bool { true } - /// app.add_systems(Update, (a, b).distributive_run_if(condition)); - /// app.add_systems(Update, (a.run_if(condition), b.run_if(condition))); + /// schedule.add_systems((a, b).distributive_run_if(condition)); + /// schedule.add_systems((a.run_if(condition), b.run_if(condition))); /// ``` /// /// # Note @@ -249,14 +249,14 @@ where /// /// ``` /// # use bevy_ecs::prelude::*; - /// # let mut app = Schedule::new(); + /// # let mut schedule = Schedule::new(); /// # fn a() {} /// # fn b() {} /// # fn condition() -> bool { true } /// # #[derive(SystemSet, Debug, Eq, PartialEq, Hash, Clone, Copy)] /// # struct C; - /// app.add_systems(Update, (a, b).run_if(condition)); - /// app.add_systems(Update, (a, b).in_set(C)).configure_set(C.run_if(condition)); + /// schedule.add_systems((a, b).run_if(condition)); + /// schedule.add_systems((a, b).in_set(C)).configure_set(C.run_if(condition)); /// ``` /// /// # Note From b1aa4a8f2683ff4808b957aa20f9e0141d3e43d7 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Thu, 30 Mar 2023 14:20:39 -0700 Subject: [PATCH 4/5] Fix run_if, handle single Configs case, consolidate code --- crates/bevy_ecs/src/schedule/config.rs | 6 +-- crates/bevy_ecs/src/schedule/schedule.rs | 47 +++++++++--------------- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 44c7c4576df40..74cf880c58bc7 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -79,7 +79,7 @@ impl SystemConfigs { }) } - fn in_set_inner(&mut self, set: BoxedSystemSet) { + pub(crate) fn in_set_inner(&mut self, set: BoxedSystemSet) { match self { SystemConfigs::SystemConfig(config) => { config.graph_info.sets.push(set); @@ -163,7 +163,7 @@ impl SystemConfigs { } } - fn run_if_inner(&mut self, condition: BoxedCondition) { + pub(crate) fn run_if_inner(&mut self, condition: BoxedCondition) { match self { SystemConfigs::SystemConfig(config) => { config.conditions.push(condition); @@ -243,7 +243,7 @@ where /// the first time a system in this set prepares to run. /// /// If this set contains more than one system, calling `run_if` is equivalent to adding each - /// system to an common set and configuring the run condition on that set, as shown below: + /// system to a common set and configuring the run condition on that set, as shown below: /// /// # Examples /// diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index f373580c02ebb..8a148cf375b51 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -530,10 +530,26 @@ impl ScheduleGraph { } } SystemConfigs::Configs { - configs, + mut configs, collective_conditions, chained, } => { + let more_than_one_entry = configs.len() > 1; + if !collective_conditions.is_empty() { + if more_than_one_entry { + let set = AnonymousSet::new(); + for config in configs.iter_mut() { + config.in_set_inner(set.dyn_clone()); + } + let mut set_config = set.into_config(); + set_config.conditions.extend(collective_conditions); + self.configure_set(set_config); + } else { + for condition in collective_conditions { + configs[0].run_if_inner(condition); + } + } + } let mut config_iter = configs.into_iter(); let mut nodes_in_scope = Vec::new(); let mut densely_chained = true; @@ -544,26 +560,9 @@ impl ScheduleGraph { densely_chained: true } }; - let more_than_one_entry = config_iter.len() > 0; - let anonymous_set = if more_than_one_entry && !collective_conditions.is_empty() - { - Some(AnonymousSet::new()) - } else { - None - }; - let prev = if let Some(anonymous_set) = anonymous_set { - prev.in_set(anonymous_set) - } else { - prev - }; let mut previous_result = self.add_systems_inner(prev, true); densely_chained = previous_result.densely_chained; for current in config_iter { - let current = if let Some(anonymous_set) = anonymous_set { - current.in_set(anonymous_set) - } else { - current - }; let current_result = self.add_systems_inner(current, true); densely_chained = densely_chained && current_result.densely_chained; match ( @@ -632,19 +631,7 @@ impl ScheduleGraph { nodes_in_scope.append(&mut previous_result.nodes); } } else { - let more_than_one_entry = config_iter.len() > 1; - let anonymous_set = if more_than_one_entry && !collective_conditions.is_empty() - { - Some(AnonymousSet::new()) - } else { - None - }; for config in config_iter { - let config = if let Some(anonymous_set) = anonymous_set { - config.in_set(anonymous_set) - } else { - config - }; let result = self.add_systems_inner(config, ancestor_chained); densely_chained = densely_chained && result.densely_chained; if ancestor_chained { From 90c9f3ef5654196e3ac70122ec17f7ef00ded487 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Thu, 30 Mar 2023 14:23:49 -0700 Subject: [PATCH 5/5] Clippy --- crates/bevy_ecs/src/schedule/schedule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 8a148cf375b51..3269f9015ff2a 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -538,7 +538,7 @@ impl ScheduleGraph { if !collective_conditions.is_empty() { if more_than_one_entry { let set = AnonymousSet::new(); - for config in configs.iter_mut() { + for config in &mut configs { config.in_set_inner(set.dyn_clone()); } let mut set_config = set.into_config();