Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schedule-First: the new and improved add_systems #8079

Merged
merged 41 commits into from
Mar 18, 2023

Conversation

cart
Copy link
Member

@cart cart commented Mar 14, 2023

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.

Migration Guide

We have unified adding systems to schedules under a single API! add_systems now accepts a ScheduleLabel as the first parameter. app.add_system, app.add_startup_system, app.add_startup_systems, system.on_startup(), and system.in_schedule() have been deprecated in favor of the unified app.add_systems API.

"base sets" have been removed entirely in favor of Schedules. The built in CoreSet and StartupSet base sets have been replaced with top level schedules. (ex: CoreSet::Update is now the Update schedule).

This removes a ton of redundant APIs, removes implicit defaults entirely, and clears up a lot of the confusion introduced by base sets. We believe the consistency and ergonomics of the new add_systems API speaks for itself:

// Before
app.add_system(a)
// After
app.add_systems(Update, a)

// Before
app.add_systems((a, b).in_schedule(CoreSchedule::Startup))
// After
app.add_systems(Startup, (a, b))

// Before
app.add_systems((a, b).in_schedule(CoreSchedule::Startup).in_base_set(StartupSet::PreStartup))
// After
app.add_systems(PreStartup, (a, b))

// Before
app.add_startup_systems((a, b))
// After
app.add_systems(Startup, (a, b))

// Before
app.add_systems((a, b).on_startup())
// After
app.add_systems(Startup, (a, b))

// Before
app.add_systems((c, d, e))
// After (Update is no longer implied by default)
app.add_systems(Update, (c, d, e))

// Before
app.add_systems((f, g).in_schedule(CoreSchedule::FixedUpdate))
// After
app.add_systems(FixedUpdate, (f, g))

// Before
app.add_systems(h.in_base_set(CoreSet::PostUpdate))
// After
app.add_systems(PostUpdate, h)

// Before
app.add_systems(enter_menu.in_schedule(OnEnter(AppState::Menu)))
// After
app.add_systems(OnEnter(AppState::Menu), enter_menu)
    
// Before
app.add_systems(exit_menu.in_schedule(OnExit(AppState::Menu)))
// After
app.add_systems(OnExit(AppState::Menu), exit_menu)

// Before
render_app.add_systems((a, b).in_set(RenderSet::Queue))
// After
render_app.add_systems(Render, (a, b).in_set(RenderSet::Queue))

Set configuration now also accepts a schedule:

// Before
app.configure_set(A.in_schedule(PostUpdate).after(B))
// After
app.configure_set(PostUpdate, A.after(B))

// Before
app.configure_set(A.after(B))
// After (Update is no longer implied by default)
app.configure_set(Update, A.after(B))

// Before
app.configure_sets((A, B).in_schedule(PostUpdate).after(C))
// After
app.configure_sets(PostUpdate, (A, B).after(C))

// Before
app.configure_sets((A, B).after(C))
// After (Update is no longer implied by default)
app.configure_sets(Update, (A, B).after(C))

Changelog

Added

  • Tuples of systems in add_systems can now be nested (and the nested tuples can be chained)
  • The RunFixedUpdateLoop schedule has been added to coordinate the runs of FixedUpdate in Main
  • MainScheduleOrder was added to enable configuring the schedules run in Main (and their order)

Changed

  • add_systems now accepts a ScheduleLabel as the first parameter
  • add_systems can now accept a single system
  • CoreSet and StartupSet base sets have been replaced with top level schedules. Ex: CoreSet::Update is now Update
  • The Main schedule has been merged into the old "outer" schedule. Main now runs all of the top level schedules directly
  • The RenderSet sets now live in the new Render schedule

Removed

  • "base sets" have been removed in favor of schedules
  • The "default schedule" concept has been removed from App
  • app.add_system(), app.add_startup_system(), app.add_startup_systems(), system.on_startup(), and system.in_schedule() have been deprecated in favor of app.add_systems
  • App::outer_schedule_label has been removed

@alice-i-cecile alice-i-cecile requested review from alice-i-cecile and JoJoJet and removed request for alice-i-cecile March 14, 2023 01:28
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR labels Mar 14, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 14, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 14, 2023

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

Elabajaba added a commit to Elabajaba/bevy that referenced this pull request Mar 22, 2023
iwek7 added a commit to iwek7/bevy_prototype_lyon that referenced this pull request Mar 22, 2023
iwek7 added a commit to iwek7/bevy_prototype_lyon that referenced this pull request Mar 22, 2023
Nilirad pushed a commit to Nilirad/bevy_prototype_lyon that referenced this pull request Mar 23, 2023
Elabajaba added a commit to Elabajaba/bevy that referenced this pull request Mar 29, 2023
@IceSentry IceSentry added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 8, 2023
Nilirad added a commit to Nilirad/bevy_prototype_lyon that referenced this pull request Jul 10, 2023
* Update code after changes of systems api in bevy (#208)

Breaking changes were introduced in
bevyengine/bevy#8079

* Use GitHub Actions config from master branch (#217)

* Use GitHub Actions config from master branch

* Fix formatting

* Fix deprecation warnings on add_plugin

* Fix CI (#218)

* Fix shader (#219)

* Depend on Bevy 0.11

* Bump version to v0.9.0

* Use cargo add instruction in README

* Update README

* Update CHANGELOG

---------

Co-authored-by: Michał Iwańczuk <miwanczuk7@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2023
# Objective

Remove some references to `CoreSet` which was removed in #8079.
github-merge-queue bot pushed a commit that referenced this pull request Dec 9, 2023
Simple doc change since there is no more a default schedule after #8079.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Remove some references to `CoreSet` which was removed in bevyengine#8079.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants