RFC: Before/After markers

Explicit ordering of systems is hard: during refactoring you forgot one
edge, and your program befomes non-deterministic.

This draft PR proposes implementing ordering based on function signature
params (`SystemParam`).

Basically, the idea is this. We introduce two new system params:

```
Before<T>
After<T>
```

where `T` must be a `SystemSet + Default`.

And it can be used like this:

```
#[derive(SystemSet)]
struct MyBarrier;

fn first(_before: Before<MyBarrier>) {}
fn second(_after: After<MyBarrier>) {}
```

Now if `first` and `second` are added to the same schedule, they
will be ordered like if `first.before(after)` was done explicitly.

`SystemParam` are composable, so it is possible to build higher level
constructs on top of it. For example, we may want to schedule all event
writers before all event writers:

```
#[derive(SystemParam)]
struct MyOrderedEventWriter<T> {
    writer: EventWriter<T>,
    after: After<MyOrderedEventWriterBarrier<T>>,
}

#[derive(SystemParam)]
struct MyOrderedEventWriter<T> {
    reader: EventReader<T>,
    after: Before<MyOrderedEventWriterBarrier<T>>,
}
```

Want to get early feedback before spending several more hours on it.

WDYT?

Author: stepancheg

crates/bevy_ecs/macros/src/lib.rs

