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)