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

Added bevy_dynload to replace the dynamic_plugin feature #544

Merged
merged 5 commits into from
Oct 1, 2020

Conversation

EthanYidong
Copy link
Contributor

The load_plugin function under the AppBuilder impl has been moved to its own crate.

@EthanYidong EthanYidong changed the title Added bevy_dynload to replace the dynamic_plugin feature (#415) Added bevy_dynload to replace the dynamic_plugin feature Sep 21, 2020
@memoryruins memoryruins added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change labels Sep 21, 2020
@EthanYidong
Copy link
Contributor Author

This introduces breaking changes with having to import DynamicPluginExt to use AppBuilder::load_plugin(). This is fine in most cases as it's included in bevy::prelude, but might break some people's code. Would it be a good idea to wait for 0.3 to introduce this?

@cart
Copy link
Member

cart commented Sep 21, 2020

I like these changes, but I think I'd prefer the slightly more specific/explicit bevy_dynamic_plugin as the crate name and dynamic_plugin as the feature name. bevy_dynload is both ambiguous with the dyn rust keyword and could cover "all" dynamic contexts, whereas I think this crate should be scoped to dynamic plugins.

@memoryruins
Copy link
Contributor

memoryruins commented Sep 21, 2020

Would it be a good idea to wait for 0.3 to introduce this?

I appreciate the consideration. It's probably best to minimize our assumptions about prelude, especially when we have a growing plugin ecosystem where they might not depend on the main bevy crate.

For users of the dynamic_plugins feature, the new feature name is a breaking change before prelude consideration. Changing to bevy_dynload or dynamic_plugin (without the s) would lead cargo to error with the following for those users:

error: failed to select a version for `bevy`.
the package `project` depends on `bevy`, with features: `dynamic_plugins` but `bevy` does not have these features.

@cart to avoid this issue altogether, should we keep the original (plural) feature name: dynamic_plugins?

@EthanYidong
Copy link
Contributor Author

@cart @memoryruins I've renamed the crate to bevy_dynamic_plugin and the feature back to dynamic_plugins so it should be more compatible with the current features. Eventually it might be a good idea to make this more in line with the other bevy_* crates/features, but for now, the only breaking change is now having to import DynamicPluginExt.

@cart
Copy link
Member

cart commented Sep 21, 2020

@memoryruins I do like the thought process in general, but in this case people shouldn't really be using dynamic plugins anyway. Also at this stage in the project I think I want to value consistency + correctness over backward compatibility.

In general for "crate" features I want them to be in the style: crate_name == bevy_crate and feature == crate.

So the question is, do we think bevy_dynamic_plugins or bevy_dynamic_plugin is the better crate name. In general I've favored singular names over plural names (ex: window instead of windows). So I think my preference here is bevy_dynamic_plugin and dynamic_plugin.

The DynamicPluginExt breaking change is totally fine imo. I also do want AppBuilder extensions to be in the bevy prelude to make them work "by default". For "plugin developers" that consume individual crates directly, its actually probably good that they can't register dynamic plugins by default anymore / importing the extensions are a manual step. In general dynamic plugin loading should only be used by developers to speed up their process.

I think I'm cool with merging "breaking changes" into master. If we need to do 0.2 maintenance releases, we can cherry pick changes.

@memoryruins
Copy link
Contributor

memoryruins commented Sep 21, 2020

@cart I agree. We shouldn't impede those goals when a) many things to experiment with and figure out b) there is an explicit warning about this on the readme. I mentioned the error to avoid surprise if included in a patch version, though it's unlikely that anyone is using that feature in this way today.

In general for "crate" features I want them to be in the style: crate_name == bevy_crate and feature == crate.

This is not the case for most of the current features that enable crates. If the feature enables a crate, it uses the name of the crate. This is the inconsistency of using the feature name dynamic_plugin(s) to enable bevy_dynamic_plugin that @EthanYidong was pointing out.

edit: that is to say, would you like the other current feature names to be changed in another PR?

@memoryruins
Copy link
Contributor

So the question is, do we think bevy_dynamic_plugins or bevy_dynamic_plugin is the better crate name. In general I've favored singular names over plural names (ex: window instead of windows). So I think my preference here is bevy_dynamic_plugin and dynamic_plugin.

Let's go with bevy_dynamic_plugin for the crate name. 👍

@cart
Copy link
Member

cart commented Oct 1, 2020

Ahh I see your point about the inconsistency now. I think using bevy_dynamic_plugin as the feature name is probably a good idea, just to keep things simple.

@cart
Copy link
Member

cart commented Oct 1, 2020

I think I'll just make the change + rebase so we can get this merged asap.

@cart cart force-pushed the add-libloading-crate branch from dacdf1c to ffa1dca Compare October 1, 2020 19:45
@cart cart merged commit 4c753e2 into bevyengine:master Oct 1, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
@DJMcNab
Copy link
Member

DJMcNab commented Jul 6, 2021

@EthanYidong please respond in #2373 for the relicense to MIT/Apache 2.0. Thanks!

@DJMcNab
Copy link
Member

DJMcNab commented Jul 18, 2021

@EthanYidongArchives please have a look at ^, although you'd need to comment using the account @EthanYidong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants