System sets and run criteria v2 (#1675)

I'm opening this prematurely; consider this an RFC that predates RFCs and therefore not super-RFC-like.

This PR does two "big" things: decouple run criteria from system sets, reimagine system sets as weapons of mass system description.

### What it lets us do:

* Reuse run criteria within a stage.
* Pipe output of one run criteria as input to another.
* Assign labels, dependencies, run criteria, and ambiguity sets to many systems at the same time.

### Things already done:
* Decoupled run criteria from system sets.
* Mass system description superpowers to `SystemSet`.
* Implemented `RunCriteriaDescriptor`.
* Removed `VirtualSystemSet`.
* Centralized all run criteria of `SystemStage`.
* Extended system descriptors with per-system run criteria.
* `.before()` and `.after()` for run criteria.
* Explicit order between state driver and related run criteria. Fixes #1672.
* Opt-in run criteria deduplication; default behavior is to panic.
* Labels (not exposed) for state run criteria; state run criteria are deduplicated.

### API issues that need discussion:

* [`FixedTimestep::step(1.0).label("my label")`](https://github.com/Ratysz/bevy/blob/eaccf857cdaeb5a5632b6e75feab5c1ad6267d1d/crates/bevy_ecs/src/schedule/run_criteria.rs#L120-L122) and [`FixedTimestep::step(1.0).with_label("my label")`](https://github.com/Ratysz/bevy/blob/eaccf857cdaeb5a5632b6e75feab5c1ad6267d1d/crates/bevy_core/src/time/fixed_timestep.rs#L86-L89) are both valid but do very different things.

---

I will try to maintain this post up-to-date as things change. Do check the diffs in "edited" thingy from time to time.

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

crates/bevy_app/src/app_builder.rs

@@ -13,6 +13,7 @@ use bevy_ecs::{
     world::{FromWorld, World},
 };
 use bevy_utils::tracing::debug;
+use std::{fmt::Debug, hash::Hash};
 
 /// Configure [App]s using the builder pattern
 pub struct AppBuilder {
@@ -177,16 +178,18 @@ impl AppBuilder {
         self
     }
 
-    pub fn add_state<T: Component + Clone + Eq>(&mut self, initial: T) -> &mut Self {
+    pub fn add_state<T>(&mut self, initial: T) -> &mut Self
+    where
+        T: Component + Debug + Clone + Eq + Hash,
+    {
         self.insert_resource(State::new(initial))
             .add_system_set(State::<T>::make_driver())
     }
 
-    pub fn add_state_to_stage<T: Component + Clone + Eq>(
-        &mut self,
-        stage: impl StageLabel,
-        initial: T,
-    ) -> &mut Self {
+    pub fn add_state_to_stage<T>(&mut self, stage: impl StageLabel, initial: T) -> &mut Self
+    where
+        T: Component + Debug + Clone + Eq + Hash,
+    {
         self.insert_resource(State::new(initial))
             .add_system_set_to_stage(stage, State::<T>::make_driver())
     }

crates/bevy_ecs/macros/src/lib.rs

@@ -433,6 +433,12 @@ pub fn derive_ambiguity_set_label(input: TokenStream) -> TokenStream {
     derive_label(input, Ident::new("AmbiguitySetLabel", Span::call_site())).into()
 }
 
+#[proc_macro_derive(RunCriteriaLabel)]
+pub fn derive_run_criteria_label(input: TokenStream) -> TokenStream {
+    let input = parse_macro_input!(input as DeriveInput);
+    derive_label(input, Ident::new("RunCriteriaLabel", Span::call_site())).into()
+}
+
 fn derive_label(input: DeriveInput, label_type: Ident) -> TokenStream2 {
     let ident = input.ident;
     let ecs_path: Path = bevy_ecs_path();

crates/bevy_ecs/src/lib.rs

@@ -19,6 +19,7 @@ pub mod prelude {
         query::{Added, ChangeTrackers, Changed, Or, QueryState, With, WithBundle, Without},
         schedule::{
             AmbiguitySetLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion,
+            RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel, RunCriteriaPiping,
             Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, SystemStage,
         },
         system::{

crates/bevy_ecs/src/schedule/graph_utils.rs

@@ -0,0 +1,125 @@
+use bevy_utils::{tracing::warn, HashMap, HashSet};
+use fixedbitset::FixedBitSet;
+use std::{borrow::Cow, fmt::Debug, hash::Hash};
+
+pub enum DependencyGraphError<Labels> {
+    GraphCycles(Vec<(usize, Labels)>),
+}
+
+pub trait GraphNode<Label> {
+    fn name(&self) -> Cow<'static, str>;
+    fn labels(&self) -> &[Label];
+    fn before(&self) -> &[Label];
+    fn after(&self) -> &[Label];
+}
+
+/// Constructs a dependency graph of given nodes.
+pub fn build_dependency_graph<Node, Label>(
+    nodes: &[Node],
+) -> HashMap<usize, HashMap<usize, HashSet<Label>>>
+where
+    Node: GraphNode<Label>,
+    Label: Debug + Clone + Eq + Hash,
+{
+    let mut labels = HashMap::<Label, FixedBitSet>::default();
+    for (label, index) in nodes.iter().enumerate().flat_map(|(index, container)| {
+        container
+            .labels()
+            .iter()
+            .cloned()
+            .map(move |label| (label, index))
+    }) {
+        labels
+            .entry(label)
+            .or_insert_with(|| FixedBitSet::with_capacity(nodes.len()))
+            .insert(index);
+    }
+    let mut graph = HashMap::with_capacity_and_hasher(nodes.len(), Default::default());
+    for (index, node) in nodes.iter().enumerate() {
+        let dependencies = graph.entry(index).or_insert_with(HashMap::default);
+        for label in node.after() {
+            match labels.get(label) {
+                Some(new_dependencies) => {
+                    for dependency in new_dependencies.ones() {
+                        dependencies
+                            .entry(dependency)
+                            .or_insert_with(HashSet::default)
+                            .insert(label.clone());
+                    }
+                }
+                None => warn!(
+                    // TODO: plumb this as proper output?
+                    "{} wants to be after unknown label: {:?}",
+                    nodes[index].name(),
+                    label
+                ),
+            }
+        }
+        for label in node.before() {
+            match labels.get(label) {
+                Some(dependants) => {
+                    for dependant in dependants.ones() {
+                        graph
+                            .entry(dependant)
+                            .or_insert_with(HashMap::default)
+                            .entry(index)
+                            .or_insert_with(HashSet::default)
+                            .insert(label.clone());
+                    }
+                }
+                None => warn!(
+                    "{} wants to be before unknown label: {:?}",
+                    nodes[index].name(),
+                    label
+                ),
+            }
+        }
+    }
+    graph
+}
+
+/// Generates a topological order for the given graph.
+pub fn topological_order<Labels: Clone>(
+    graph: &HashMap<usize, HashMap<usize, Labels>>,
+) -> Result<Vec<usize>, DependencyGraphError<Labels>> {
+    fn check_if_cycles_and_visit<L>(
+        node: &usize,
+        graph: &HashMap<usize, HashMap<usize, L>>,
+        sorted: &mut Vec<usize>,
+        unvisited: &mut HashSet<usize>,
+        current: &mut Vec<usize>,
+    ) -> bool {
+        if current.contains(node) {
+            return true;
+        } else if !unvisited.remove(node) {
+            return false;
+        }
+        current.push(*node);
+        for dependency in graph.get(node).unwrap().keys() {
+            if check_if_cycles_and_visit(dependency, &graph, sorted, unvisited, current) {
+                return true;
+            }
+        }
+        sorted.push(*node);
+        current.pop();
+        false
+    }
+    let mut sorted = Vec::with_capacity(graph.len());
+    let mut current = Vec::with_capacity(graph.len());
+    let mut unvisited = HashSet::with_capacity_and_hasher(graph.len(), Default::default());
+    unvisited.extend(graph.keys().cloned());
+    while let Some(node) = unvisited.iter().next().cloned() {
+        if check_if_cycles_and_visit(&node, graph, &mut sorted, &mut unvisited, &mut current) {
+            let mut cycle = Vec::new();
+            let last_window = [*current.last().unwrap(), current[0]];
+            let mut windows = current
+                .windows(2)
+                .chain(std::iter::once(&last_window as &[usize]));
+            while let Some(&[dependant, dependency]) = windows.next() {
+                cycle.push((dependant, graph[&dependant][&dependency].clone()));
+            }
+            return Err(DependencyGraphError::GraphCycles(cycle));
+        }
+    }
+    Ok(sorted)
+}

crates/bevy_ecs/src/schedule/label.rs

@@ -1,4 +1,4 @@
-pub use bevy_ecs_macros::{AmbiguitySetLabel, StageLabel, SystemLabel};
+pub use bevy_ecs_macros::{AmbiguitySetLabel, RunCriteriaLabel, StageLabel, SystemLabel};
 
 use std::{
     any::Any,
@@ -67,6 +67,12 @@ pub trait AmbiguitySetLabel: DynHash + Debug + Send + Sync + 'static {
 }
 pub(crate) type BoxedAmbiguitySetLabel = Box<dyn AmbiguitySetLabel>;
 
+pub trait RunCriteriaLabel: DynHash + Debug + Send + Sync + 'static {
+    #[doc(hidden)]
+    fn dyn_clone(&self) -> Box<dyn RunCriteriaLabel>;
+}
+pub(crate) type BoxedRunCriteriaLabel = Box<dyn RunCriteriaLabel>;
+
 macro_rules! impl_label {
     ($trait_name:ident) => {
         impl PartialEq for dyn $trait_name {
@@ -106,3 +112,4 @@ macro_rules! impl_label {
 impl_label!(StageLabel);
 impl_label!(SystemLabel);
 impl_label!(AmbiguitySetLabel);
+impl_label!(RunCriteriaLabel);

crates/bevy_ecs/src/schedule/mod.rs

@@ -1,6 +1,8 @@
 mod executor;
 mod executor_parallel;
+pub mod graph_utils;
 mod label;
+mod run_criteria;
 mod stage;
 mod state;
 mod system_container;
@@ -9,28 +11,26 @@ mod system_set;
 
 pub use executor::*;
 pub use executor_parallel::*;
+pub use graph_utils::GraphNode;
 pub use label::*;
+pub use run_criteria::*;
 pub use stage::*;
 pub use state::*;
 pub use system_container::*;
 pub use system_descriptor::*;
 pub use system_set::*;
 
 use crate::{
-    archetype::{Archetype, ArchetypeComponentId},
-    component::ComponentId,
-    query::Access,
-    system::{BoxedSystem, IntoSystem, System, SystemId},
+    system::{IntoSystem, System},
     world::World,
 };
 use bevy_utils::HashMap;
-use std::borrow::Cow;
 
 #[derive(Default)]
 pub struct Schedule {
     stages: HashMap<BoxedStageLabel, Box<dyn Stage>>,
     stage_order: Vec<BoxedStageLabel>,
-    run_criteria: RunCriteria,
+    run_criteria: BoxedRunCriteria,
 }
 
 impl Schedule {
@@ -229,110 +229,3 @@ impl Stage for Schedule {
         }
     }
 }
-
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum ShouldRun {
-    /// Yes, the system should run.
-    Yes,
-    /// No, the system should not run.
-    No,
-    /// Yes, the system should run, and afterwards the criteria should be checked again.
-    YesAndCheckAgain,
-    /// No, the system should not run right now, but the criteria should be checked again later.
-    NoAndCheckAgain,
-}
-
-pub(crate) struct RunCriteria {
-    criteria_system: Option<BoxedSystem<(), ShouldRun>>,
-    initialized: bool,
-}
-
-impl Default for RunCriteria {
-    fn default() -> Self {
-        Self {
-            criteria_system: None,
-            initialized: false,
-        }
-    }
-}
-
-impl RunCriteria {
-    pub fn set(&mut self, criteria_system: BoxedSystem<(), ShouldRun>) {
-        self.criteria_system = Some(criteria_system);
-        self.initialized = false;
-    }
-
-    pub fn should_run(&mut self, world: &mut World) -> ShouldRun {
-        if let Some(ref mut run_criteria) = self.criteria_system {
-            if !self.initialized {
-                run_criteria.initialize(world);
-                self.initialized = true;
-            }
-            let should_run = run_criteria.run((), world);
-            run_criteria.apply_buffers(world);
-            should_run
-        } else {
-            ShouldRun::Yes
-        }
-    }
-}
-
-pub struct RunOnce {
-    ran: bool,
-    system_id: SystemId,
-    archetype_component_access: Access<ArchetypeComponentId>,
-    component_access: Access<ComponentId>,
-}
-
-impl Default for RunOnce {
-    fn default() -> Self {
-        Self {
-            ran: false,
-            system_id: SystemId::new(),
-            archetype_component_access: Default::default(),
-            component_access: Default::default(),
-        }
-    }
-}
-
-impl System for RunOnce {
-    type In = ();
-    type Out = ShouldRun;
-
-    fn name(&self) -> Cow<'static, str> {
-        Cow::Borrowed(std::any::type_name::<RunOnce>())
-    }
-
-    fn id(&self) -> SystemId {
-        self.system_id
-    }
-
-    fn new_archetype(&mut self, _archetype: &Archetype) {}
-
-    fn archetype_component_access(&self) -> &Access<ArchetypeComponentId> {
-        &self.archetype_component_access
-    }
-
-    fn component_access(&self) -> &Access<ComponentId> {
-        &self.component_access
-    }
-
-    fn is_send(&self) -> bool {
-        true
-    }
-
-    unsafe fn run_unsafe(&mut self, _input: Self::In, _world: &World) -> Self::Out {
-        if self.ran {
-            ShouldRun::No
-        } else {
-            self.ran = true;
-            ShouldRun::Yes
-        }
-    }
-
-    fn apply_buffers(&mut self, _world: &mut World) {}
-
-    fn initialize(&mut self, _world: &mut World) {}
-
-    fn check_change_tick(&mut self, _change_tick: u32) {}
-}