bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Schedule-First: the new and improved add_systems

Open cart opened this issue 2 years ago • 5 comments

Objectives

While these changes are relatively straightforward, this is a very complicated and controversial topic. Please read this entire description before sharing your thoughts.

Resolve "Base Set" confusion

The "base set" concept introduced in Bevy 0.10 is more confusing than we would like:

  • It is a new concept that users need to contend with. What is a base set? When do I use them?
  • It was easy to accidentally create invalid orderings: add_system(foo.after(TransformPropagate)) would fail because foo is implicitly in CoreSet::Update by default, but transform propagation happens in CoreSet::PostUpdate.
  • They were largely added to enable "default base sets", which enabled add_system(foo) to be automatically added to CoreSet::Update, ensuring that foo can interact with "engine features" without ordering itself relative to all present (and future) "engine features". This produced roughly equivalent behavior to the old "default update stage" in Bevy 0.9 and below.

Systems must work by default

We can't just remove "base sets", as they enable normal "update" systems to work "as expected" automatically. Without a solution to this problem, add_system(foo) would be unconstrained in the schedule. Engine features like events, assets, transforms, animation, etc would silently fail for users. It is completely unreasonable to expect Bevy users to be aware of all present and future "built in" and 3rd party engine systems.

Unify system apis

We have way too many ways to add systems to schedules:

  • add_system(foo): add a single system to the default schedule
  • add_systems((foo, bar)): add multiple systems to the default schedule
  • add_system(foo.in_schedule(CoreSchedule::Startup)): add one system to a specified schedule
  • add_systems((foo, bar).in_schedule(CoreSchedule::Startup)): add multiple systems to a specified schedule
  • add_startup_system(foo): add a system to the startup schedule
  • add_startup_systems((foo, bar)): add multiple systems to the startup schedule
  • add_system(foo.on_startup()): add one systems to the startup schedule
  • add_systems((foo, bar).on_startup()): add multiple systems to the startup schedule

Note that we have 6 different ways to add systems to the startup schedule!

Adding systems to schedules should be ergonomic and extensible

  • add_system(foo.in_schedule(CoreSchedule::Startup)) is a lot to type. Naturally people gravitate to alternatives that are shorter and read better: add_startup_system(foo) and add_system(foo.on_startup())
  • Both of the current "ergonomic startup" solutions are hard-coded and startup-specific. Developers that want equivalents for other schedules must define new custom hard coded apis (ex: on_enter and on_exit have been proposed). This is not tenable, especially given that extending the SystemConfig api is non trivial (we have multiple SystemConfig and SystemSetConfig variants and users would need to define a custom trait and implement it for all of them).
  • CoreSchedule::Startup is a long name. People don't want to type it or look at it.

A system's Schedule (and purpose) should be clear

  • Bevy apps currently have a "default schedule". This defaults to the Main schedule, but users are often not aware of this. This is "implied context". And that context changes across apps. Confusing!
  • Bevy schedules currently have a "default base set". This defaults to Update for the Main schedule, but users are often not aware of this. This is "implied context". And that context changes across schedules. Confusing!
  • Both of the current "ergonomic schedule patterns" (add_startup_system, and foo.on_startup()) completely erase the actual schedule type!

Expressing invalid system configuration should be impossible, when possible

Chaining multiple schedules is currently possible: add_system(foo.in_schedule(A).in_schedule(B)). The on_x() pattern is especially hazardous, as users could easily think "I want to run this system both on_enter and on_exit". But foo.on_enter(SomeState::X).on_exit(SomeState::Y) is invalid! Ideally these patterns are not expressible.

Solution

Note that this is only a proposal, but I personally believe this is by far the best path forward. This is the result of much discussion and user feedback. But if you believe there is a better solution, please let us know!

The new add_systems ... one api for everything

There is now exactly one API for adding systems to schedules:

