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

make Plugin::build consume the plugin #1045

Closed

Conversation

blunted2night
Copy link
Contributor

@blunted2night blunted2night commented Dec 11, 2020

This patch makes the build method of the Plugin trait consume the plugin. This allows the plugin to transport non-clone-able configuration data into the build process of the plugin. Because this changes a core trait, it ends up touching a-lot, but in the vast majority of cases the change is just removing a & from self in the build method. It will break most users too, but the fix should in most if not all cases should just be removal of the & from self in the build method. It also changes the signature of the dynamically loaded plugin factory function slightly.

The relevant changes, those not just fixing the signature, are in "crates/bevy_app/src/plugin.rs" and "crates/bevy_app/src/plugin_group.rs".

There are two instances in the existing code base that utilized the plugin to transport data into the build process, the PrintDiagnosticsPlugin and the example PrintMessagePlugin which no longer needed to clone the data and can instead just transfer ownership.

My use case for this a precursor for one last alternate approach to overriding the AssetIo instance used in the AssetServer. With this in place, it would be possible to implement overriding the AssetIo like so:

use bevy::asset::AssetPlugin;

app.add_plugins_with(DefaultPlugins, |group| {
    group
        .disable::<AssetPlugin>()
        .add_after::<AssetPlugin, _>(AssetPlugin::with_io (CustomAssetIo))
});

This patch is necessary for this approach to work since AssetIo does not, and should not, in my opinion, implement Clone.

I don't know what future plans may be in store for the plugin system, so this might be taking things in the wrong direction.

@cart
Copy link
Member

cart commented Dec 13, 2020

I think I'll hold off on merging this for a bit because I'm not quite sure I want plugins to be "one time use". Right now they serve as "re-usable arbitrary app initializers". We don't use that property today, but there may be a scenario where its desirable (ex: editors / dynamic plugins / multiple identical app instances / etc). It also sort of goes against the pattern of "configuring plugins via resources".

Is the solution we came to in #1037 a suitable middle ground in the interim?

app
    .add_resource(AssetServer::new(CustomAssetIo, task_pool))
    .add_plugins(DefaultPlugins) 

@blunted2night
Copy link
Contributor Author

Yeah, this was just an idea that popped into my head.

@cart
Copy link
Member

cart commented Dec 13, 2020

Cool cool. Yeah the approach in this pr might be the right call. I just want to think a bit about the implications first (which I can't do thoroughly right now).

@Moxinilian Moxinilian added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible labels Dec 15, 2020
@alice-i-cecile alice-i-cecile 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 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 as I don't see much of a reason to break things (and we might want Plugins to be reusable for certain scenarios). We can re-open this if/when the need arises.

@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
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants