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

app: PluginGroups and DefaultPlugins #744

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Conversation

cart
Copy link
Member

@cart cart commented Oct 29, 2020

Resolves #65
Alternative to #721

This adds the concept of a PluginGroup which is an "ordered collection of plugins, where each plugin can be enabled or disabled".

It migrates the "default plugins" to a DefaultPlugins plugin group:

App::build()
     // this is now just shorthand
    .add_default_plugins()
     // for this
    .add_plugin_group(DefaultPlugins)

You can also define custom plugin groups:

pub struct HelloWorldPlugins;

impl PluginGroup for HelloWorldPlugins {
    fn build(&mut self, group: &mut PluginGroupBuilder) {
        group
            .add(PrintHelloPlugin)
            .add(PrintWorldPlugin);
    }
}

app.add_plugin_group(HelloWorldPlugins);

Or disable plugins:

App::build()
    .add_plugin_group_with(DefaultPlugins, |group| {
        group.disable::<RenderPlugin>()
    })

Or "replace" plugins:

App::build()
    .add_plugin_group_with(DefaultPlugins, |group| {
        group.disable::<AssetPlugin>()
             .add_after::<AssetPlugin, _>(MyAssetPlugin)
    })

@cart
Copy link
Member Author

cart commented Oct 29, 2020

Open question: should we deprecate app.add_default_plugins() in favor of app.add_plugin_group(DefaultPlugins)

@mockersf
Copy link
Member

nice, way more generic than my proposition 👍

Or "replace" plugins:

App::build()
    .add_plugin_group_with(DefaultPlugins, |group| {
        group.disable::<AssetPlugin>()
             .add_after::<AssetPlugin, _>(MyAssetPlugin)
    })

Strictly from an api point of view, it doesn't seem intuitive when I want to replace, to first disable then add_after. Could there be a function replace that would do both for me?

@mockersf
Copy link
Member

Open question: should we deprecate app.add_default_plugins() in favor of app.add_plugin_group(DefaultPlugins)

I would say yes if you want to promote the existence / usage of other plugin groups than DefaultPlugins. One for 2D? for 3D? for headless? I'm not sure other groups are meaningful right now

@cart
Copy link
Member Author

cart commented Oct 29, 2020

Strictly from an api point of view, it doesn't seem intuitive when I want to replace, to first disable then add_after. Could there be a function replace that would do both for me?

Happy to discuss it, but that was an intentional omission. I don't really want to create the idea that you are "replacing" a plugin of a given type. It produces a false correlation between the two plugins when there isn't really anything forcing them to be "the same". Additionally, registration order might need to be different for plugins that replace other plugins.

Semantically I think I would prefer it if "replacement" was "disable X"+"insert Y at some location". instead of "replace X with Y"

I would say yes if you want to promote the existence / usage of other plugin groups than DefaultPlugins. One for 2D? for 3D? for headless? I'm not sure other groups are meaningful right now

Yeah I think you're right. I also generally like there to be "exactly one way of doing things".

@mockersf
Copy link
Member

mockersf commented Oct 29, 2020

Semantically I think I would prefer it if "replacement" was "disable X"+"insert Y at some location". instead of "replace X with Y"

Something like use_in_place_of rather than replace? English is not my native tongue, not sure if that conveys what you're meaning better.
Anyway it's an advanced usage, I think it's not that much an issue if how to do it is not obvious and actually requires to read the doc to make sure not to make an error

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels Oct 29, 2020
@cart
Copy link
Member Author

cart commented Oct 29, 2020

Yeah i think for now I'd prefer it if we kept the API small. If "why can't I replace ergonomically" is a common papercut we can revisit this (and you can always use extension methods to add your own group.replace if you want)

@cart cart merged commit bf2a917 into bevyengine:master Oct 29, 2020
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin overriding
3 participants