app
    // No more add_startup_system/add_startup_systems/on_startup/in_schedule(Startup)
    .add_systems(Startup, (a, b))
    // The Update schedule context is now explicit 
    .add_systems(Update, (c, d, e))
    // This pairs very nicely with FixedUpdate
    .add_systems(FixedUpdate, (f, g))
    // You can now add single systems with add_systems!
    .add_systems(PostUpdate, h)
    .add_systems(OnEnter(AppState::Menu), enter_menu)
    .add_systems(OnExit(AppState::Menu), exit_menu)

Lets take a moment to appreciate how explicit and consistent this is, while still being short and sweet. No implied context. We use the same "schedule pattern" for every "phase" of the Bevy App. Moving systems to a different phase is as simple as changing the schedule name. The schedule names align vertically. You can take a look at this App and immediately know its structure and roughly what it does.

Compare that to what it used to be:

app
    // Startup system variant 1. Has an implied default StartupSet::Startup base set
    .add_startup_systems((a, b))
    // Startup system variant 2. Has an implied default StartupSet::Startup base set
    .add_systems((a, b).on_startup())
    // Startup system variant 3. Has an implied default StartupSet::Startup base set
    .add_systems((a, b).in_schedule(CoreSchedule::Startup))
    // The `CoreSet::Update` base set and `CoreSchedule::Main` are implied
    .add_systems((c, d, e))
    // This has no implied default base set
    .add_systems((f, g).in_schedule(CoreSchedule::FixedUpdate))
    // Base sets are used for PostUpdate, `CoreSchedule::Main` is implied
    .add_systems(h.in_base_set(CoreSet::PostUpdate))
    // This has no implied default base set
    .add_systems(enter_menu.in_schedule(OnEnter(AppState::Menu)))
    // This has no implied default base set
    .add_systems(exit_menu.in_schedule(OnExit(AppState::Menu)))

Note that this does not mean we're returning to a "stages/schedules only" world. Within each schedule, we still have all of the Schedule v3 niceties that we had before. Additionaly, "base sets" already had hard sync points between them. PreUpdate/Update/PostUpdate exist to draw hard lines between these phases (enabling "implied dependencies"), so I see promoting them to schedules as a natural, roughly lateral progression. However if this rubs you the wrong way, please see Implied Dependencies in the next steps section. There is a world where we can actually meaningfully dissolve these hard lines.

And yes, this does mean you can no longer just type:

app.add_systems((foo, bar))

You must type

app.add_systems(Update, (foo, bar))

I do anticipate some people preferring the ergonomics of add_systems((foo, bar)). It is certainly nice to look at! But I believe specifying the schedule is extremely worth it when we consider the global picture. Everything becomes so much simpler, clearer, and consistent when we put schedules first.

add_system, add_startup_system, add_startup_systems, in_schedule, and on_startup() have been deprecated in favor of the new add_systems

Nested System Tuples and Chaining

It is now possible to infinitely nest tuples in a single .add_systems call:

// Nest as much as you want!
app.add_systems(Update (
    (a, (b, c, d, e), f),
    (g, h),
    i
))

This enables us to get past the arbitrary 15-items-per-tuple limit and allows users to organize their systems more effectively.

Nested tuples can also have configuration, which makes configuration much more flexible:

app.add_systems(Update (
    (a, b).before(c).in_set(X)
    c,
    (d, e).after(c)
))

This is especially powerful because chain() can now be nested!

app.add_systems(Update, (
    (a, b, c).chain(),
    (d, e),
).chain())

Nested chaining will logically ensure each entry in the chain (either a system or a group of systems) will run in the order it was defined.

For example this:

app.add_systems(Update, (
  (a, (b, c))
  d,
  (e, f)
).chain())

Implies a->d, b->d, c->d, d->e, d->f. This makes it much easier to express graph-like orderings (that still preserve parallelism where necessary).

Chaining has two optimizations implemented:

  1. Nested chaining necessitates internally collecting the entire hierarchy of NodeIds required for the chain. However we only allocate for a given tuple of systems if an ancestor tuple requires it. Ex: we won't allocate lists of NodeIds at all in this situation:
    app.add_systems(Update, ((a, b), c, (d, e)))
    
  2. In cases where a given "scope" is "densely chained" (aka all items in the scope, no matter how nested they are, are chained), we only chain the first or last item (where relevant). For most reasonable chains this should keep the graph nice and slim / easy to look at:
    // (a, b, c) is "densely chained", so we only need to add `d.after(c)` and `e.after(c)`
    // to implement the top level chain 
    app.add_systems(Update, (
    (a, b, c).chain(),
    (d, e),
    ).chain())
    

No more implied context

"Default base sets" and "default schedules" have both been removed.

"Base sets" have been removed

We don't need them anymore now that we don't have implicit defaults.

configure_set now accepts a schedule

This creates parity with the new add_systems API and resolves the common problem of forgetting that set configuration only applies to specific schedules:

// before
app.configure_set(Foo.after(Bar).in_schedule(PostUpdate))
// after
app.configure_set(PostUpdate, Foo.after(Bar))

// before: defaults to the Main schedule
app.configure_set(Foo.after(Bar))
// after: explicit
app.configure_set(Update, Foo.after(Bar))

The new Main Schedule

  • No more CoreSchedule::Outer schedule. It has been merged into the new Main schedule. (and App::outer_schedule_label concept has been renamed to App:main_schedule_label)
  • The CoreSet base set has been removed in favor of nice and short: First, PreUpdate, Update, PostUpdate, StateTransition, and Last schedules. No need for "flush" variants because schedules flush automatically
  • The CoreSchedule enum has been broken up into Startup, Main, FixedUpdate, PreStartup, and PostStartup schedules (joining the new schedules from the previous point) and the StartupSet base set has been removed.
  • The FixedUpdateLoop schedule has been added to facilitate running the FixedUpdate loop
  • The MainScheduleOrderresource has been added, which defines the schedules the Main schedule will run (and their order). This allows plugin authors to insert schedules if that is necessary (but it should generally be discouraged in favor of adding ordered sets to existing schedules).

Render schedule

The prepare, queue, render, etc sets now live in the new Render schedule in RenderApp.

One SystemConfigs struct and IntoSystemConfigs trait

This cuts down on code in our internals and also makes it easier for users to extend!

Todo for this PR

  • Add deprecated versions of in_schedule and on_startup to ease migration. These can panic with a migration notice (as we're doing a breaking change anyway).
  • Migration Guide and Changelog
  • Tests for nested chains and nested tuples. This logic is non-trivial and deserves solid validation.

Next Steps

  • run_if for sets OnUpdate removal: run_if is not implemented for SystemConfigs::Configs. We should seriously consider adding support for it via "anonymous system sets". If we do this, we can consider removing OnUpdate in favor of add_systems(Update, foo.run_if(in_state(MyState::A))). State API Ergonomics / Consistency: We should consider removing the OnX prefix from OnEnter and OnExit for shorter names and parity with the Startup "event-style" schedule. We should also consider renaming the state_equals run condition to in_state for less typing and better readability (foo.run_if(in_state(X)) vs foo.run_if(state_equals(X))).
  • Ensure visualization works properly: We have divided up the Main schedule a bit more than before. We should ensure https://github.com/jakobhellermann/bevy_mod_debugdump still produces useful outputs.
  • Explore Implied Dependency Patterns: The current PreUpdate/Update/PostUpdate pattern enables "engine internal" dependencies to be "implied", which allows developers writing Update code to do so without knowledge of internals. We should investigate other "implied dependency" patterns, especially ones that allow us to remove the hard lines and sync points between PreUpdate/Update/PostUpdate (ex: merge them all into an Update schedule). For example, we could devise a way for the schedule to see a normal Update system using Transform components to automatically add a before(TransformPropagation) dependency. Or a system that uses Events to add an after(EventBufferFlip) dependency. There is a lot to consider here. I have my doubts about complexity management (the current system is very straightforward which will be hard to beat). But we might be able to find something that we like even more!
  • Consider a combined system and system set api:
    • We could take this unification even further by adding something like:
    // before
    app
        .configure_sets(Update, MySet.after(Other))
        .add_systems(Update, foo.in_set(MySet))
    ))
    // after
    app.schedule(Update, (
        MySet.after(Other),
        foo.in_set(MySet),
    ))
    
    • This would likely pair very well "Schedule Templates" (see the next point)
  • Schedule Templates: Developers want to define reusable / flexible sets of schedule configuration in their plugins. A common request is to be able to add new transform propagation to arbitrary schedules. We already duplicate this for PostStartup and PostUpdate. It would be great to support something like:
    // In TransformPlugin
    app
        .schedule_template(TransformTemplate, || (
            PropagateTransformsSet.in_set(TransformSystem::TransformPropagate),
            sync_simple_transforms.in_set(TransformPropagation),
            propagate_transforms.in_set(PropagateTransformsSet),
        ))
        .add_template_to_schedule(PostStartup, TransformTemplate)
        .add_template_to_schedule(PostUpdate, TransformTemplate)
    
    // User app
    app.add_template_to_schedule(MyCustomSchedule, TransformTemplate)
    
  • Rename ExtractSchedule?: We should consider renaming ExtractSchedule to something like Extraction to better match the other shorter "built in schedule" naming conventions. Sadly it can't be Extract because we already use that for the extract system param.
  • Accessibility plugin systems should probably not be in Update: the new explicit add_systems api revealed this. They should probably "react" to Update changes in PostUpdate?
  • boxed schedule labels are silently broken with &label: &*label is required for things to work properly (not a problem introduced in this pr). This is a very easy mistake to make and it will fail silently by just treating the label as a different label value.

cart avatar Mar 14 '23 00:03 cart

Things I love

  • no base sets!
  • no implicit defaults
  • moving away from special implicit main schedule special-casing
  • shorter schedule names
  • consistent, smaller API
  • only two config traits!
  • no outer schedule!
  • nested chaining

Things I regret but don't see a better way around

  • no ability to order between schedules. This is an unfortunate regression, but ultimately didn't end up being super useful because you still couldn't order relative to state transitions or fixed update systems. Plugin configurability is the big thing there: the ScheduleTemplate idea we bikeshedded seems like a great future direction to resolve this.

Things I think should be changed

  • add_system and add_startup_system should use a deprecation warning
  • the arity for the tuple of systems allowed in add_systems should be increased significantly from 12. I would probably go with 20 or more.
  • bevy_asset never really needed its own stages, or flush points, or schedules. I'd really like to cut that out here, but we can punt if you think it's too unrelated

Things I'm conflicted on

  • automatically flushing commands after each schedule is a bit implicit. It won't hurt parallelism, but it does rule out potentially useful patterns where you delay applications. Probably a net win just because of how much it improves footguns and reduces the need for users to add flush points, especially for states

alice-i-cecile avatar Mar 14 '23 01:03 alice-i-cecile

@alice-i-cecile

no ability to order between schedules. This is an unfortunate regression, but ultimately didn't end up being super useful because you still couldn't order relative to state transitions or fixed update systems.

I think in practice ordering between schedules wasn't particularly relevant, as we already had the hard lines between each base set. I think its generally reasonable to embrace "phase ordering" to encode these ordering deps (even if it is a bit less explicit). We could probably add some sort of (before_across_schedule/after_across_schedule) constraint if people really want to express these deps directly.

Alternatively, if we roll with a more granular "implied deps" solution, this problem largely goes away as Pre+PostUpdate would be merged into Update.

add_system and add_startup_system should use a deprecation warning

They already do that! They use the deprecation attribute.

the arity for the tuple of systems allowed in add_systems should be increased significantly from 12. I would probably go with 20 or more.

The arity is already 15. 20 doesn't seem like too much of a stretch though.

bevy_asset never really needed its own stages, or flush points, or schedules. I'd really like to cut that out here, but we can punt if you think it's too unrelated

Yeah I don't want to rock the boat there. I think this requires some careful thought.

automatically flushing commands after each schedule is a bit implicit. It won't hurt parallelism, but it does rule out potentially useful patterns where you delay applications. Probably a net win just because of how much it improves footguns and reduces the need for users to add flush points, especially for states

Yeah I personally these flushes are desirable. If we allow work to "bleed outside" of schedules its harder to think about them as logical units of execution. And in pretty much all of these cases punting that work to users seems undesirable / easily forgettable / full of foot-guns. But this is already configurable per-schedule so we can easily test out alternatives if we want (ex: we disable flushing commands in the Extract schedule). But I think "flush by default" is the right call.

cart avatar Mar 14 '23 03:03 cart

Alternatively, if we roll with a more granular "implied deps" solution, this problem largely goes away as Pre+PostUpdate would be merged into Update.

Agreed, I'm not too fussed about losing this, although I am still hopeful that we're able to do this one day.

But I think "flush by default" is the right call.

Great, when I was reading the PR description I didn't realize that behavior was configurable! Fully on board with flush-at-end-of-schedule then. Strongly agree that this is the right default behavior.

alice-i-cecile avatar Mar 14 '23 03:03 alice-i-cecile

Haven't done a full review yet, but generally in favor of these changes. Definitely fixes a lot of the pain points that users are currently experiencing. This is a pretty conservative change that just uses the already existing building blocks. As you've stated we can try and figure out how to soften the boundaries between PreUpdate, Update, and PostUpdate in the future.

no ability to order between schedules

One of the biggest losses with this PR is that won't be able to enforce ordering between different schedule templates if they have some systems that run in one schedule and some in another. i.e. some systems in PreFixedUpdate and some in PostFixedUpdate. In practice I'm not sure how big of a problem it will be. There aren't a crazy amount of schedules to choose and it's more of an advanced feature where docs can give guidance for any required orderings.


I do want a follow up PR to this one to minimize the number of schedules we're using. Specifically I'd like to get rid of First, Last, and the asset schedules. Fewer hard boundaries between systems should still be a goal. Also fewer places for users to think about putting things will be good too. Users should be depending on system ordering and apply_system_buffers and not worry about putting something in First or PreUpdate. Another good reason to minimize schedules is that there is a thread context switch cost for each additional Schedule::run call (about 10-20us).

hymm avatar Mar 14 '23 06:03 hymm

Fantastic! I think this will solve a lot of the confusion users are having currently :)

I'll try porting some things to this branch to see how it goes.

tim-blackbird avatar Mar 14 '23 13:03 tim-blackbird

I ported bevy_rapier and had a pretty good time. The diff is nice too.

Things I've noticed:

  • Being able to nest SystemConfigs is great!
  • configure_set being kept while add_system is deprecated feels weird

tim-blackbird avatar Mar 14 '23 18:03 tim-blackbird

We should consider removing the OnX prefix from OnEnter and OnExit for shorter names and parity with the Startup "event-style" schedule.

I am just a little bit worried to end up with a little bit more difficult to distinguish similar-but-different names. Specifically, I mean this (invalid) chain of schedules I create in my head:

Enter
First
PreUpdate
Update
PostUpdate
Last 
Exit

They seem as a single family of a "thing that you use to define when to run your system", but, as far as I understand, Enter/Exit differ from the rest. They are not global, but relative to a state put as their param. Also, an extra bit of effort needed to distinguish what is the difference between Enter vs First and Exit vs Last.

Or maybe I just got biased by learning about OnEnter/OnExit and putting them into a separate box in my head (thanks to the prefix) i while I should not?


It was my only comment so far.

I read the whole description and am very happy with proposed changes 😄 ❤️

I especially love 2 changes, looking from the perspective of a newcomer who tries to wrap their head around Bevy's ECS:

  • explicitly defined schedules for systems = less "magic", less things that are not visible in code but one has to know them and remember them
  • smaller API surface = less confusion of type: "I used function A and things didn't work, then I used function B which probably should do the same, but now it does work, oh, there is also approach C and D… which one is the correct one? Or are they equivalent?"

beetrootpaul avatar Mar 14 '23 21:03 beetrootpaul

We should also consider renaming the state_equals run condition to in_state for less typing and better readability.

This is already in_state in 0.10. https://docs.rs/bevy/latest/bevy/ecs/prelude/fn.in_state.html

Consider a combined system and system set api

I've definitely been wanting this to do:

app.schedule(Update, 
    (
        MySet,
        apply_system_buffers.in_set(MySetFlush)
    ).chain()
);

hymm avatar Mar 15 '23 05:03 hymm

As a novice user that's implementing systems, ordering, etc. in the new Bevy 0.10 ecosystem, these changes are a breath of fresh air. I found a lot of the "magic" equated to looking through the Bevy source to understand the schedules anyway. Putting them front and center sounds like a great idea.

akappel avatar Mar 15 '23 20:03 akappel

@hymm

I do want a follow up PR to this one to minimize the number of schedules we're using. Specifically I'd like to get rid of First, Last, and the asset schedules. Fewer hard boundaries between systems should still be a goal. Also fewer places for users to think about putting things will be good too. Users should be depending on system ordering and apply_system_buffers and not worry about putting something in First or PreUpdate. Another good reason to minimize schedules is that there is a thread context switch cost for each additional Schedule::run call (about 10-20us).

Biggest issue with getting rid of First is that we can't run the timing system "truly first" anymore. This isn't just about dependency ordering (ex: ensuring that PreUpdate systems using time add a dependency on the time system/set). Its about "time correctness". If anyone forgets to add a dependency on time in PreUpdate, suddenly the frame time is messed up! We really do need time to run "first" with reasonably strong guarantees. However we do currently tick events in First (without a dependency on the time system), so thats not good.

cart avatar Mar 16 '23 07:03 cart

@beetrootpaul

They seem as a single family of a "thing that you use to define when to run your system", but, as far as I understand, Enter/Exit differ from the rest. They are not global, but relative to a state put as their param. Also, an extra bit of effort needed to distinguish what is the difference between Enter vs First and Exit vs Last.

I don't see this as a problem because it will never just be Enter. It will be Enter(MyState::Foo). This feels unambiguous and unconfusing to me.

cart avatar Mar 16 '23 07:03 cart

configure_set being kept while add_system is deprecated feels weird

I think we should probably do this as a followup. I agree, but merging these builder traits is slightly non-trivial (see the SystemConfig->SystemConfigs unification). We might even want to move this straight into the unified app.schedule API (By extending the new SystemConfigs to be something like ScheduleConfig and deprecating everything else)

cart avatar Mar 16 '23 07:03 cart

Biggest issue with getting rid of First is that we can't run the timing system "truly first" anymore.

We could potentially insert it into the Main schedule. I wouldn't want to encourage the pattern. But for a small number of engine internal systems, it might be acceptable.

hymm avatar Mar 17 '23 01:03 hymm

I agree with these changes. Observing some discussions in the Discord, there's a lot of confusion around the differences between base sets and schedules.

I wonder if there will be a need for PreUpdate and PostUpdate in fixed updates. I've created an issue related to this that would become obsolete with these changes (#7835).

Without a solution to this problem, add_system(foo) would be unconstrained in the schedule. Engine features like events, assets, transforms, animation, etc would silently fail for users.

I don't think it's unreasonable to make a similar argument for plugins that interact with fixed updates. With these changes there will be no way to schedule around "normal gameplay" systems similar to how you can in frame updates.

jabuwu avatar Mar 17 '23 16:03 jabuwu

I wonder if there will be a need for PreUpdate and PostUpdate in fixed updates. I've created an issue related to this that would become obsolete with these changes (https://github.com/bevyengine/bevy/issues/7835).

I think its worth considering. For "abstracted out prep and response work that doesn't need to run on each run of FixedUpdate", PreUpdate and PostUpdate still fill that role. But if we have demonstrated need for "abstracted out prep and response work that needs to run on each run of FixedUpdate", we can definitely mirror the pattern there.

Probably something to be done in a separate PR though.

cart avatar Mar 17 '23 22:03 cart

Is there a reason the ScheduleLabel's aren't Copy?

context: i ran into the same question with CoreSet (i want to make a plugin where the user can freely decide when it runs by passing in the CoreSet or i guess ScheduleLable now) but it seems that already isn't relevant anymore.

laundmo avatar Mar 19 '23 19:03 laundmo