@@ -188,6 +188,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
             {
                 type State = (#(#param::State,)*);
                 type Item<'w, 's> = ParamSet<'w, 's, (#(#param,)*)>;
+                type BarrierList = (#(#param::BarrierList,)*);
 
                 fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
                     #(
@@ -392,6 +393,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
             {
                 type State = #state_struct_name<#punctuated_generic_idents>;
                 type Item<'w, 's> = #struct_name #ty_generics;
+                type BarrierList = ();
 
                 fn init_state(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self::State {
                     #state_struct_name {

crates/bevy_ecs/src/removal_detection.rs

@@ -263,6 +263,7 @@ unsafe impl<'a> ReadOnlySystemParam for &'a RemovedComponentEvents {}
 unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
     type State = ();
     type Item<'w, 's> = &'w RemovedComponentEvents;
+    type BarrierList = ();
 
     fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
 

crates/bevy_ecs/src/schedule/config.rs

@@ -79,10 +79,18 @@ impl SystemConfigs {
     fn new_system(system: BoxedSystem) -> Self {
         // include system in its default sets
         let sets = system.default_system_sets().into_iter().collect();
+        let mut dependencies = Vec::new();
+        for before in system.before_list() {
+            dependencies.push(Dependency::new(DependencyKind::Before, before));
+        }
+        for after in system.after_list() {
+            dependencies.push(Dependency::new(DependencyKind::After, after));
+        }
         Self::NodeConfig(SystemConfig {
             node: system,
             graph_info: GraphInfo {
                 sets,
+                dependencies,
                 ..Default::default()
             },
             conditions: Vec::new(),

crates/bevy_ecs/src/schedule/mod.rs

@@ -22,10 +22,17 @@ pub use self::graph_utils::NodeId;
 #[cfg(test)]
 mod tests {
     use super::*;
+    use bevy_ecs_macros::SystemParam;
+    use std::fmt;
+    use std::fmt::Debug;
+    use std::hash::{Hash, Hasher};
+    use std::marker::PhantomData;
     use std::sync::atomic::{AtomicU32, Ordering};
 
     pub use crate as bevy_ecs;
+    use crate::event::{Event, EventReader, EventWriter, Events};
     pub use crate::schedule::{IntoSystemSetConfigs, Schedule, SystemSet};
+    use crate::system::{After, Before};
     pub use crate::system::{Res, ResMut};
     pub use crate::{prelude::World, system::Resource};
 
@@ -1098,4 +1105,114 @@ mod tests {
             assert!(schedule.graph().conflicting_systems().is_empty());
         }
     }
+
+    #[test]
+    fn test_before_after_from_param() {
+        #[derive(SystemSet, Debug, Hash, Eq, PartialEq, Default, Clone, Copy)]
+        struct MyBarrier;
+
+        #[derive(Resource)]
+        struct MyRes {
+            x: u32,
+        }
+
+        fn first(_before: Before<MyBarrier>, mut res: ResMut<MyRes>) {
+            assert_eq!(0, res.x);
+            res.x = 1;
+        }
+
+        fn second(_after: After<MyBarrier>, mut res: ResMut<MyRes>) {
+            assert_eq!(1, res.x);
+            res.x = 2;
+        }
+
+        let mut schedule = Schedule::default();
+        schedule.add_systems((second, first));
+
+        let mut world = World::new();
+        world.insert_resource(MyRes { x: 0 });
+
+        schedule.run(&mut world);
+
+        assert_eq!(2, world.resource::<MyRes>().x);
+    }
+
+    // Test `Before`/`After` work when hidden inside `#[derive(SystemParam)]`.
+    #[test]
+    fn test_before_after_derive_system_param() {
+        #[derive(SystemSet)]
+        struct MyEventBarrier<T>(PhantomData<T>);
+
+        impl<T> Default for MyEventBarrier<T> {
+            fn default() -> Self {
+                Self(PhantomData)
+            }
+        }
+
+        impl<T> Clone for MyEventBarrier<T> {
+            fn clone(&self) -> Self {
+                Self(PhantomData)
+            }
+        }
+
+        impl<T> Copy for MyEventBarrier<T> {}
+
+        impl<T> PartialEq for MyEventBarrier<T> {
+            fn eq(&self, _other: &Self) -> bool {
+                true
+            }
+        }
+
+        impl<T> Eq for MyEventBarrier<T> {}
+
+        impl<T> Hash for MyEventBarrier<T> {
+            fn hash<H: Hasher>(&self, _state: &mut H) {}
+        }
+
+        impl<T> Debug for MyEventBarrier<T> {
+            fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                Ok(())
+            }
+        }
+
+        #[derive(SystemParam)]
+        struct MyBarrierEventReader<'w, 's, T: Event> {
+            reader: EventReader<'w, 's, T>,
+            _before: Before<MyEventBarrier<T>>,
+        }
+
+        #[derive(SystemParam)]
+        struct MyBarrierEventWriter<'w, T: Event> {
+            writer: EventWriter<'w, T>,
+            _after: After<MyEventBarrier<T>>,
+        }
+
+        #[derive(Event)]
+        struct MyEvent;
+
+        #[derive(Resource)]
+        struct MyRes(bool);
+
+        fn read(mut reader: MyBarrierEventReader<MyEvent>) {
+            assert_eq!(1, reader.reader.read().count());
+        }
+
+        fn write(mut writer: MyBarrierEventWriter<MyEvent>, mut res: ResMut<MyRes>) {
+            writer.writer.send(MyEvent);
+            assert_eq!(false, res.0);
+            res.0 = true;
+        }
+
+        let mut schedule = Schedule::default();
+
+        let mut world = World::default();
+        world.insert_resource(MyRes(false));
+        world.init_resource::<Events<MyEvent>>();
+
+        schedule.add_systems((read, write));
+
+        schedule.run(&mut world);
+
+        assert_eq!(true, world.resource::<MyRes>().0);
+    }
 }

crates/bevy_ecs/src/system/adapter_system.rs

@@ -146,6 +146,14 @@ where
     fn default_system_sets(&self) -> Vec<InternedSystemSet> {
         self.system.default_system_sets()
     }
+
+    fn before_list(&self) -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
+
+    fn after_list(&self) -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
 }
 
 // SAFETY: The inner system is read-only.

crates/bevy_ecs/src/system/barrier.rs

@@ -0,0 +1,119 @@
+use bevy_utils::all_tuples;
+
+use crate::component::Tick;
+use crate::prelude::World;
+use crate::schedule::{InternedSystemSet, SystemSet};
+use crate::system::{SystemMeta, SystemParam};
+use crate::world::unsafe_world_cell::UnsafeWorldCell;
+use std::marker::PhantomData;
+
+/// List of barrier dependencies for the system.
+pub trait BarrierList {
+    /// System sets before this system.
+    fn before_list() -> Vec<InternedSystemSet>;
+    /// System sets after this system.
+    fn after_list() -> Vec<InternedSystemSet>;
+}
+
+/// System param to mark current system to run before a given system set.
+pub struct Before<S: SystemSet + Default> {
+    marker: PhantomData<S>,
+}
+
+/// System param to mark current system to run after a given system set.
+pub struct After<S: SystemSet + Default> {
+    marker: PhantomData<S>,
+}
+
+unsafe impl<S: SystemSet + Default> SystemParam for Before<S> {
+    type State = ();
+    type Item<'world, 'state> = Self;
+    type BarrierList = BeforeBarrierList<S>;
+
+    fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {
+        ()
+    }
+
+    unsafe fn get_param<'world, 'state>(
+        _state: &'state mut Self::State,
+        _system_meta: &SystemMeta,
+        _world: UnsafeWorldCell<'world>,
+        _change_tick: Tick,
+    ) -> Self::Item<'world, 'state> {
+        Before {
+            marker: PhantomData,
+        }
+    }
+}
+
+unsafe impl<S: SystemSet + Default> SystemParam for After<S> {
+    type State = ();
+    type Item<'world, 'state> = Self;
+
+    type BarrierList = AfterBarrierList<S>;
+
+    fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {
+        ()
+    }
+
+    unsafe fn get_param<'world, 'state>(
+        _state: &'state mut Self::State,
+        _system_meta: &SystemMeta,
+        _world: UnsafeWorldCell<'world>,
+        _change_tick: Tick,
+    ) -> Self::Item<'world, 'state> {
+        After {
+            marker: PhantomData,
+        }
+    }
+}
+
+#[doc(hidden)]
+pub struct BeforeBarrierList<S: SystemSet + Default>(PhantomData<S>);
+#[doc(hidden)]
+pub struct AfterBarrierList<S: SystemSet + Default>(PhantomData<S>);
+
+impl<S: SystemSet + Default> BarrierList for BeforeBarrierList<S> {
+    fn before_list() -> Vec<InternedSystemSet> {
+        vec![S::default().intern()]
+    }
+
+    fn after_list() -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
+}
+
+impl<S: SystemSet + Default> BarrierList for AfterBarrierList<S> {
+    fn before_list() -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
+
+    fn after_list() -> Vec<InternedSystemSet> {
+        vec![S::default().intern()]
+    }
+}
+
+macro_rules! barrier_list_for_tuple {
+    ($($barrier: ident),*) => {
+        impl<$($barrier),*> BarrierList for ($($barrier,)*)
+        where
+            $($barrier: BarrierList),*
+        {
+            fn before_list() -> Vec<InternedSystemSet> {
+                let mut list = Vec::new();
+                let _ignore_empty = &mut list;
+                $(list.extend($barrier::before_list());)*
+                list
+            }
+
+            fn after_list() -> Vec<InternedSystemSet> {
+                let mut list = Vec::new();
+                let _ignore_empty = &mut list;
+                $(list.extend($barrier::after_list());)*
+                list
+            }
+        }
+    }
+}
+
+all_tuples!(barrier_list_for_tuple, 0, 16, P);

crates/bevy_ecs/src/system/combinator.rs

@@ -232,6 +232,22 @@ where
         default_sets.append(&mut self.b.default_system_sets());
         default_sets
     }
+
+    fn before_list(&self) -> Vec<InternedSystemSet> {
+        self.a
+            .before_list()
+            .into_iter()
+            .chain(self.b.before_list())
+            .collect()
+    }
+
+    fn after_list(&self) -> Vec<InternedSystemSet> {
+        self.a
+            .after_list()
+            .into_iter()
+            .chain(self.b.after_list())
+            .collect()
+    }
 }
 
 /// SAFETY: Both systems are read-only, so any system created by combining them will only read from the world.

crates/bevy_ecs/src/system/exclusive_function_system.rs

@@ -155,6 +155,14 @@ where
         let set = crate::schedule::SystemTypeSet::<F>::new();
         vec![set.intern()]
     }
+
+    fn before_list(&self) -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
+
+    fn after_list(&self) -> Vec<InternedSystemSet> {
+        Vec::new()
+    }
 }
 
 /// A trait implemented for all exclusive system functions that can be used as [`System`]s.

crates/bevy_ecs/src/system/function_system.rs

@@ -11,6 +11,7 @@ use crate::{
 use bevy_utils::all_tuples;
 use std::{any::TypeId, borrow::Cow, marker::PhantomData};
 
+use crate::system::barrier::BarrierList;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::{info_span, Span};
 
@@ -533,6 +534,14 @@ where
         let set = crate::schedule::SystemTypeSet::<F>::new();
         vec![set.intern()]
     }
+
+    fn before_list(&self) -> Vec<InternedSystemSet> {
+        <F::Param as SystemParam>::BarrierList::before_list()
+    }
+
+    fn after_list(&self) -> Vec<InternedSystemSet> {
+        <F::Param as SystemParam>::BarrierList::after_list()
+    }
 }
 
 /// SAFETY: `F`'s param is [`ReadOnlySystemParam`], so this system will only read from the world.

crates/bevy_ecs/src/system/mod.rs

@@ -103,6 +103,7 @@
 //! - [`()` (unit primitive type)](https://doc.rust-lang.org/stable/std/primitive.unit.html)
 
 mod adapter_system;
+mod barrier;
 mod combinator;
 mod commands;
 mod exclusive_function_system;
@@ -117,6 +118,7 @@ mod system_registry;
 use std::borrow::Cow;
 
 pub use adapter_system::*;
+pub use barrier::*;
 pub use combinator::*;
 pub use commands::*;
 pub use exclusive_function_system::*;

crates/bevy_ecs/src/system/system.rs

@@ -39,6 +39,9 @@ pub trait System: Send + Sync + 'static {
     /// Returns true if the system is [`Send`].
     fn is_send(&self) -> bool;
 
+    fn before_list(&self) -> Vec<InternedSystemSet>;
+    fn after_list(&self) -> Vec<InternedSystemSet>;
+
     /// Returns true if the system must be run exclusively.
     fn is_exclusive(&self) -> bool;