Extensible builder syntax for system insertion

[alice-i-cecile]

What problem does this solve or what need does it fill?

As we create more and more interesting ways to initialize systems, the number of AppBuilder methods we require explodes in an exponential fashion.

add_system becomes add_startup_system becomes add_startup_system_to_stage become add_startup_system_set_to_stage becomes...

What solution would you like?

Use two base methods add_system and add_system_set. Then, use the builder pattern (see EntityCommands for an example) to replicate the existing API using method chaining.

For example, add_startup_system_set_to_stage(my_stage, my_system_set) would become add_system_set(my_system_set).startup().to_stage(my_stage).

What alternative(s) have you considered?

Continue expanding our API and method names. That's what IDEs are for right? ;)

Additional context

This would help enable the subworlds RFC and similar extensions.

This is a very nice building block for "labels of mass description", hinted at in #1375. It also opens the door to principled modification of systems added by plugins (e.g. to move them into the required States).

[NathanSWard]

Yep I really like the builder pattern more, especially since it's more rust-y 🙂.
I'll being looking into this and trying out some implementations :)

[Ratysz]

add_system_set(my_system_set).startup().to_stage(my_stage) should probably be add_system_set(my_system_set.startup().to_stage(my_stage))? That way we can use existing system descriptor machinery, instead of having to make the builders stateful (likely via the typestate pattern). Our system insertion syntax is already extensible, we just haven't extended it with the outlined functionality yet - mostly because this one would be much less of a hassle after stageless.

I'm not sure what you mean by "labels of mass description", or how this is going to open the door for anything new anywhere, especially for something like tweaking how systems population is done in a given plugin. We can already modify it, but we have to open up the plugin and do the population ourselves, and this will not improve that story.

[alice-i-cecile]

That way we can use existing system descriptor machinery, instead of having to make the builders stateful (likely via the typestate pattern).

That's a compelling argument; I agree with this even if it is slightly less pretty.

I'm not sure what you mean by "labels of mass description"

This is the idea about how we could tie other information about the system, like which state or stage it runs in, to the label it's associated with.

Like usual, you're probably right that this doesn't make anything any easier though.

[NathanSWard]

Is it also worth considering dropping the requirement to call .system() whenever you try to add one?
For example, I would like the API to look like:

App::build()
    .add_system(movement_system) // no need to call .system()
    .add_system(other_system)

IMO it's much cleaner since a lot of the time the call site looks like this:

.add_system(movement_system.system())

which is just one too many systems for my liking

[alice-i-cecile]

Is it also worth considering dropping the requirement to call .system() whenever you try to add one?

We have consensus on this, but doing so is reasonably hard.

Collaborate with @DJMcNab on doing so if you want to tackle it; but split it up into two PRs :)

[NathanSWard]

add_system_set(my_system_set).startup().to_stage(my_stage)

@alice-i-cecile I'm also curious as to what we would want the API for a single system to look like.
Branching off what @Ratysz suggested, would we want something like:

add_system(
    my_system
        .system()
        .label("Hello")
        .startup()
        .to_stage(my_stage) // note `to_stage` overrides `startup`
) 

or something more like

add_system(
    SystemConfig::new(my_system.system())
        .label("Hello")
        .startup()
        .to_stage()
) 

The first one is more difficult to implement as it will require refactoring with the SystemDescriptors, but is arguably cleaner since it does not require the SystemConfig struct?

[alice-i-cecile]

I have a strong preference for the former, and expect Cart will insist on it ;)

[cart]

Yup the former is the only one of the two I'm willing to roll with. The second one requires too much typing 😄

I have an alternative implementation proposal (which does have downsides compared to the current impl). Heres a rough sketch:

trait System {
  /* same functions as before */
  fn config(&mut self) -> &mut SystemConfig;
}

trait LabelSystem {
  fn label(self, label: impl SystemLabel) -> Self
}

impl<T: System> LabelSystem for T {
  fn label(self, label: impl SystemLabel) -> Self {
    self.config().add_label(label);
    self
  }
}

Pros:

• Extensible, even from userspace (although i dont expect there to be much of a need for users to add new SystemConfig builder methods)
• Removes all of the "system coercion" traits, which feel pretty messy to me
• With some changes, might allow us to cut down on the duplicate impls we currently have for exclusive systems.
• Allows add_system() methods to actually accept system types instead of impl Into<SystemDescriptor> types
• Persists system configuration past the point of system construction, which might be nice for debugging (and editor features)

Cons:

• Forces the system to own config, even after the config has been "used". might increase memory usage
• Forces systems to own "schedule config", which muddles the "dependency tree" a bit. Systems currently exist a level "below" shcedules, which means other schedule apis could theoretically be built on top. This "mixes" the levels a bit.
• Forces System trait implementers to store the config / expose it (aka more boilerplate for each System impl)

[alice-i-cecile]

That's a neat impl @cart! I think it's better on the whole.

Forces systems to own "schedule config", which muddles the "dependency tree" a bit. Systems currently exist a level "below" shcedules, which means other schedule apis could theoretically be built on top. This "mixes" the levels a bit.

This definitely seems like the biggest drawback. @Ratysz does this look like it'll be compatible with your stageless plans?

[NathanSWard]

Forces the system to own config, even after the config has been "used". might increase memory usage

There's probably a way around this, I'll have to do some experimentation :)

@Ratysz does this look like it'll be compatible with your stageless plans?

stageless plans?

[alice-i-cecile]

stageless plans?

This is covered in rough, messy detail in #1375, which was written before we had RFCs. The basic idea is that a) stages should no longer own systems b) we should attempt to minimize the number of hard-sync points (where exclusive systems can run and commands can be processed) c) we should ensure that things like States work across hard-sync points.

[Ratysz]

Is it also worth considering dropping the requirement to call .system() whenever you try to add one?

Hopefully, eventually. There are language-level obstacles which we haven't found a good way to work around yet; it's hard to tell which of these will come first: us figuring it out, or Rust fixing the relevant bugs and oddities.

I would also recommend omitting _system from your systems' names: you know it's a system, no need to also put that in the name.

[...] API for a single system to look like.

The first one. Ease of implementation is always second to ease of use.

I have an alternative implementation proposal [...]

I can't tell from the sketch how is that actually going to work, or how would that be less messy than (admittedly) meh coercion traits. But, bundling systems' descriptors with the system itself can be done - we're holding onto most of that data anyway already.

I would also like to reiterate that all of this (and more) would be much cleaner if we stopped trying to plumb exclusive systems through the same API as actual systems. I'll be bringing this up in the stageless RFC.

@Ratysz does this look like it'll be compatible with your stageless plans?

Stageless will require some rework of what we have now, and it will require some rework of what we could have with this implementation. The "mixing of the levels" is about separation of concerns wrt descriptor data storage and is tangential.

[cart]

I would also recommend omitting _system from your systems' names: you know it's a system, no need to also put that in the name.

I'm constantly torn on this one. It feels especially redundant when my_system.system() is required. And I do like minimalism and I agree that in most cases, the system context is implied. But unlike most other types (ex: a render graph node struct implementing the GraphNode trait) there isn't much indicating that a function is a system other than the types of the system parameters (which in some cases could be ambiguous with non-systems). The _system suffix pattern encourages the author to explicitly differentiate between system and non-system.

[cart]

I can't tell from the sketch how is that actually going to work, or how would that be less messy than (admittedly) meh coercion traits.

Yeah I think its hard to tell if its a win without actually doing it. Fortunately I think thats probably a 20-30 minute project for people familiar with that part of the code. That list is pretty small, so the onus is probably on me to prove its worth doing 😄

[Nilirad]

I would also recommend omitting _system from your systems' names: you know it's a system, no need to also put that in the name.

I think namespacing systems is a better solution. You know that the module contains only systems. Any helper function should go outside the module or at least be inserted into a submodule.

EDIT: i actually would love a #[system] attribute, maybe even #[resource] or #[component] ones if that's possible and doesn't have major drawbacks. I'd like to know what the community thinks about it.

[alice-i-cecile]

EDIT: i actually would love a #[system] attribute, maybe even #[resource] or #[component] ones if that's possible and doesn't have major drawbacks. I'd like to know what the community thinks about it.

For related discussion on this point, see #1843 :)

[Ratysz]

i actually would love a #[system] attribute, maybe even #[resource] or #[component] ones if that's possible and doesn't have major drawbacks. I'd like to know what the community thinks about it.

The drawbacks are all those of procedural macros. Compilation times, code clarity, ease of troubleshooting, cognitive load.

I don't see any immediate benefits for our case.

For related discussion on this point, see #1843 :)

I don't think that's related. I think this is about Legion-like system declaration macro.

[NathanSWard]

I would also like to reiterate that all of this (and more) would be much cleaner if we stopped trying to plumb exclusive systems through the same API as actual systems.

@Ratysz would the recommendation be then that we have explicit add_system and add_exclusive_system functions when adding systems?

[Ratysz]

Not sure what you mean by "recommendation"; I don't know what exact form of this I'll settle on for the RFC.

Currently leaning towards building the schedule from distinct and strictly separate parallel and sequential blocks of systems, with exclusive systems only being able to be used in the latter. This is, essentially, same as stages (we just can't avoid some form of them if we want to support parallel->exclusive->parallel case) graph-wise, just flatter and with less seams API-wise.

[Ratysz]

From #2510 (comment) (@Nilirad)

Is there a way to avoid hardcoding references to the startup schedule in the API? While it is added by default by App::build, it is still possible to design an app schedule from scratch.

The API from the snippet would still require hardcoding: something needs to know that stages labelled with StartupStage are nested in some other stage. We might want to either invent a way to define that from the user side, or figure out a generalized "nested insertion" API. (Or not nest things, but that's a topic for another time.)

[alice-i-cecile]

From #2510, @Nilirad comments:

Is there a way to avoid hardcoding references to the startup schedule in the API? While it is added by default by App::build, it is still possible to design an app schedule from scratch.

The API would look like this, directly referencing the stage by label...

add_system_set(
    my_system_set
        .in_stage(StartupStage::PreStartup)
)

So, I mostly agree with this. Inserting into a specific stage should cause startup-like behavior if it's a startup stage: no need for another method.

However I'd like to keep .startup() around as a syntactic shortcut for the common case.

Of course, with the stageless schedule rework we may need a "run this system only once at the beginning" signifier, or it may just get rolled into the States abstraction.

[Nilirad]

We might want to either invent a way to define that from the user side, or figure out a generalized "nested insertion" API.

I think a nested insertion API would be a good solution: the user just specifies the stage label and then the schedule does the job of finding the stage. It may look as simple as changing Schedule::get_stage and Schedule::get_stage_mut to do the nested search, but the stages field of Schedule is a HashMap, which is flat and cannot represent tree structures. Maybe we can use some kind of recursion to call get_stage or get_stage_mut on each stage, but we cannot assume that each element of stages is a Schedule...

[thechubbypanda]

As discussed on discord, I'm gonna look at this in the coming days, if you have any suggestions, feel free to ping me :)

(I'll reference this in a pr when I get a chance so check below)

[TheNeikos]

Use two base methods add_system and add_system_set. Then, use the builder pattern (see EntityCommands for an example) to replicate the existing API using method chaining.

For example, add_startup_system_set_to_stage(my_stage, my_system_set) would become add_system_set(my_system_set).startup().to_stage(my_stage).

I would suggest chaining on the common denominator that one usually groups things around, which is stages! I would feel that having app.stage(CoreStage::Update).add(update_player).add(update_score) is easier to use. In the same way, one could then 'override' the current stage by doing .app.stage(CoreStage::Update).<add more>.stage(CoreStage::PostUpdate).add(synchronize).

This also makes it easier to add systems to stages, as rustfmt would put each of these on a seperate line, so all you'd have to do is insert it below a .stage call.

[IceSentry]

That would be nice with the current stage based API, but since there are discussions of making the scheduler stageless I'm not sure it's the right move to tie everything to stages

[Ratysz]

This isn't as much "tying everything to stages" as it is making the app/schedule builder stateful, which is a bad idea more often than it's not.

[IceSentry]

Yeah, I'm also not a fan of the stateful part, but I was mostly commenting about the stages being the common denominator part.

[thechubbypanda]

Am I wrong in thinking that a lot of the comments are out of date as we now have a way to do:

App::new().add_system(some_system);

rather than

App::new().add_system(some.system());

@cart's implementation idea:

trait System {
  /* same functions as before */
  fn config(&mut self) -> &mut SystemConfig;
}

trait LabelSystem {
  fn label(self, label: impl SystemLabel) -> Self
}

impl<T: System> LabelSystem for T {
  fn label(self, label: impl SystemLabel) -> Self {
    self.config().add_label(label);
    self
  }
}

is no longer a thing since we're no longer using Into<SystemDescriptor>, rather IntoSystemDescriptor<Params> (which I understand very little of XD
Does anyone have any opinions on how to port it to the new system?

[Ratysz]

Cart's idea replaces descriptors entirely, the data and API that belonged to the descritor now belong to the system. add_system() will need to take impl IntoSystem directly; you'll also need a dedicated add_exclusive_system() that takes impl IntoExclusiveSystem - not merging these two will simplify your implementation and will let us yeet .exclusive_system(), just like we did with .system().

I'm not yet fully certain reworking system description like this is the way we should take; it seems like it would be pretty neat, but last time I examined it I fould some counterpoints that I cannot recall right now.

[james7132]

Following up on this, it does seem like the new APIs exposed for stageless mirror some of the goals here. Is this issue still relevant?

[Nilirad]

Probably not anymore, since bevyengine/rfcs#31 has been closed.