[Trashtalk217]

Objective

SystemSet is a collection of systems and other system sets. Schedule is also a collection of systems and system sets.
We could simplify Schedules by only storing very few schedules and make schedules like First, Update and Last into system sets. One benefit is that Schedules can currently never run concurrently, even if none of the systems conflict. Putting everything in one big schedule with system sets can potentially increase parallelism.

Solution

Before this PR, scheduling worked as follows: When an App was first created, the MainSchedulePlugin was initialized. This adds three schedules to the App, one of them being Main. Next, the system Main::run_main is added to Main. Whenever this system is called, they look up the MainScheduleOrder resource in the main world and run the schedules specified within. These are the schedules we're all familiar with: First, Update, PostUpdate, etc. If a user ever needs to reorder this set of schedules or add their own, methods are provided for on the MainScheduleOrder resource.

In this PR the schedule we add first is called EntryPoint, which has the system EntryPoint::run_main. Instead of running the First, Update and PostUpdate schedules, this only runs one schedule, namely Main. This schedule contains First, Update, PostUpdate, etc, as system sets, ordered by simply chain()ing them together with app.configure_sets.

In summary. The following are still schedules*: StartupMain, FixedMain, Main, EntryPoint, SpawnScene.

While the following have become system sets: PreStartup, StartupMain, PostStartup, First, PreUpdate, Update, PostUpdate, Last, RunFixedMainLoop, FixedFirst, FixedPreUpdate, FixedUpdate, FixedPostUpdate, FixedLast, and RemoteLast.

• There are other schedules, which mostly have to do with extraction, rendering, or state transitions. Those are out of scope for this PR. I expect extraction to stay a schedule, rendering to transition to sets pretty easily, and I will have to double-check if the state transition schedules still work within this PR.

Testing

Already existing tests.

AI Disclosure

I did not use AI in anyway, but full credit goes to @andriyDev, who I'm adopting this PR from.

[alice-i-cecile]

This needs a release note explaining the implications and strategy at a high level please <3 I have wanted something like this for years but we need to be careful not to regress the problems that drove us to separate these in the first place.

[hymm]

This is a quick skim of the code with some random comments. Mostly just focusing on the approach/public api.

Generally onboard with the approach here. I think to get this merged we'll want to do some benchmarking with the stress tests and do some before and after comparisons with bevy_mod_debug_dump to sanity check the changes.

[hymm]

shouldn't there be .in_set(PostUpdate) somewhere here? Same with a lot of the other configures_sets calls.

I feel like configure_sets should probably just take a impl SystemLocation for ease of migration.

[Trashtalk217]

Good find, I've changed it.

I don't know if using impl SystemLocation for configure_sets is a good idea because you run into the edge case where it's not clear what should happen if you have two system sets with different default schedules.

[hymm]

So if you have SetA with a default schedule of ScheduleA and a SetB with a default schedule of ScheduleB and then you do configure_set(SetA, SetB). In this case I'd say you're adding SetB into SetA in ScheduleA. It makes sense to me that the first term is the defacto schedule. If SetA had no default schedule it could be a warning and do nothing. Ideally it'd be a compile error, but not sure that's possible.

With being able to make it a compiler error I'm not sure we should do it. Adding a panic to configure_sets feels pretty bad. Adding a warning feels like it'd be easy to miss. But maybe a warning would be enough? I'm not sure.

The reason I suggested this is that if you scroll down in the review a little it doesn't take long to find 5-6 more instances where we didn't add in in_set(PostUpdate). If we as the devs are doing this then users migrating are going to have this issue too.

[Trashtalk217]

I don't like that approach because then configure_set seizes to have a good name. configure_set(Main, SetA.after(SetB)) is very clearly about configuring SetA and SetB to make it so that SetB is after SetA, all within the context of the Main schedule. If you instead allow configure_set(SetA, SetB) it's no longer clear to me how we're configuring SetB. It feels like configure_set becomes a function that does two seperate things.

[urben1680]

I did not follow the whole design idea here but I was hoping also the base schedules will be sets and configure_sets will have no first argument anymore.

[Trashtalk217]

I've gone through the entire PR and added .in_set() where it was necessary. Since you have to specify the system set when adding systems add_systems(PostUpdate, ...) it doesn't cause any problems in practice.

Also @urben1680, as much as I would like to report that we did indeed got rid of all schedules, that sadly isn't possible. (Or at least not feasible in this PR) That's much closer to a 'systems-as-entities' type goal.

[urben1680]

I figured, nevertheless I do not want to undervalue your effort here!

[JMS55]

Ooh is this going to let us use weak chaining between main system sets? From #25128:

Note that this won't solve overlap between schedules. This means that the main world (Update/PostUpdate/etc) can't overlap, and same for Render -> RenderGraph. TBD how we solve that.

I'm not seeing any changes to RenderGraph in this PR though.

[andriyDev]

@JMS55 yes, you could in principle use weak ordering between the main system sets. I'm personally not a fan of that though because it could make it pretty easy to end up with "floating" systems if the system doesn't conflict with anything. I'm still anti-"after means weak, after_strict means strict" since it breaks the very obvious intuition that systems that you say come after actually don't come after.

The other thing to consider is whether we'll take advantage of this very much for scheduling. e.g., should we start adding systems to Main and allow them to float (and users have to explicitly order their systems against some system set)? Like does the asset server tracking system need to be in PostUpdate? Or should it just be floating, and people can order their systems relative to its system set. I don't know! So we'll see how much things change at all.