Back to Base Sets

[andriyDev]

Background

Today, we have many schedules. Main, First, PreUpdate, Update, etc. Primarily, these are used for general ordering of systems. This has necessitated a bunch of sad parts like MainScheduleOrder. This is just code duplication. We already have a way to order a bunch of systems: system sets! In theory, instead of Update being a schedule, it could be a system set and get basically the same behaviour. In theory we could even have systems that run between PreUpdate and Update (without needing a schedule in between). Schedules reduce parallelism and system set constraints aren't (currently) shared across schedules.

Base Sets

In #8079, we removed base sets. I believe this was the correct move at the time. The main problems listed in that PR:

• "It is a new concept that users need to content with"
  • Agreed! However I primarily believe this was in the nomenclature. To change a system's base set, you had to call in_base_set which was different from in_set.
• "It was easy to accidentally create invalid orderings: add_system(foo.after(TransformPropagate)) would fail because foo is implicitly in CoreSet::Update"
  • This is a problem because Update is nowhere to be seen, and no way to really "reach" it with "Go to Definition".
• "They were largely added to enable 'default base sets'"
  • I would argue this is the root of the problem.

I am not trying to bring back base sets as they were. However, they have a nice advantage: there's pretty much one schedule to deal with: Main.

Bringing them back in style

The current syntax for adding systems is very nice:

app.add_systems(Update, my_system);

Let's keep that! I propose adding a trait like:

pub trait SystemLocation {
    fn get_schedule_set_pair(&self) -> (Box<dyn ScheduleLabel>, Option<Box<dyn SystemSet>>);
}

impl<T: ScheduleLabel> SystemLocation for T {
    fn get_schedule_set_pair(&self) -> (Box<dyn ScheduleLabel>, Option<Box<dyn SystemSet>>) {
        (self.dyn_clone(), None)
    }
}

Then add_systems becomes:

pub fn add_systems<M>(
    &mut self,
    system_location: impl SystemLocation,
    systems: impl IntoSystemConfigs<M>
) -> &mut App {
    // ...
}

So far, nothing has changed: we can still add systems to a schedule like normal. We can also still add systems to system sets like normal:

render_app.add_systems(Render, queue_donkey.in_set(RenderSet::Queue));

The kicker comes from the next step. First, we need to bring back the CoreSet enum (part of it at least):

#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)]
pub enum CoreSet {
    First,
    PreUpdate,
    RunFixedMainLoop, // We can probably remove this one.
    Update,
    SpawnScene,
    PostUpdate,
    Last,
}

And finally, we change Update and friends, to the following:

pub struct Update;

impl SystemLocation for Update {
    fn get_schedule_set_pair(&self) -> (Box<dyn ScheduleLabel>, Option<Box<dyn SystemSet>>) {
        (Box::new(Main), Some(Box::new(CoreSet::Update)))
    }
}

Now look what this does:

app.add_systems(Update, move_donkey.in_set(MovementSystemSet));

Wait... nothing changed... Exactly! Under the hood, Update is no longer a schedule. It is a system set in the Main schedule. Provided that Bevy adds a .chain() for all the CoreSet, #9822 will automatically insert a sync point between the "base sets". Now we've just made some syntactic sugar for:

app.add_systems(Main, move_donkey.in_set(MovementSystemSet).in_set(CoreSet::Update));

User's don't need to learn anything about base sets, they just silently use them. There's also nothing special about base sets. Users can create their own "base sets" by just making a new struct that impls SystemLocation.

app.add_systems(avian2d::PhysicsSet::Prepare, explode);

Even rendering can take advantage of this. As far as I can tell, rendering uses one schedule with a bunch of system sets for performance reasons. Now we can have a nicer API:

render_app.add_systems(Queue, queue_donkey);

MainScheduleOrder can be replaced with just configure_sets calls:

#[derive(SystemSets)]
struct PrePreUpdate;
app.configure_sets(Main, (First, PrePreUpdate, PreUpdate).chain());

Alternatives

• We could make CoreSet implement SystemLocation. Then our add_systems calls would look like app.add_systems(CoreSet::Update, move_donkey).
• We could make system sets always be associated with one schedule. Then we can just allow system sets to be used in add_systems. (I'm not a fan of this since it reduces what we can do with system sets).
• We don't strictly need this. It's mostly cleaning up and "simplifying" things (simplifying in the sense that there are fewer moving parts).

Risks

• This can be confusing since we're now passing in schedules and things that look like system sets into add_systems. This can be confusing to learn about the difference between Update, CoreSet::Update, and Main. (it may be confusing that you can add systems to a system set in two ways). I think this is fine since this is mostly just an implementation detail. From a user's perspective they are doing the same thing that schedules do today.
• The schedule is now implicit. I think this is fine since there's still something to "Go to Definition" on. It's fairly easy to go to the definition of Update and (assuming its SystemLocation impl is close), see that it belongs to the Main schedule.

[alice-i-cecile]

I generally agree with your criticisms of the state of thing here, but I would like to fully remove schedules in a refactor like this, and explicitly pass in a base set in place of the ScheduleLabel argument to add_systems(Update, my_system). There's too much conceptual overlap.

[andriyDev]

I generally agree with your criticisms of the state of thing here, but I would like to fully remove schedules in a refactor like this, and explicitly pass in a base set in place of the ScheduleLabel argument to add_systems(Update, my_system). There's too much conceptual overlap.

I don't think this is feasible. Schedules can't go away really. The most obvious example is FixedUpdate. This has to be a schedule since it needs to run multiple times per frame. We can't achieve this with system sets directly. I'd also want to be able to keep saying app.add_systems(FixedUpdate, movement), so keeping the Schedule-first API seems good.

We could make schedules lighter weight, especially if we switch to systems as entities. Then a schedule just becomes a system set to run and a cached topo sort. But that's a future ask, and I'm not sure if we need to wait on this. Note this issue requires pretty much no migration guide.

[alice-i-cecile]

There definitely needs to be a world.run_group_of_systems API due to the branching control flow :)

We could make schedules lighter weight, especially if we switch to systems as entities. Then a schedule just becomes a system set to run and a cached topo sort

This was exactly the sort of thing I had in mind. I don't think we need to wait on systems-as-entities for this, but it sure would be nice to be able to quickly filter by label like that.