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

Expressively define plugins using functions #11080

Merged
merged 7 commits into from
Jan 27, 2024

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Dec 24, 2023

Objective

Plugins are an incredible tool for encapsulating functionality. They are low-key one of Bevy's best features. Combined with rust's module and privacy system, it's a match made in heaven.

The one downside is that they can be a little too verbose to define. 90% of all plugin definitions look something like this:

pub struct MyPlugin;

impl Plugin for MyPlugin {
    fn build(&self, app: &mut App) {
        app.init_resource::<CameraAssets>()
           .add_event::<SetCamera>()
           .add_systems(Update, (collect_set_camera_events, drive_camera).chain());
    }
}

Every so often it gets a little spicier:

pub struct MyGenericPlugin<T>(PhantomData<T>);

impl<T> Default for MyGenericPlugin<T> { 
    fn default() -> Self { ... }
 }

impl<T> Plugin for MyGenericPlugin<T> { ... }

This is an annoying amount of boilerplate. Ideally, plugins should be focused and small in scope, which means any app is going to have a lot of them. Writing a plugin should be as easy as possible, and the only part of this process that carries any meaning is the body of fn build.

Solution

Implement Plugin for functions that take &mut App as a parameter.

The two examples above now look like this:

pub fn my_plugin(app: &mut App) {
    app.init_resource::<CameraAssets>()
       .add_event::<SetCamera>()
       .add_systems(Update, (collect_set_camera_events, drive_camera).chain());
} 

pub fn my_generic_plugin<T>(app: &mut App) {
    // No need for PhantomData, it just works.
}

Almost all plugins can be written this way, which I believe will make bevy code much more attractive. Less boilerplate and less meaningless indentation. More plugins with smaller scopes.


Changelog

The Plugin trait is now implemented for all functions that take &mut App as their only parameter. This is an abbreviated way of defining plugins with less boilerplate than manually implementing the trait.

@JoJoJet JoJoJet added C-Enhancement A new feature A-ECS Entities, components, systems, and events labels Dec 24, 2023
@Nilirad
Copy link
Contributor

Nilirad commented Dec 24, 2023

Great idea! Wouldn't C-Usability fit more than C-Enhancement?

@MrGVSV
Copy link
Member

MrGVSV commented Dec 24, 2023

It's worth calling out prior art to demonstrate the usefulness and desire for this pattern:

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

It might be a little weird having mixed-case plugins haha but overall I like the simplicity of this new approach— especially for generic plugins.

@MrGVSV
Copy link
Member

MrGVSV commented Dec 24, 2023

As a side note, if we continue with this approach, I think we should make a distinction of when it should be used style-wise. As in, engine code should use the current method for plugins so as to avoid breakages and examples (or "user code") should use this new method for plugins.

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 24, 2023

Wouldn't C-Usability fit more than C-Enhancement?

I can never tell when to use either tag. However I chose C-Enhancement since this is introducing a new pattern that I expect to be widespread.

@Nilirad
Copy link
Contributor

Nilirad commented Dec 24, 2023

I can never tell when to use either tag. However I chose C-Enhancement since this is introducing a new pattern that I expect to be widespread.

If I understood correctly C-Enhancement is for new features, like relations or hooks. C-Usability simply eases developer experience.

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
@JoJoJet JoJoJet added C-Usability A simple quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins and removed C-Enhancement A new feature A-ECS Entities, components, systems, and events labels Dec 24, 2023
Copy link
Contributor

@rlidwka rlidwka left a comment

Choose a reason for hiding this comment

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

This works for plugins that only have build function and no configuration. In my code that's almost all of them.

I agree with this, it seems to simplify things a lot.

@rlidwka
Copy link
Contributor

rlidwka commented Dec 25, 2023

I think we should make a distinction of when it should be used style-wise. As in, engine code should use the current method for plugins so as to avoid breakages and examples (or "user code") should use this new method for plugins.

Private plugins that are only created for better modularity would be functions.

Top-level plugins in crates and other public API in general would be structs (they most likely have some configuration anyway).

Although making almost everything functions is possible via closures, it's too early to make any judgement on that.

@JoJoJet JoJoJet added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 25, 2023
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Dec 25, 2023
@alice-i-cecile
Copy link
Member

As an incremental step: I really like this!

It's a simple improvement that reduces boilerplate and fully captures everything it means to be a plugin.

However, I'm quite nervous about how this would interact with other plans for plugins. For example, automatically labelling systems from a plugin with the plugin type for #2160, or a more structured approach with multiple methods to tackle #1255. From my perspective, this code is likely to break or complicate those efforts.

If you can convince me that it's either worth it in the interim, or that this flavor of approach can coexist with those plans (or if there's an alternate solution to those serious longstanding problems in our plugin architecture!), then I'd be happy to approve this and reduce boilerplate!

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 25, 2023

However, I'm quite nervous about how this would interact with other plans for plugins. For example, automatically labelling systems from a plugin with the plugin type for #2160,

TBH, automatically labelling systems from a plugin seems like a kind of strange thing to want. However it could still be done, using the same trick we use for SystemTypeSet. We can implement IntoSystemSet<...> for all Fn(&mut App), which would allow specifying dependencies on these plugins.

or a more structured approach with multiple methods to tackle #1255. From my perspective, this code is likely to break or complicate those efforts.

About plugins with build order. This is an important feature, however I don't think it makes much sense for build dependencies to be specified internally to a plugin. If we want graph-like plugin build order, we should specify dependencies externally similar to how systems are scheduled. If I want plugin B to build after plugin A, I should just write app.add_plugins(B.after(A)). If I want to add them both at once, I should write .add_plugins((A, B).chain()). If I want to encapsulate these dependencies, I should wrap them in another plugin. This sort of build dependency is compatible with the PR at hand.

Whatever form that feature ends up taking, I feel like this PR will still be applicable. Even if we have powerful new tools for ordering plugins, that only matters for a small number of plugins. Most plugins are just containers of configuration added to an app, which can be easily expressed with a single function. If we end up adding more methods to the Plugin trait for ordering, we can just add defaults for them.

Merry Christmas BTW 🎄

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm convinced: I think this is a nice step forward then.

@NthTensor
Copy link
Contributor

If I want plugin B to build after plugin A, I should just write app.add_plugins(B.after(A)). If I want to add them both at once, I should write .add_plugins((A, B).chain()). If I want to encapsulate these dependencies, I should wrap them in another plugin. This sort of build dependency is compatible with the PR at hand.

Agreed. It's easy to see how we could implement after, chain and possibly even before for plugins, if both this and explicit plugin dependencies make it in. Seems like a great change.

@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 7, 2024

I feel like before would be tricky to implement. after dependencies seem fairly straightforward: if a plugin with a dependency gets added, just wait to call .build() until after build gets called for its dependency. It seems more complicated with before though: If I call app.add_plugins(A), and then later call add_plugins(B.before(A)), there's no way to know when adding A that it will have a dependency on B, so we don't know that we have to wait to build A.

@NthTensor
Copy link
Contributor

I was more thinking as an extension to #11228.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I think this is a great improvement in terms of ergonomics and generally am OK with merging as is.

However, I do think some things need to be considered here. Namely the mental separation and impact on documentation discoverablity. Functions have historically been only systems or actual functions in Bevy-related projects. Likewise plugins have been discoverable as structs or enums, but that consistency won't be maintained going forward. These aren't hard blocker issues, but we may need to think on what impact this will have on the ecosystem as a whole.

@@ -60,6 +94,12 @@ pub trait Plugin: Downcast + Any + Send + Sync {

impl_downcast!(Plugin);

impl<T: Fn(&mut App) + Send + Sync + 'static> Plugin for T {
Copy link
Member

Choose a reason for hiding this comment

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

This prevents us from making any other auto-impl, which is not an issue onto itself, but I think this is worth calling out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. Even if we can't add more impls for Plugin directly, the Plugins<M> trait has a marker generic, which means we could add auto-impls that allow us to call .add_plugins(...) for additional types

@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 7, 2024

Functions have historically been only systems or actual functions in Bevy-related projects.

Commands and EntityCommands can also be defined as functions, so I think there's precedent for using functions to implement a trait.

@TotalKrill
Copy link
Contributor

TotalKrill commented Jan 11, 2024

Just adding my two cents here: the lack of something removes the discoverability and expressiveness of things. And while I really would like the ability to have functions as plugins, I would also want it quite clear that a function is a plugin somehow by just looking at the function.

One example is that we now enforce the #[derive(Component)] and #[derive(Resource)] on structures. I would love if we could do something similar to functions.

I have noticed that since sometimes, we declare functions that are basically only helper functions inside a system. The mental overhead of keeping track of what is a system, and what is a regular function is a bit taxing, especially so if you are new to the codebase, new to bevy, or going back to fix or change something old.

ideally without i would like it if the functions that are systems was declared something like:

#[system]
fn a_system(mut commands: Commands) {
   ...code
}

and likewise I would prefer if we now would add plugins as a system:

#[plugin]
fn a_plugin(mut commands: Commands) {
   ...code
}

The issue is mainly when you have a plugin, a helper function and a system, and they look like this:

fn foo(mut commands: Commands) {
   ...code
}

fn bar(mut commands: Commands) {
   ...code
}

fn zed(mut command: Commands) {
   ...code
}

Please tell me what is a plugin and what is a system? Because how I add them to app will surely change things, and most likely result in a runtime crash

@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 12, 2024

I think that whenever we use or advertise this feature, we have to be really strict about the _plugin suffix. If you're lazy about naming then I agree - plugin functions would get confusing fast. However I don't think it's a problem if people are good about following this convention, and I think they will be. The convention of naming struct plugins with the struct SomethingPlugin convention is ubiquitous, and it nicely translates over to snake_case functions such as fn something_plugin().
I've never found systems to be a source of confusion. System param types look quite distinct which makes it pretty easy to tell them apart from regular functions.
Just my 2c

@Nilirad
Copy link
Contributor

Nilirad commented Jan 12, 2024

I like the idea of marking systems and plugins with attributes, even more than adding a suffix to the identifier, as it is statically checked by the compiler. Unfortunately, Cart have always been determined to not follow this style, and for the time being, it has been a reasonable decision that kept system definition very simple.
However, since now we're trying to use functions even to define plugins, usage of attributes may become more valid. Plus, it has been some time since we've started explicitly implementing traits for specific ECS roles, such as components, resources and events, instead of relying on blanket implementations.
In any case, this is a discussion that can be deferred after this PR gets merged, to see if there are some relevant sources of confusion.

@alice-i-cecile
Copy link
Member

Merging with @JoJoJet as SME-ECS.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 27, 2024
Merged via the queue into bevyengine:main with commit d66c868 Jan 27, 2024
26 checks passed
@JoJoJet JoJoJet deleted the fn-plugins branch January 27, 2024 02:56
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Plugins are an incredible tool for encapsulating functionality. They are
low-key one of Bevy's best features. Combined with rust's module and
privacy system, it's a match made in heaven.

The one downside is that they can be a little too verbose to define. 90%
of all plugin definitions look something like this:

```rust
pub struct MyPlugin;

impl Plugin for MyPlugin {
    fn build(&self, app: &mut App) {
        app.init_resource::<CameraAssets>()
           .add_event::<SetCamera>()
           .add_systems(Update, (collect_set_camera_events, drive_camera).chain());
    }
}
```

Every so often it gets a little spicier:

```rust
pub struct MyGenericPlugin<T>(PhantomData<T>);

impl<T> Default for MyGenericPlugin<T> { 
    fn default() -> Self { ... }
 }

impl<T> Plugin for MyGenericPlugin<T> { ... }
```

This is an annoying amount of boilerplate. Ideally, plugins should be
focused and small in scope, which means any app is going to have a *lot*
of them. Writing a plugin should be as easy as possible, and the *only*
part of this process that carries any meaning is the body of `fn build`.

## Solution

Implement `Plugin` for functions that take `&mut App` as a parameter.

The two examples above now look like this:

```rust
pub fn my_plugin(app: &mut App) {
    app.init_resource::<CameraAssets>()
       .add_event::<SetCamera>()
       .add_systems(Update, (collect_set_camera_events, drive_camera).chain());
} 

pub fn my_generic_plugin<T>(app: &mut App) {
    // No need for PhantomData, it just works.
}
```

Almost all plugins can be written this way, which I believe will make
bevy code much more attractive. Less boilerplate and less meaningless
indentation. More plugins with smaller scopes.

---

## Changelog

The `Plugin` trait is now implemented for all functions that take `&mut
App` as their only parameter. This is an abbreviated way of defining
plugins with less boilerplate than manually implementing the trait.

---------

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants