-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Description
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 fromin_set
.
- Agreed! However I primarily believe this was in the nomenclature. To change a system's base set, you had to call
- "It was easy to accidentally create invalid orderings:
add_system(foo.after(TransformPropagate))
would fail because foo is implicitly inCoreSet::Update
"- This is a problem because
Update
is nowhere to be seen, and no way to really "reach" it with "Go to Definition".
- This is a problem because
- "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
implementSystemLocation
. Then ouradd_systems
calls would look likeapp.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 betweenUpdate
,CoreSet::Update
, andMain
. (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 itsSystemLocation
impl is close), see that it belongs to theMain
schedule.