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

Back to Base Sets #16432

Open
andriyDev opened this issue Nov 19, 2024 · 7 comments
Open

Back to Base Sets #16432

andriyDev opened this issue Nov 19, 2024 · 7 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Controversial There is active debate or serious implications around merging this PR

Comments

@andriyDev
Copy link
Contributor

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.
@andriyDev andriyDev added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Nov 19, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels Nov 19, 2024
@alice-i-cecile
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

alice-i-cecile commented Nov 19, 2024

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.

@maniwani
Copy link
Contributor

maniwani commented Nov 19, 2024

(this is kind of a brain dump)

In theory, instead of Update being a schedule, it could be a system set and get basically the same behaviour.

IIUC where you're coming from, Main being split into multiple schedules creates an ergonomic issue where users can seemingly add constraints between systems in different schedules, but it's illusory. Bevy can't tell the user if those constraints are violated because the graphs are actually totally separate.

Could I paraphrase what you're proposing like this? "It should be possible to tie a system set to a schedule, such that the schedule is implied by the set and users can just cite the set alone when calling add_system."

If so, let's say scheduled systems become entities. At that point, it would become possible to analyze constraints between systems in different schedules, as long as there's something attached to the system entity indicating which schedules include it.

I say "schedules" (plural) because, with systems being entities, it would be possible to allow using one instance of a system (e.g. propagate_transforms) in multiple schedules. Same goes for system sets. After all, schedules and triggers are relatively simple abstractions built on top of retrieving and running systems manually. (edit: That said, I'm interested in arguments against letting schedules share systems.)

There used to be complaints about having to duplicate a set's configuration in multiple schedules. If systems and sets were stored in a single, shared repository (i.e. entities in the World), the need to create/configure duplicate instances disappears.

So on the surface, tying a system set to one schedule deviates from that, but I'm wondering if that's a genuine deviation or if a system set exclusively belonging to one schedule (just like parent-child) is the right framing. Either way, I think I would prefer that sets relate to schedules in a consistent manner (i.e. It feels kinda sus if some sets are exclusive and others are not).

@maniwani
Copy link
Contributor

This can be confusing since we're now passing in schedules and things that look like system sets into add_systems.

I said this on Discord too, but the full scope of a schedule is an exclusive system that runs other systems (using, at the very least, a cached list of systems, possibly augmented with dependency information).

I think this executable / "gets treated as an exclusive system" is an essential part of what a schedule is. The run_schedule function sort of acts like a generic exclusive system, so it's not wrong to treat schedules like systems even if it's confusing.

The labeling aspect is just how schedules are presented by the scheduling API. System sets are just the labeling aspect, so "a system set you can run" just turns it back into a schedule.

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

I think it'd make sense to merge sets and schedules into a single concept. A set basically gives an identity to a graph and a schedule is that + a "compiled executable" of the graph, so I see unification as making it so an "executable" can be materialized for any set.

The idea of being able to run any system set (i.e. materialize a schedule for any set on-demand) was actually present in an earlier revision of the "stageless" proposal (but it was rejected due to concerns about the proposed implementation). It implies some potentially expensive costs in the presence of a lot of sets, i.e. observers that sort the subgraphs given by these sets whenever systems are added/removed.

@maniwani
Copy link
Contributor

maniwani commented Nov 19, 2024

But yeah, getting back on the main topic, I think the suggestion is reasonable, I'd just like some more supporting arguments for why it makes sense for a system set to be optionally vs. always associated with one schedule. (I'm trying to convince myself.)

It'd also be nice to share some thoughts about what a dynamic representation of these exclusive/non-exclusive connections would be. (I assume it'd be a relationship, but like would it be child of or a new one?)

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Nov 19, 2024
@BenjaminBrienen
Copy link
Contributor

I just wanted to say... amazing job on the issue description. It's beautifully written. I like how you addressed past feedback and gave examples to show how it would look.

I like the advantages and I am interested to see where this goes!

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-Feature A new feature, making something new possible S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

4 participants