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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Things I love
Things I regret but don't see a better way around
Things I think should be changed
Things I'm conflicted on
|
alice-i-cecile
approved these changes
Mar 18, 2023
Shfty
pushed a commit
to Shfty/bevy
that referenced
this pull request
Mar 19, 2023
Co-authored-by: Mike <mike.hsu@gmail.com>
Shfty
pushed a commit
to Shfty/bevy
that referenced
this pull request
Mar 19, 2023
Co-authored-by: Mike <mike.hsu@gmail.com>
|
Is there a reason the 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. |
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
Breaking changes were introduced in bevyengine/bevy#8079
iwek7
added a commit
to iwek7/bevy_prototype_lyon
that referenced
this pull request
Mar 22, 2023
Breaking changes were introduced in bevyengine/bevy#8079
Nilirad
pushed a commit
to Nilirad/bevy_prototype_lyon
that referenced
this pull request
Mar 23, 2023
Breaking changes were introduced in bevyengine/bevy#8079
Elabajaba
added a commit
to Elabajaba/bevy
that referenced
this pull request
Mar 29, 2023
josekoalas
pushed a commit
to josekoalas/bevy
that referenced
this pull request
Apr 13, 2023
Co-authored-by: Mike <mike.hsu@gmail.com>
This was referenced Apr 13, 2023
This was referenced Apr 20, 2023
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-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!
S-Controversial
This requires a heightened standard of review before it can be merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
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:
add_system(foo.after(TransformPropagate))would fail because foo is implicitly inCoreSet::Updateby default, but transform propagation happens inCoreSet::PostUpdate.add_system(foo)to be automatically added toCoreSet::Update, ensuring thatfoocan 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 scheduleadd_systems((foo, bar)): add multiple systems to the default scheduleadd_system(foo.in_schedule(CoreSchedule::Startup)): add one system to a specified scheduleadd_systems((foo, bar).in_schedule(CoreSchedule::Startup)): add multiple systems to a specified scheduleadd_startup_system(foo): add a system to the startup scheduleadd_startup_systems((foo, bar)): add multiple systems to the startup scheduleadd_system(foo.on_startup()): add one systems to the startup scheduleadd_systems((foo, bar).on_startup()): add multiple systems to the startup scheduleNote 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)andadd_system(foo.on_startup())on_enterandon_exithave 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::Startupis a long name. People don't want to type it or look at it.A system's
Schedule(and purpose) should be clearMainschedule, but users are often not aware of this. This is "implied context". And that context changes across apps. Confusing!Updatefor theMainschedule, but users are often not aware of this. This is "implied context". And that context changes across schedules. Confusing!add_startup_system, andfoo.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)). Theon_x()pattern is especially hazardous, as users could easily think "I want to run this system both on_enter and on_exit". Butfoo.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 everythingThere 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:
You must type
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, andon_startup()have been deprecated in favor of the newadd_systemsNested System Tuples and Chaining
It is now possible to infinitely nest tuples in a single
.add_systemscall: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:
This is especially powerful because
chain()can now be nested!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:
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:
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_setnow accepts a scheduleThis creates parity with the new
add_systemsAPI and resolves the common problem of forgetting that set configuration only applies to specific schedules:The new
MainScheduleCoreSchedule::Outerschedule. It has been merged into the newMainschedule. (andApp::outer_schedule_labelconcept has been renamed toApp:main_schedule_label)CoreSetbase set has been removed in favor of nice and short:First,PreUpdate,Update,PostUpdate,StateTransition, andLastschedules. No need for "flush" variants because schedules flush automaticallyCoreScheduleenum has been broken up intoStartup,Main,FixedUpdate,PreStartup, andPostStartupschedules (joining the new schedules from the previous point) and theStartupSetbase set has been removed.FixedUpdateLoopschedule has been added to facilitate running theFixedUpdateloopMainScheduleOrderresource has been added, which defines the schedules theMainschedule 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).RenderscheduleThe prepare, queue, render, etc sets now live in the new
Renderschedule inRenderApp.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
in_scheduleandon_startupto ease migration. These can panic with a migration notice (as we're doing a breaking change anyway).Next Steps
add_systems(Update, foo.run_if(in_state(MyState::A))).State API Ergonomics / Consistency: We should consider removing the OnX prefix from
OnEnterandOnExitfor shorter names and parity with the Startup "event-style" schedule. We should also consider renaming thestate_equalsrun condition toin_statefor less typing and better readability (foo.run_if(in_state(X))vsfoo.run_if(state_equals(X))).Mainschedule a bit more than before. We should ensure https://github.com/jakobhellermann/bevy_mod_debugdump still produces useful outputs.Transformcomponents to automatically add abefore(TransformPropagation)dependency. Or a system that uses Events to add anafter(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!ExtractScheduleto something likeExtractionto better match the other shorter "built in schedule" naming conventions. Sadly it can't beExtractbecause we already use that for the extract system param.&label:&*labelis 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_systemsnow accepts aScheduleLabelas the first parameter.app.add_system,app.add_startup_system,app.add_startup_systems,system.on_startup(), andsystem.in_schedule()have been deprecated in favor of the unifiedapp.add_systemsAPI."base sets" have been removed entirely in favor of Schedules. The built in
CoreSetandStartupSetbase sets have been replaced with top level schedules. (ex:CoreSet::Updateis now theUpdateschedule).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_systemsAPI speaks for itself:Set configuration now also accepts a schedule:
Changelog
Added
add_systemscan now be nested (and the nested tuples can be chained)RunFixedUpdateLoopschedule has been added to coordinate the runs ofFixedUpdateinMainMainScheduleOrderwas added to enable configuring the schedules run inMain(and their order)Changed
add_systemsnow accepts aScheduleLabelas the first parameteradd_systemscan now accept a single systemCoreSetandStartupSetbase sets have been replaced with top level schedules. Ex:CoreSet::Updateis nowUpdateMainschedule has been merged into the old "outer" schedule.Mainnow runs all of the top level schedules directlyRenderSetsets now live in the newRenderscheduleRemoved
Appapp.add_system(),app.add_startup_system(),app.add_startup_systems(),system.on_startup(), andsystem.in_schedule()have been deprecated in favor ofapp.add_systemsApp::outer_schedule_labelhas been removed