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

Allow reconfiguring a Plugin inside a PluginGroup #1382

Closed
wants to merge 2 commits into from

Conversation

rmsc
Copy link
Contributor

@rmsc rmsc commented Feb 2, 2021

This is a simple change that allows one to reconfigure a plugin inside a PluginGroup. I'm not sure if this is to be allowed or not, but it allows plugin crates to provide their own specialized version of the DefaultPlugins.

A good use case would be if a Plugin requires some change to the BaseRenderGraphConfig, but would otherwise play nice with the rest of the DefaultPlugins.

To illustrate how this would work, right now we could disable exit_on_close like this:

fn main() {
    App::build()
        .add_plugins_with(DefaultPlugins |group| {
            group.disable::<bevy::window::WindowPlugin>()
        })
        .add_plugin(bevy::window::WindowPlugin {
            add_primary_window: true,
            exit_on_close: false
        })
        .run();
}

Now lets imagine a crate that needs such change to work. Such crate would either have to create (and maintain) their own full version of the DefaultPlugins, or force the user to always include the above boilerplate.

With this PR the crate would provide this:

pub struct DefaultFooPlugins;

impl PluginGroup for DefaultFooPlugins {
    fn build(&mut self, group: &mut PluginGroupBuilder) {
        let mut default_plugins = DefaultPlugins {};
        default_plugins.build(group);
        group.reconfigure(bevy::window::WindowPlugin {
            add_primary_window: true,
            exit_on_close: false
        });
        group.add(FooPlugin);
    }
}

and the user would just have to do this:

fn main() {
    App::build()
        // .add_plugins(DefaultPlugins)
        .add_plugins(DefaultFooPlugins)
        .run();
}

The

@rmsc rmsc changed the title Plugin group reconfigure Allow reconfiguring a Plugin inside a PluginGroup Feb 2, 2021
@cart
Copy link
Member

cart commented Feb 2, 2021

This is a good idea, but before merging I think we need to decide what #1255 looks like (and in general how we want to handle plugin configuration).

I've currently been pushing for a move to "resources as config" over setting config in the plugin type itself because "dynamic plugin loading" implies that the actual type of the plugin (and its constructor) is erased at load time. It also opens the door to a more "dynamic" form of configuration that can be modified at runtime from within the ecs.

This pr might be the way to go, but before making this style of configuration easier, I want to make sure this is the direction we want to go with.

@alice-i-cecile
Copy link
Member

I've currently been pushing for a move to "resources as config" over setting config in the plugin type itself because "dynamic plugin loading" implies that the actual type of the plugin (and its constructor) is erased at load time. It also opens the door to a more "dynamic" form of configuration that can be modified at runtime from within the ecs.

I like this idea quite a lot: it's natural, and tackles some of the bigger questions about how to integrate a heterogenous ecosystem in a single program.

For posterity: this also ties into some of the much broader thinking around config / metadata in general as seen in #1375.

@rmsc
Copy link
Contributor Author

rmsc commented Feb 3, 2021

I agree, lets wait. Plus this did feel a bit like abusing the existing API.

@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@cart
Copy link
Member

cart commented Mar 3, 2021

I'm going to close this for now in favor of "resources as config". I'm happy to revisit later if/when that ceases to be the pattern we're encouraging. Also feel free to continue the discussion here if there are dissenting opinions.

@cart cart closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants