run_if for SystemConfigs via anonymous system sets (#7676)

# Objective

- Fixes #7659

## Solution

The idea of anonymous system sets or "implicit hidden organizational
sets" was briefly mentioned by @cart here:
#7634 (comment).

- `Schedule::add_systems` creates an implicit, anonymous system set of
all systems in `SystemConfigs`.
- All dependencies and conditions from the `SystemConfigs` are now
applied to the implicit system set, instead of being applied to each
individual system. This should not change the behavior, AFAIU, because
`before`, `after`, `run_if` and `ambiguous_with` are transitive
properties from a set to its members.
- The newly added `AnonymousSystemSet` stores the names of its members
to provide better error messages.
- The names are stored in a reference counted slice, allowing fast
clones of the `AnonymousSystemSet`.
- However, only the pointer of the slice is used for hash and equality
operations
- This ensures that two `AnonymousSystemSet` are not equal, even if they
have the same members / member names.
- So two identical `add_systems` calls will produce two different
`AnonymousSystemSet`s.
  - Clones of the same `AnonymousSystemSet` will be equal.

## Drawbacks
If my assumptions are correct, the observed behavior should stay the
same. But the number of system sets in the `Schedule` will increase with
each `add_systems` call. If this has negative performance implications,
`add_systems` could be changed to only create the implicit system set if
necessary / when a run condition was added.

---------

Co-authored-by: Carter Anderson <[email protected]>

Author: geieredgar

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).run_if(should_run));
     /// ```
     pub fn add_systems<M>(
         &mut self,

crates/bevy_ecs/src/schedule/config.rs

@@ -59,6 +59,7 @@ pub enum SystemConfigs {
     SystemConfig(SystemConfig),
     Configs {
         configs: Vec<SystemConfigs>,
+        collective_conditions: Vec<BoxedCondition>,
         /// If `true`, adds `before -> after` ordering constraints between the successive elements.
         chained: bool,
     },
@@ -78,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);
@@ -162,13 +163,16 @@ 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);
             }
-            SystemConfigs::Configs { .. } => {
-                todo!("run_if is not implemented for groups of systems yet")
+            SystemConfigs::Configs {
+                collective_conditions,
+                ..
+            } => {
+                collective_conditions.push(condition);
             }
         }
     }
@@ -212,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((a, b).distributive_run_if(condition));
-    /// app.add_systems((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
@@ -237,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 a common set and configuring the run condition on that set, as shown below:
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use bevy_ecs::prelude::*;
+    /// # let mut schedule = Schedule::new();
+    /// # fn a() {}
+    /// # fn b() {}
+    /// # fn condition() -> bool { true }
+    /// # #[derive(SystemSet, Debug, Eq, PartialEq, Hash, Clone, Copy)]
+    /// # struct C;
+    /// schedule.add_systems((a, b).run_if(condition));
+    /// schedule.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 run_if<M>(self, condition: impl Condition<M>) -> SystemConfigs {
         self.into_configs().run_if(condition)
     }
@@ -364,6 +394,7 @@ macro_rules! impl_system_collection {
                 let ($($sys,)*) = self;
                 SystemConfigs::Configs {
                     configs: vec![$($sys.into_configs(),)*],
+                    collective_conditions: Vec::new(),
                     chained: false,
                 }
             }

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,27 @@ impl ScheduleGraph {
                     }
                 }
             }
-            SystemConfigs::Configs { configs, chained } => {
+            SystemConfigs::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 &mut configs {
+                            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;
@@ -607,7 +631,6 @@ impl ScheduleGraph {
                         nodes_in_scope.append(&mut previous_result.nodes);
                     }
                 } else {
-                    let more_than_one_entry = config_iter.len() > 1;
                     for config in config_iter {
                         let result = self.add_systems_inner(config, ancestor_chained);
                         densely_chained = densely_chained && result.densely_chained;
@@ -705,8 +728,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 +764,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,14 +1278,33 @@ 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);
         }
         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",

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<dyn SystemSet>;
 }
@@ -102,6 +107,29 @@ impl<T> SystemSet for SystemTypeSet<T> {
     }
 }
 
+/// 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<dyn SystemSet> {
+        Box::new(*self)
+    }
+}
+
 /// Types that can be converted into a [`SystemSet`].
 pub trait IntoSystemSet<Marker>: Sized {
     type Set: SystemSet;