Stageless Problem: User-Added Systems Should "Just Work"

[alice-i-cecile]

Most user-written systems have to do with "gameplay logic".
These systems move enitities around, work with users defined constructs like gold, mana or damage, respond to player input and create effects to be rendered.
Engine and plugin code, by contrast, tends to handle the lower-level primitives needed to make all this work: input, audio, rendering, maintenance and so on.

Within a frame, we can generally divide our systems into three groups:

• things that should happen before gameplay code
  • updating time
  • input processing
• gameplay code
• things that should happen after gameplay code
  • rendering
  • audio
  • transform propagation

By default, user-added systems should be treated as gameplay code, and ordering them relative to "engine" code should generally not be a large concern.

Current Solution: CoreStage::Update

Bevy's initial solution to this was simple.
Create a CoreStage enum, which lays out the different stages of each frame.

enum CoreStage {
    PreUpdate,
    Update,
    PostUpdate
}

PreUpdate runs first, then Update, then PostUpdate.
User systems are added to Update by default, and all was good with the world.

The Problem with Stages, in a Nutshell

As evidenced by the fact that we wrote an entire RFC about the problems caused by stages and their kin, this approach was not idyllic.

Let's focus on the most relevant problems:

1. Systems in one stage could not see each other, in any form, as they were stored in distinct objects.
   1. Suppose we know that A must run before B. We tell it to run before B. If they're in the same stage, this works great! If they're not, we get an inscrutable error discussing how B was not found. If we start in the same stage, and then refactor A or B to live in a different stage, we must hold this constraint (and dozens like it) in our faulty human memory, because it cannot be encoded.
   2. This is particularly painful as a plugin author. If we have plugin_system that must run after engine_system, we cannot simply loosely specify this constraint, and then let the end user precisely coordinate things to match their needs. Instead, we can either overspecify, breaking many users and taking a performance hit, or simply tell users to "read the docs" (which works as well as you might expect).
   3. Higher order scheduling contructs, such as run criteria or states, simply could not be shared across these divisions.
2. Command flush points were coupled one-to-one with broad-grained tools for system organization.
   1. This caused pointless bottlenecking, as all systems needed to come to a halt before the next organizational unit could start.
   2. This introduced severe frustration when refactoring, as there was no way to split a stage in two by introducing a command flush point.
   3. This made commands very implicit, making "when are commands applied" hard to visualize, teach and debug.
3. System organization was hard-coded, and nearly impossible to modify as an end user.
   1. Even with access to APIs to apply additional labels and ordering constraints to various third-party systems, users hands were fundamnetally tied. Moving system objects between different containers is too complex, costly and privacy-breaking to be exposed.

Yeeting Stages == Yeeting Common Sense Defaults?

Ultimately, the stageless RFC said "thou store all of your systems in a single container*".
Stages are out, systems sets (previously labels) are in, command flush points (via apply_system_buffers) are just scheduled like any other system.
See #6587 for the initial implementation.

To ease migration (#7267) we replaced CoreStage with CoreSet.
All of the same sync points are there, and systems have been ported over one-to-one whenever possible.
Since App::add_system added things to CoreStage::Update, it should add things to CoreSet::Update instead now, right?

Right?

Nope, that doesn't work.
A short list of the problems with that idea:

1. CoreSet::Update is magical and special-cased. We could make this configurable, but that leads to complex internals and spooky action at a distance when you change the default set before adding a third-party plugin.
2. app.add_system(late_system.in_set(CoreSet::PostUpdate)) panics, as late_system is implicitly added to CoreSet::Update, which must run strictly before CoreSet::PostUpdate.
3. app.add_system(late_system.after(TransformPropagateSystem)) panics, as TransformPropagateSystem is in CoreSet::PostUpdate.
4. How does this work with other schedules? Does FixedTimestep or RenderSchedule need their own default sets?

We can try and fix these footguns by detecting these cases and adding configurability, but this sort of intricate pile of patches is hard to learn and a clear sign that something is deeply wrong with our solution.

Currently, #7267 has punted on this whole problem, given up on default system sets and just manually stuck systems in CoreSet::Update when needed.
Great for getting that monster merged, not a good plan to ship.
We need to solve this, one way or another, before we ship the next version of Bevy to users.

*unless they don't follow the linear looping control flow, in which case you should run a schedule in an exclusive system.

Solutions and Evaluation Criteria

Now comes the fun bit! You, dear reader, get to propose, comment on and generally bikeshed proposals to fix this!
In the discussion below, make a new top-level reply for each proposal.

Here are some criteria to think about and evaluate when considering proposals.

1. Ergonomic: Simply calling app.add_system(my_system) should cause my_system run before / after things they should generally run before / after.
2. Correct: If users add a system which is accidentally badly ordered, they should always receive an actionable warning.
3. Clear: any new concepts should be intuitive with simple rules.
4. Free there should be minimal run-time performance cost, as we should be able to do all of our work up-front in schedule creation.
5. Elegant: Avoid special-casing the "standard" app setup, although you can add sugary methods on App to make it nicer.
6. Cheap: It should be feasible for our ECS experts to quickly implement your solution.
7. No regressions: not reintroduce any of the problems above with stages.

Our goals:

• minimize what users have to specify in common setups, without preventing or making it hard to have explicit control
• if there are defaults, it should be simple to support them (requiring the addition of 200+ LoC internally, very subtle implicit behavior, or introducing errors isn't a good sign)
• the user-facing API should not influence or be influenced by the way systems are contained

[alice-i-cecile]

Proposal: Fundamental sets

1. Define an extensible group of "fundamental system sets" for each schedule.
2. Fundamental system sets must be well-ordered relative to each other.
3. Each system must belong to at least one fundamental system set.
4. Optionally, specfy a "default system set".

Obviously, CoreSet / StartupSet / RenderSet would be used as the starting fundamental sets for their respective schedules.

Evaluation

1. Ergonomic: Yep, especially if we add systems to a fundamental set by default!
2. Correct: Yep: we can catch all of the failures.
3. Clear: Simple and easy to migrate to.
4. Free Yes.
5. Elegant: Eh, it's a bit hacky.
6. Cheap: Easy as heck to implement.
7. No regressions: Probably doesn't regress, although it's quite rigid.

[alice-i-cecile]

IMO this is straightforward but a bit hacky. Very easy for users to resolve, and easy to teach.

I wouldn't hate to see this.

[alice-i-cecile]

Proposal: Contiguous schedule graph

1. After building the schedule, check to see if it has any disconnected components.
2. If it does, error out, reporting which systems were disconnected.

The basic idea here is that disconnected systems are fully unmoored from the schedule, and as a result can be run at any random point in the frame.
This is probably a mistake, so we should error out.

Optionally, we can try automatically repairing this error by adding a default set.

Evaluation

1. Ergonomic: Maybe. Depends on if we add a default set on error.
2. Correct: No: systems can be specified to e.g. run after TimeSystem, and while this still connects them, is not going to result in happy behavior.
3. Clear: Eh... this is a lot of graph theory to throw at people, and I'm not sure the error messages can be great.
4. Free Yes.
5. Elegant: So pretty!
6. Cheap: Standard graph algorithms go brr.
7. No regressions: No structure, no regressions! Really only logging systems should (maybe) be free-floating.

[alice-i-cecile]

This solution leads to bad error messages, still requires explicit labelling, and doesn't actually catch all the problems. I don't think this should be seriously considered.

[james7132]

Reposting the discussion from Discord:

The basic idea here is that disconnected systems are fully unmoored from the schedule, and as a result can be run at any random point in the frame.
This is probably a mistake, so we should error out.

Is it an error? Seems to me if a disconnected subgraph exists and it has no ambiguities with the rest of the graph, it seems like it could run at any time, and we should greedily run it whenever there's CPU capacity to do so.

It may be better to rephrase that as "disconnected subgraphs with ambiguities" should be an error.

With this, I posit that, if the ambiguity detector is not viable right now due to accuracy issues, perhaps we should consider using this approach with the ambiguity detector once we've refined it's false positive rate.

[alice-i-cecile]

Proposal: Just use the ambiguity detector

The system ambiguity order detector is a powerful and fairly fundamental tool.

Systems that are unmoored (see above) will have a huge number of ambgiuities, alerting users to the problem.

Although it pains my heart to say it, this is a bad solution that doesn't even make it to the evaluation stage.
There are still too many false positives, the configuration tools aren't powerful enough, and the reporting isn't good enough.

[alice-i-cecile]

🥲 One day...

[alice-i-cecile]

Proposal: Exclusive system ambgiuities are fatal

When detecting execution order ambiguities between systems, error out if any exclusive system has any ambiguities.

The logic is twofold:

1. Unmoored systems will always fail this test.
2. Ambiguities with (and especially between!) exclusive systems are very dangerous due to the far-reaching capabilities, and are almost always a genuine bug.

Optionally, we can try automatically repairing this error by adding a default set.

Evaluation

1. Ergonomic: Maybe. Depends on if we add a default set on error. Errors may be hard to resolve still.
2. Correct: Yes! Unlike the "disconnected components"
3. Clear: Yep: we're just reusing existing concepts and enforcing an implicit stage-like structure to the app.
4. Free Yes.
5. Elegant: 😎
6. Cheap: We're literally already computing these.
7. No regressions: No structure, no regressions!

[alice-i-cecile]

A much better version of the "no disconnected components in the system graph" idea, crossed with the flavor of fundamental sets. I quite like this, but worry about the UX.

[alice-i-cecile]

Proposal: System ordering hints

/// Suggests to the scheduler how to break ties
enum OrderingHint {
	/// If this system is ambiguous with a `None` or `Late` system, it runs first.
	Early,
	/// If this system is ambiguous with an `Early` system it runs second. With `Late` systems, it runs first.
	Neutral,
	/// If this system is ambiguous with a `Early` or `None` system, it runs second.
	Late,
}

// When setting up the core sets
app.configure_set(CoreSet::PreUpdate.hint(OrderingHint::Early);
app.configure_set(CoreSet::PostUpdate.hint(OrderingHint::Late);

Systems in CoreSet::PreUpdate and before are given SystemOrderingHint::Early, while those in CoreSet::PostUpdate and later get SystemOrderingHint::Late.
By default, all systems have SystemOrderingHint::Neutral.

By embedding our desired behavior directly into the scheduling graph, we can cause ties to break the way we want, without having to add additional structure.
These hints can be freely overridden by an explicit ordering, and so users still retain full control and the cycle problems of default system sets are avoided.

Evaluation

1. Ergonomic: Yes.
2. Correct: Yes.
3. Clear: Yes: this is a pretty direct extension with a simple explanation.
4. Free Yes.
5. Elegant: Yes! I'm excited to see what users do with this new primitive.
6. Cheap: Probably involves some trickery in the internals, but shouldn't be too bad.
7. No regressions: No regressions!

[alice-i-cecile]

Both per system and to entire sets, using the exact same techniques as other system configuration. We probably want a "specific overrides general" logic somewhere, but that's not unique to this.

[james7132]

It's unclear what happens when you have multiple Early or Late systems that are ambiguous with each other. In this case, it seems like we're just kicking the problem down the line and only solves the initial first order ambiguities. Is this enough in practice?

[alice-i-cecile]

I think that for this narrow problem, yes. Gameplay systems will generally want to break ties in these ways.

Ultimately I think it's one tool that should be combined with more powerful scheduling and especially debugging + visualization strategies.

[stephenmartindale]

What I would ask for is very clear messaging on what one SHOULD expect. First, so that users can quickly recognise when they're wrong, themselves, or when it is a bug. Secondly, so that bug reports can clearly refer to what is "supposed to happen" and what "actually happens".

	/// If this system is ambiguous with an `Early` system it runs second. With `Late` systems, it runs first.
	Neutral,

↑ That. Lots of that. Unambiguous. Clear. Easily refuted if a bug is found. Easily amended if it is discovered that this was actually not the intended design.

[stephenmartindale]

Wouldn't it be grand if, after the app-building is done:

⋮
// When setting up the core sets
app.configure_set(CoreSet::PreUpdate.hint(OrderingHint::Early);
app.configure_set(CoreSet::PostUpdate.hint(OrderingHint::Late);
⋮
// Test system scheduling preconditions
assert_set_order!(app,
   CoreSet::PostUpdate.not_before(CoreSet::PreUpdate),
   MyUserSystem.after(CoreSet::PostUpdate),
)

I don't think a DSL like that would be wildly beyond the capabilities of Rust's macros and a lot of user code does have "invariants" like "input is always scheduled before player-update" etc. It would also be hugely useful in doc tests.