Add SystemGraph as a handle-based method for explicitly creating system dependency graphs

[james7132]

Objective

This PR adds SystemGraph as way to create system dependency graphs without explicit labeling.

Motivation

It is currently difficult to force a strict system dependency graph. Currently the only way is manually set label, before, and/or after on each system. This can lead to some particularly boilerplate/non-ergonomic builder code that is difficult to read, harder to think about, and more prone to programmer error. An example of this kind of code can be found here: https://github.com/HouraiTeahouse/FantasyCresendoBevy/blob/master/src/match/mod.rs#L248.

Natural ordering invariants in game dev are common, and this should provide a simpler way to define those requirements. One notable case is the common simple sequential execution. Deterministic simulation is much easier to reason about with a single execution order, which should make it easier to create networked games that rely on determinism.

Solution

This PR adds SystemGraph. It provides a handle based API for creating dependency graphs, and can be converted into SystemSets. It enforces ordering by generating NodeIds, a newtype around u32, as a globally unique system label, that is then used to label and order systems relative to each other.

This should be able to be used alongside "normal" SystemLabels to allow for ordering other systems relative any system in the SystemSet.

As for the choice to use AtomicU32, unless a developer is adding >2^32 systems to a stage, there will be no collisions. If multiple Apps are set up and recycled in the same process (i.e. headless server environments spinning up game instances), the static AtomicU32 uses wrapping adds when generating IDs, so there is no risk of crashing from an overflow.

[NathanSWard]

It would be nice to add a test to ensue the is working as intended (it looks good to me, but tests always help)

[mockersf]

bors try

[bors]

try

Build failed:

• build (stable, ubuntu-latest)

[james7132]

It would be nice to add a test to ensue the is working as intended (it looks good to me, but tests always help)

Added two tests for ensuring that labels were generated in sequence when sequential is enabled and are not present otherwise.

[NathanSWard]

This looks good to me. 👍
I'm still not the biggest fan of the global AtomicU32, however, I don't know of another way to implement this without risks of label collision, so the current impl should suffice :)

[james7132]

Reproducing some points from a followup discussion on Discord:

• The end goal is to produce a distinct group of system DAGs
• Labels only exist at app build time, not during actual execution.
• Adding a large number of bevy_ecs provided labels may end up being noisier with tools that visualize the execution graph.
• Ultimately it boils down to specifying dependencies in ParallelSystemContainer::dependencies.
• It's best not to have multiple confusing ways of specifying system execution dependencies (i.e. labels, SystemSet::as_sequential, etc.)

IMO, explicitly labeling can be quite error prone for common use cases, particularly with larger and more complex dependency graphs. However, labels still have their use in exposing dependencies to other crates. Likewise, with these points, it's likely that SystemSet::as_sequential is not going to cut it for these kinds of use cases, and may end up just fragmenting the many ways to configure system dependencies.

The dependency graph seems very similar to how some big data pipeline services and frameworks like Apache BEAM and Google Cloud DataFlow build their DAGs. An example of one of these DAGs can be seen here:

These frameworks often have a graph handle based implementation of defining them in code. Here's a possible example of how such an API might look:

let graph = SystemGraph::new();
let start_a = graph.root(sys_a.system().label("ENTRY_A"));
let start_b = graph.root(sys_b.system().label("ENTRY_B"));

let mid_c = start_a.then(sys_c.system());
let mid_d = start_a.then(sys_d.system());

(start_b, mid_c, mid_d)
  .join()
  .then(sys_e.system().label("EXIT_E"));

App::build().add_system_set(graph.into());

This should provide a general solution that enables every possible dependency graph without explicit labeling. An example that SystemSet::as_sequential can cover right now:

let graph = SystemGraph::new();
graph.root(sys_a.system())
  .then(sys_b.system())
  .then(sys_c.system())
  .then(sys_d.system())
  .then(sys_e.system());
App::build().add_system_set(graph.into());

A public interface can be relatively easily made building on top of what is in this PR using labels, and be switched to a label-less implementation later.

Would something like this need a RFC? As proposed here, this could be implemented as a utility ecosystem crate since it simply builds a SystemSet, but it would require first-party integration when migrating to a label-less implementation.

[james7132]

Pushed a tentative implementation of a SystemGraph API loosely based on the SystemSet::as_sequential implementation. This should allow creating SystemSets that as_sequential allowed, but also support both of the fan-in/fan-out topologies as well.

This implementation uses Arc/Mutex to ensure the resultant types are Send/Sync, but it's not completely necessary and can be done with Rc/RefCell instead.

[NathanSWard]

This implementation uses Arc/Mutex to ensure the resultant types are Send/Sync, but it's not completely necessary and can be done with Rc/RefCell instead.

If we don't need Arc/Mutex and can get away with Rc/RefCell, we should imo :)

[NathanSWard]

I really like where this is headed 😄
I think using graph-like syntax to define the dependencies of systems is really nice.
Also, I also like the push to move away from a label-based API

[james7132]

Added a split_into utility function for simplifying fan-out topologies. Had to add a fix to bevy_ecs_macros to support tuple starts at something other than 0.

[bjorn3]

As for the choice to use AtomicU32, unless a developer is adding >2^32 systems to a stage, there will be no collisions. If multiple Apps are set up and recycled in the same process (i.e. headless server environments spinning up game instances), the static AtomicU32 uses wrapping adds when generating IDs, so there is no risk of crashing from an overflow.

That will cause it to silently misbehave when adding >2^32 systems to a stage or if you add one system to one stage, 2^32 to another stage and then one to the first stage. Maybe you could have the counter local to each SystemGraph and panic on overflow. The label can then contain the local counter and a global SystemGraph counter.

[james7132]

Implemented the panicking add and atomic graph/node ID split. Ultimately (as in, not in this PR), however, it's likely desirable to not use labels at all and just opt for writing the metadata to Parallel/ExclusiveSystemContainer.

[NathanSWard]

This looks really good :)
Sorry to be a bother haha, but the last thing I'd add is we should provide an example in the examples/ directory.
While the doc examples is a great start, actually seeing this in a runnable example would be very nice imo 😄

[james7132]

bors try

[bors]

try

Build failed:

• ci

[james7132]

Made the main chunk of this PR available as an ecosystem crate in the form of bevy_system_graph. I do want some form of this as a first-party solution since this seems like something users generally need in a standardized way, but with stageless on the horizon I'll be closing this and opening a new PR once stageless has landed in some form.

The fixes to all_tuples macro, I'll split into a separate PR.