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

Plugin configurability: apps own scheduling logic #33

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Sep 15, 2021

RENDERED

Many thanks to @Ratysz, @TheRawMeatball and @cart for helping shape my thoughts on this topic.

Summary

All scheduling information is attached directly to systems, and ultimately owned and configurable by the central app (or hand-written bevy_ecs equivalent).
To ensure that this can be done safely, plugins specify invariants about the ordering of their systems which must be respected.
Hard ordering constraints (where systems must be both ordered and separated by a hard sync point) are introduced as an assert-style protection to guard against a serious and common form of plugin misconfiguration.

Motivation

The current plugin model works well in simple cases, but completely breaks down when the central consumer (typically, an App) needs to configure when and if their systems run. The main difficulties are:

  • 3rd party plugins can have "unfixable" ordering ambiguities or interop issues with other 3rd party plugins
  • plugins can not be added to states, and run criteria cannot be added
  • plugins can not be incorporated into apps and games that use a non-standard control flow architecture
  • end users who care about granular scheduling control for performance optimization have no way to change when systems run
  • worse, plugins can freely insert their own stages, causing sudden and uncontrollable bottlenecks
  • plugins cannot be used by standalone bevy_ecs users

See Bevy #2160 for more background on this problem.

By moving the ultimate responsibility for scheduling configuration to the central app, users can comfortably combine plugins that were not originally designed to work together correctly and fit the logic into their particular control flow.

" Imo, it's fine to saddle plugin authors with boilerplate if it makes it easier for the users downstream" - Ratys
@hymm
Copy link

hymm commented Sep 15, 2021

could we get some code for how configuring a plugin would look?

@froggyC
Copy link

froggyC commented Sep 15, 2021

I think going through some use cases with related dependency diagrams would definitely help make this RFC more clear.

I have a question: If a plugin is able to declare a system set, and that system set is able to declare the stage it belongs to, doesn't that defeat the purpose of these changes?

Ideally, the plugin should not be aware of what stages of the app are available, as I would consider that a user-side decision.

@HackerFoo
Copy link

Here's my use case: It's useful to selectively disable plugins for power saving or when capabilities are disabled in a mobile app. For example, I have to do this in an iOS app:

    for label in [RenderStage::RenderGraphSystems,
                  RenderStage::Draw,
                  RenderStage::Render,
                  RenderStage::PostRender]
    {
        if let Some(mut stage) = app.schedule.get_stage_mut::<SystemStage>(&label) {
            stage.set_run_criteria(is_not_idle);
        }
    }

which works, but could be better. Without this, the OS will kill the app if it is put in the background.

Furthermore, it would be nice to have plugins for different OSes that handle this kind of thing, so that users aren't required to handle OS-specific schedule changes.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Sep 15, 2021

I have a question: If a plugin is able to declare a system set, and that system set is able to declare the stage it belongs to, doesn't that defeat the purpose of these changes?

Ideally, the plugin should not be aware of what stages of the app are available, as I would consider that a user-side decision.

@hymm: So, I very much see your point: my perspective is that the stages (and high-level control flow in general) of an app are a user-side decision that plugins have no business knowing about.

However, in earlier discussions @cart made clear that we should be careful not to impact the beginner's experience, as having to grapple with scheduling immediately on starting to make their first Bevy game would be quite overwhelming and frustrating. Basically, adding your first plugins (especially default plugins!) should not need new users to specify which stage they're inserted into.

That said, now that I look back on the design, I think we could get away with removing the ability for plugins to dictate which stage systems run in. Basically, we could do some very minor smart scheduling, by letting apps specify a default stage for systems to be inserted into, then using hard ordering dependencies to insert the system sets to the schedule correctly.

I'll make a commit to change that in the morning, which IMO makes the design way cleaner.

rfcs/33-apps_own_scheduling.md Show resolved Hide resolved
rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
1. Plugins are no longer quite as powerful as they were. This is both good and bad: it reduces their expressivity, but reduces the risk that they mysteriously break your app.
2. Apps can directly manipulate the system ordering of their plugins. This is essential for ensuring that interoperability and configurability issues are resolved, but risks breaking internal guarantees.
3. Users can accidentally break the requirement for a hard sync point to pass between systems within a plugin as encouraged by the plugin configuration by moving system sets into the same stage. Hard ordering dependencies briefly discussed in the *Future Work* section of this RFC could ensure that this is impossible.
4. Replacing existing resource values is more verbose, requiring either a startup system or explicitly removing the resource from the pre-existing plugin. This is much more explicit, but could slow down some common use cases such as setting window size.
Copy link
Member

@cart cart Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this RFC to explicitly address how Plugins will handle "extending" existing resources. Ex:

  1. Calling register_type::<T>() to add T to the TypeRegistry resource
  2. Calling init_asset_loader::<T>() to add a new asset loader to the AssetServer resource

This requires solving the "order of operations" problem. When will Plugins be initialized? How will we ensure that TypeRegistry exists before register_type is called?

I can think of two options:

Plugins have dependencies

Plugins have dependencies, either by implicitly defining them via something like registration order or explicitly defining them via something like labels (or Plugins providing a list of TypeId dependencies). This is basically accepting that Plugins can have arbitrary side effects. At this point, we might as well give direct access to World. Plugins have more freedom to interact with the app and implementing things like register_type is both simple and efficient (no need to schedule any extra systems, define dependencies between them, etc).

In some ways, this is what we have today. But we could still constrain things as defined in this RFC / add the additional system set configurability. And we could still have "deferred plugin init" if theres a good reason for it.

Plugins do not have dependencies

Plugins are "just" order-independent descriptions of systems to add. Only Systems can have orders. Some initialization things depend on specific resources, so resources must be added via Systems. Something like plugin.init_asset_loader::<T>() would internally expand to something like:

fn init_resource<T: Resource + FromWorld>(mut self) -> Plugin {
  self.add_system(|world: &mut World| {
    let resource = T::from_world(world);
    world.insert_resource(resource);
  }.label(ResInit::<T>));
}

fn init_asset_loader<T: AssetLoader + FromWorld>(mut self) -> Plugin {
  self.add_system(|world: &mut World| {
    let loader = T::from_world(world);
    let asset_server = world.get_resource::<AssetServer>().unwrap();
    asset_server.add_asset_loader(loader);
  }.after(ResInit::<AssetServer>));
}

Some things to note:

  1. These need to be exclusive systems due to their FromWorld constraints.
  2. This only works for "deferred value initialization". We can't support something like plugin.add_resource(MyResource { foo, bar }) or plugin.add_asset_loader(MyLoader::new()).

Lets take a look at why (2) isn't possible:

fn add_asset_loader<T: AssetLoader>(mut self, loader: T) -> Plugin {
  self.add_system(|asset_server: Res<AssetServer>| {
    asset_server.add_asset_loader(loader);
  }.after(ResInit::<AssetServer>));
}

First, this would have the benefit of being able to schedule init stuff in parallel. But honestly this type of resource init is generally cheap. I doubt that the parallelism wins will outweigh the overhead of System scheduling.

However we can't do this right now: we would need a RunOnce system equivalent to accomplish this, which would be a pretty significant change, both to the System trait and the Scheduler implementation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have some kind of plugin resource (name bikeshedding necessary) concept. Plugins would then specify some data and the plugin resource will be called when plugins are added and will handle this data appropriately? For example in case of the TypeRegistry plugin resource a plugin can provide type registrations, while for the AssetServer plugin resource a plugin can provide asset loaders. This would possibly also allow handling removal of plugins using an extra callback in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully follow this. Could you walk through the "order of operations" of how the AssetPlugin registers the AssetServer and some CustomPlugin registers a custom AssetLoader? How would the AssetLoader be represented in the CustomPlugin? When / how would it be registered with the AssetServer resource?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssetPlugin would register AssetServer as "plugin resource" using something like self.add_plugin_resource(AssetServer::default()). CustomPlugin schedules an add event for CustomAssetLoader for the AssetServer "plugin resource" using something like self.plugin_resource_add_part::<AssetServer>(CustomAssetLoader::default()). The order between the these two registrations doesn't matter. The AssetServer could then either be notified about the add of CustomAssetLoader after all plugins are registered or as soon as both AssetServer and CustomAssetLoader are registered. It doesn't really matter which one is chosen. Notably in either case if the AssetServer is not yet registered by the time CustomPlugin tries to register CustomAssetLoader, the actual registration is delayed until after AssetServer is registered. AssetServer has to implement a trait with a method that is called every time something is registered with it.

Copy link
Member

@cart cart Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that seems like it would work. This seems similar to the "Plugins don't have dependencies" solution (due to the need to ensure that AssetServer is registered prior to applying "resource parts"), but with more complicated userspace (due to the addition of "resource parts" as a concept). It means that in addition to the "normal" apis exposed on a resource, developers also need to think about the "resource plugin api" and expose the relevant bits. Feels like a bit too much complexity. If we can detect the resource_part -> resource dependency, then we can just as easily provide the custom asset loader registration with direct (but deferred) access to the AssetServer resource without the need for a new public api layer.

Copy link

@bjorn3 bjorn3 Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so. It doesn't help with removing plugins again though for the editor or runtime loadable mods. It also doesn't allow inspecting whatever the plugin injected from the editor as easily.

The current plugin model works well in simple cases, but completely breaks down when the central consumer (typically, an `App`) needs to configure when and if their systems run. The main difficulties are:

- 3rd party plugins can have "unfixable" ordering ambiguities or interop issues with other 3rd party plugins
- plugins can not be added to states, and run criteria cannot be added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing users to add new RunCriteria to a SystemSet defined in a plugin could invalidate assumptions made by the plugin author, especially when before/after constraints dont care if a system actually runs. This could break things in hard-to-identify ways, which concerns me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually one of the main reasons why I like the system sets + can only configure based on a shared label design: plugin authors could carefully control the labels they're exporting to ensure that if A doesn't run, then B also doesn't run.

However, I think this is too implicit for my taste. Instead, I think that there's an important distinction there that we're missing with the way that the current .before works.

If A is before B, and A is skipped, what should B do? The current behaviour is for B to proceed. But that's not always correct: in some cases B should be skipped if A is skipped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree that there are cases where "run B only if A ran" would be a useful tool and could help prevent certain instances of this problem. Haha we're starting to have quite the menagerie of order types: "the current soft-orders", "hard orders (side effects applied", and "only-if orders". We might need to come up with new names for these concepts / sort out the right way to express "soft order", "soft order with side effects", "only-if order", and "only-if order with side effects".

rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
rfcs/33-apps_own_scheduling.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Sep 17, 2021

Edit: added a 7th option that's actually pretty decent!

In response to @cart's comments here I'm increasingly convinced that every system must have at least one label that is visible to the App for one simple reason: unresolveable system order ambiguities are a dealbreaker in production code, and cannot be fixed if the end user does not have labels (or ownership of) both systems.

Why are unresolvable system order ambiguities such a big deal? Fundamentally, because they prevent determinism, and can introduce unfixable non-deterministic bugs that will be very difficult to detect and completely destroy any attempt at . Neither the plugin author or the end user can predict which of their systems are likely to cause these issues: the only safe strategy is to ensure that you have (or can add) a label to each system.

As I see it, there are quite a few approaches we could try to take, most of which are pretty terrible:

  1. Completely forbid system ordering ambiguities by panicking on schedule initialization. This is undesirable because:
    1. Prevents system order ambiguities in quick-and-dirty prototype code.
    2. Encourages a myopic approach to building the system ordering graph.
    3. Unfixable system order ambiguities can still be introduced by adding a new system that's incompatible with a
  2. Force users that care about determinism and test flakiness to audit and then either exclude or vendor every plugin in their dependency tree. This is terrible because:
    1. It fragments the ecosystem with near-duplicate crates as users fork or remake crates to fix ambiguity issues.
    2. It dramatically increases friction when considering trying out new crates as they must be audited.
    3. Without a way to enforce this, determinism-capable plugins / crates can accidentally depart from the informal standard of publicly labelling each system.
    4. Intermediate users will be repeatedly bitten by having to migrate away from a flaky crate after substantial effort building on it once they realize that they care about these things.
  3. Force each plugin to operate in a separate private stage. This is pretty terrible because:
    1. This seriously limits system parallelism, and adds overhead to each dependency you want to use.
    2. Plugins that require operating systems across stage boundaries become a huge mess.
    3. Other systems can never run between two systems within a plugin, even if it's required by the app's logic and desired by the plugin author.
  4. Create a "deterministic" mode for the executor, which runs the schedule in a fixed order relative to any ambiguities. This is a poor choice because:
    1. It massively reduces parallelization of systems by completely forbidding inconsequential ambiguities.
    2. The seed or other ordering must be changed and regenerated every time a system is added or removed.
    3. Tests become extremely brittle, relying on a global magic number that selects a particular valid ordering, recreating the problems with insertion-order system scheduling without the benefits.
  5. Ensure that every system has at least one publicly visible label. This avoids the problem of unresolvable ambiguities without completely exposing everything in a granular fashion but:
    1. We are forced to use a Plugin trait for our API design to enforce that labels are public by making the label type an associated type. See this proof of concept.
    2. Having to create label names for every single exported system set is unergonomic and often redundant.
    3. Reasoning about which systems should share a label is a non-trivial API design question for plugin authors.
    4. Only one label type can be used when adding system sets to a plugin, although arbitrarily-typed labels can be added to this
  6. Automatically provide a system label to each SystemDescriptor. This solves the problem and reduces boilerplate but forces plugin authors to expose serious internal details:
    1. Users can pierce the API boundary provided by plugins by referring to individual systems.
    2. This can increase maintenance costs for plugin authors when attempting to change internal structure if they are concerned with not breaking downstream users who have done so.
    3. Similarly, this can increase upgrade costs for users who are relying on these internal details, preventing them from migrating large code bases to the newest version of Bevy and/or their plugins.
    4. As we have no sensible way to require the functions that get turned into labels to be pub, actually grabbing these labels would be quite challenging, even with the use of type_id.
    5. Closures can be turned into systems, and their types cannot be named.
  7. Automatically provide a shared label to all systems in each Plugin. This behaves similarly to 5, but bypasses some of the API weirdness and boilerplate:
    1. Plugin types are always nameable and visible to the app.
    2. Doesn't pierce the abstraction boundary, as you don't get any more granular access.
    3. Allows fixing of all non-internal ambiguities. Internal ambiguities will be bugs for everyone, so fixes can be upstreamed.
    4. Plugin authors can still choose to export more granular labels.

5, 6 and 7 also share some drawbacks related to the "plugins define constraints" approach:

  1. If our set of constraints is inadequate (or the plugin authors do not provide sufficient constraints), users can subtly break the guarantees that the plugin authors rely on for their functionality to work correctly.
  2. Plugin authors may accidentally over-constrain their systems, reducing flexibility.

2 is the current solution. I feel pretty strongly that all solutions other than 5, 6 or 7 are non-viable:

  • 5 is workable, but requires a lot of boilerplate and constrains our API in some strange ways.
  • 6 is painful but will definitely work (if we forbid closures as systems). It does really bad things to code readability and breaks plugin APIs wide open
  • 7 has virtually all of the upsides of 5 and solves the immediate problem, with much less boilerplate, burden on plugin authors and general strangeness :D

I'm going to incorporate 7 into the RFC for now to make sure it's properly clear so others can provide feedback.

@cart
Copy link
Member

cart commented Sep 17, 2021

(Alice and I are planning on discussing the points above more thoroughly in the near future, especially how they relate to ambiguity problems)

But heres a quick consolidation of my current thoughts and preferences.

  • I do agree that if we decide that every system needs a label, plugin-level labeling is the right call. However I think an "automatically created" label would be better UX.
  • I still have a strong preference for Plugin being a trait implemented on user types (over the "builder functions" proposed above). See my comment above for rationale.
  • I don't yet have a strong opinion on "Plugins have dependencies" vs "Plugins dont have dependencies" (see my comment above for the details of this problem).
  • I think SystemSets shouldn't be "special" as it creates too much cognitive load when users are reasoning about the structure of a program
  • I don't think we should have special "primary key" labels

This is a rough draft of where my head is at:

pub trait Plugin: Default + 'static {
    fn build(&self, plugin: &mut PluginState);
}

#[derive(Default)]
pub struct CustomPlugin;

impl Plugin for CustomPlugin {
    fn build(&self, plugin: &mut PluginState) {
        plugin
            .add_system(hello_world)
            .add_system_set(SystemSet::new().with_system(velocity).with_system(movement))
            .init_resource::<Thing>()
    }
}

// alternatively this could be called PluginBuilder
pub struct PluginState {
    systems: Vec<SystemDescriptor>,
    system_sets: Vec<SystemSet>,
    label: PluginLabel,
}

impl PluginState {
    pub fn new<P: Plugin>() -> Self {
        Self {
            system_sets: Vec::new(),
            systems: Vec::new(),
            label: PluginLabel::of::<P>(),
        }
    }
    pub fn apply(self, app: &mut App) {
        for system in self.systems {
            app.add_system(system.label(self.label.clone()))
        }

        for system_set in self.system_sets {
            app.add_system_set(system_set.label(self.label.clone()))
        }
    }

    /* other plugin builder functions here */
}

#[derive(SystemLabel, Debug, Clone, PartialEq, Eq, Hash)]
pub struct PluginLabel(TypeId);

impl PluginLabel {
    pub fn of<P: Plugin>() -> Self {
        Self(TypeId::of::<P>())
    }
}

Some things to note:

  • Plugin requires Default, which might seem odd at first glance. But note that we are using TypeIds to uniquely identify plugins. TypeIds cannot uniquely identify plugins if we allow duplicates. We will no longer accept direct plugin references on init. Instead we will store types and bevy will initialize the plugin at the appropriate time using the Default impl. This also opens the doors to a number of things:
    • Enables Plugins to declare dependencies easily.
    • Hard requirement of using Resources to configure plugins (which we have discussed a lot in the past and seems like a reasonable pattern to enforce). This also prepares the way for "editor-managed plugin initialization" (ex: you add a plugin to your app from the "asset store" using the Bevy Editor). Because all plugins must be initializable using their type, no user input is required (although theres still the issue of doing code gen, but i digress).
    • Allows users to disable any plugin they want via TypeId at app startup (ex: app.disable_plugin::<RenderPlugin>()).
    • Enables us to replace PluginGroups with Plugins that just register other plugins (if we decide that is better)
      *I didn't include a Resource implementation in PluginState because that will depend on the "plugin dependency" decision. If plugins have dependencies, we can provide direct App access and do "normal resource registration". If plugins dont have dependencies, we would use "init systems with dependencies".

Plugin dependencies would look something like this:

impl Plugin for CustomPlugin {
    fn build(&self, plugin: &mut PluginState) {
        plugin
            // tells the app to register the plugin if it doesn't exist
            .add_plugin::<OtherPlugin>()
            // fails if SomeOtherPlugin isn't registered when the app starts
            // maybe this isn't needed when add_plugin could just init the plugin / avoid the failure in the first place
            .requires_plugin::<SomeOtherPlugin>()
            .add_system(hello_world)
    }
}

// We _could_ remove PluginGroups in favor of Plugins
impl Plugin For DefaultPlugins {
    fn build(&self, plugin: &mut PluginState) {
        plugin
            .add_plugin::<CorePlugin>()
            .add_plugin::<AssetPlugin>()
            .add_plugin::<RenderPlugin>()
            /* other deps here */
    }
}

@alice-i-cecile
Copy link
Member Author

I do agree that if we decide that every system needs a label, plugin-level labeling is the right call. However I think an "automatically created" label would be better UX.

Yep, fully agree and I'm working on that! I think these plugin labels can be automatically inferred from the type of the plugin.

I still have a strong preference for Plugin being a trait implemented on user types (over the "builder functions" proposed above). See my comment above for rationale.

Oh, that's a nice design. I was struggling to both minimize boilerplate and let it be configurable. Let me play with that some more...

I don't yet have a strong opinion on "Plugins have dependencies" vs "Plugins dont have dependencies" (see my comment above for the details of this problem).

Yeah, I don't have strong feelings here at all yet.

I think SystemSets shouldn't be "special" as it creates too much cognitive load when users are reasoning about the structure of a program

Agreed, I'm going to cut this in favor of just exporting a bunch of SystemDescriptors.

I don't think we should have special "primary key" labels

Agreed, this is no longer necessary.

@TheRawMeatball
Copy link
Member

Hmm, using the Plugin type to automatically make the label does seem like a reasonable reason to have the extra layer of indirection over exporting a function.

This is a rough draft of where my head is at

Hmm, I quite like this actually, though I have some questions:

  • If we're forcing config-through-resource, why not cut &self from Plugin::build entirely?
  • PluginLabel<P>(PhantomData<P>) might be better than PluginLabel(TypeId)
  • I wonder if directly using the plugin type itself as the label might be valid...

@alice-i-cecile
Copy link
Member Author

I wonder if directly using the plugin type itself as the label might be valid...

Yep, that's my hope!

@cart
Copy link
Member

cart commented Sep 17, 2021

If we're forcing config-through-resource, why not cut &self from Plugin::build entirely?

Very good point. That also removes the need for Default.

PluginLabel<P>(PhantomData<P>) might be better than PluginLabel(TypeId)

Hmm yeah it would be smaller / cheaper that way. And the DynEq class of stuff will already use TypeId of PluginLabel<P>, so the internal TypeId is redundant.

I wonder if directly using the plugin type itself as the label might be valid...

This would work, but it would requiring deriving SystemLabel for each Plugin, which I would like to avoid if possible.

@alice-i-cecile
Copy link
Member Author

This would work, but it would requiring deriving SystemLabel for each Plugin, which I would like to avoid if possible.

Nope, we can define a automatically implemented get_label method on the Plugin trait, which returns the label for that type and then do:

    app.configure_label(CombatPlugin.get_label().before("Foo"));

@cart
Copy link
Member

cart commented Sep 17, 2021

Thats not using the plugin type as a label. Thats using the plugin instance type to get the label, which is just a rephrasing of PluginLabel::of::<CustomPlugin>().

@alice-i-cecile
Copy link
Member Author

True, but it avoids us having to store or specify label: PluginLabel in PluginState 🤔

@cart
Copy link
Member

cart commented Sep 17, 2021

Truth. And I guess if we made PluginLabel generic, we'd need to make PluginState generic too, which would be nasty.

@TheRawMeatball
Copy link
Member

if we made PluginLabel generic, we'd need to make PluginState generic too, which would be nasty

Why? Looking at it a bit more carefully, I have no idea why we'd even store the plugin label in PluginState.

@cart
Copy link
Member

cart commented Sep 17, 2021

Why? Looking at it a bit more carefully, I have no idea why we'd even store the plugin label in PluginState.

It can definitely be factored out into the higher level "Plugin orchestrator" that ultimately needs to be built. We need something to be able to apply a specific plugin's label to the systems in that plugin. We could do that in some type-erased Box<dyn PluginApplier> (note that if PluginLabel is a generic returned by an auto-impled trait function, Plugin wont be directly boxable). For the simplicity of illustrating the concept I just encapsulated the logic in PluginState.

Credit to @cart for the design, and @TheRawMeatball for significant feedback
@Nilirad
Copy link

Nilirad commented Sep 20, 2021

What about using SomePlugin::label() instead of SomePlugin::get_label()? It looks more readable, especially in the context of App configuration.

@alice-i-cecile
Copy link
Member Author

What about using SomePlugin::label() instead of SomePlugin::get_label()? It looks more readable, especially in the context of App configuration.

That's a perfectly reasonable bike-shed. I initially steered away from this, since system.label("foo") applies a label, rather than returns one.

I think the readability improvements are worth it though.

@Nilirad
Copy link

Nilirad commented Sep 20, 2021

My two cents to the unresolved questions

  1. Should we rename SystemDescriptor to something more user-friendly like ConfiguredSystem?

If they are not user facing, I think not. Either way, I thought about SystemConfiguration instead.

  1. Do we still need plugin groups at all? Can we replace them with a simple standard collection? The ergonomics matter much less with App::default() initialization.

I think we don't strictly need them, but we can still keep them. They could just have the same power of the current plugins. Having whole App control, they can both add the necessary plugins and then anchor them to the right stages. See it as a prepackaged partial App configuration.

@alice-i-cecile
Copy link
Member Author

I think we don't strictly need them, but we can still keep them. They could just have the same power of the current plugins. Having whole App control, they can both add the necessary plugins and then anchor them to the right stages. See it as a prepackaged partial App configuration.

I'd really like to avoid blurring the line there. I think there's also room for some prepackaged App configuration, but it should either be an extra field on PluginState or preferably another form of exported data that users can add to the app. Adding this to PluginGroup is just messy.

I'm not sure we have enough context / experience to know exactly what form this extra app configuration might hold though, so I think we should wait to make that decision. Even something like "crates can choose to extend App with their own builder methods" would probably work well, and we can add that on top of this design once we have a clearer picture of things like nested schedules or how to make the winit runner more tweakable. The scope of this RFC is already huge enough though, so I'd like to defer that to another thread.

@stefnotch
Copy link

stefnotch commented Apr 26, 2023

Hi!
I got to try out this RFC, by implementing parts of it myself. Here's what I've learned so far, hopefully that's helpful information:

So, my smol team is only using the bevy_ecs crate and nothing else. As such, it made sense for us to take a stab at implementing this and using it internally. The first thing is that the RFC was written before Bevy 0.10, so it still assumes all of the stages stuff. Not a problem though, since the RFC already took the stageless future into account and mostly works nicely with it.

So far, the easiest plugins to write were those that are self-contained, and only have a single system set. Like a time plugin that just has a "update" system set.
Plugins with multiple system sets are also pretty elegant. E.g. A hypothetical physics plugin with a "read from game world and update physics world", a "physics step" and a "write back to game world" set . In the plugin, one simply specifies the order in which those sets run. And then the user can properly use the plugin, and even do advanced things like running independent game logic while parts of the physics plugin are running.

The annoying bit in terms of API design for me was "how do I expose the system sets of a plugin". It doesn't feel very ergonomic at the moment.

  • One could make a separate enum with a similar name?
    Like #[derive(SystemSet)] pub enum PhysicsPluginSet { ...}
    But that doesn't work for generic plugins. (Edit: Unless it actually does, maybe I just need more generic constraints)

  • One could add one function per set to the plugin and use TypeId in that function?
    That seems a bit rough though.

  • Something else?

An unrelated tricky bit is debugging the schedule ordering. There is a bevy plugin which visualizes a schedule, but it's not clear how well maintained it is ( jakobhellermann/bevy_mod_debugdump#7 ). And that plugin assumes that one uses all of bevy.

Another issue that I assume is somewhat tough to figure out is "default plugin ordering". As in, a newcomer probably doesn't want to deal with masterfully ordering every single plugin for maximum performance. Lots of plugins could specific a sensible default, like "the time update plugin should run at the beginning of a frame".
But maybe that's easy to do when one uses more than just bevy_ecs.

IMO, this RFC is really nice. If well implemented, then the drawback of less powerful plugins would absolutely be compensated by "plugins are more likely to properly expose system sets and thus easier to configure in terms of ordering".

@stefnotch
Copy link

image

And here's an example of what I'm doing at the moment. :)

@hymm
Copy link

hymm commented Apr 26, 2023

"how do I expose the system sets of a plugin".

cart discussed a solution for this tentatively called ScheduleTemplate's on discord here. The basic idea is that a plugin can define a function that will add some preconfigured systems to any schedule.

// TransformPlugin
app.schedule_template(TransformTemplate, || (
  PropagateTransformsSet.in_set(TransformSystem::TransformPropagate),
  sync_simple_transforms.in_set(TransformPropagation),
  propagate_transforms.in_set(PropagateTransformsSet),
))

// User app
app.add_template_to_schedule(MyCustomSchedule, TransformTemplate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Design
Development

Successfully merging this pull request may close these issues.

None yet