can only add a plugin once #3988
Conversation
alice-i-cecile
added
A-App
james7132
added
the
S-Ready-For-Final-Review
| ) { | ||
| if let Some(existing_from_group) = self.plugins.insert(*plugin_type, from_group) { | ||
| match (from_group, existing_from_group) { | ||
| (None, None) => panic!("Plugin \"{}\" was already added", plugin_name), |
There was a problem hiding this comment.
Before we add panics here, I think we should discuss moving in the direction of "plugins add the plugins they depend on and bevy only initializes each plugin once". With this, dependencies would need to be implicit. But we could make them explicit if we just de-dupe unique plugins.
However this gets a bit messy if we couple configuration to specific plugin instances, because then we've added the ability to init with multiple conflicting configs.
There was a problem hiding this comment.
I agree, a more comprehensive solution with some more involved architectural design of how plugins work would be my preferred solution. You’re small but significant fix of collecting all the plugins and deduping then initializing is good and easier fix. At some point bevy will need a more sophisticated plugin architecture, do you have an idea when you would want to work on that (or just let alice do it in her existing rfc for this more complex but more robust plugin architecture after stageless is rolled out?
alice-i-cecile
added
the
X-Controversial
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
mockersf
force-pushed
the
plugin-control
branch
from
8a83f6e to
2fc76ec
Compare
| @@ -835,10 +879,17 @@ impl App { | |||
| T: PluginGroup, | |||
| F: FnOnce(&mut PluginGroupBuilder) -> &mut PluginGroupBuilder, | |||
| { | |||
| if self.plugins.insert(TypeId::of::<T>(), None).is_some() { | |||
There was a problem hiding this comment.
Is the unique constraint on groups needed? If it contains a unique plugin, it will already fail
There might be libraries adding groups of not unique plugins.
| @@ -749,6 +749,14 @@ impl App { | |||
| self | |||
| } | |||
|
|
|||
| /// Check that a plugin has already been added to the app. | |||
| pub fn is_plugin_added<T>(&self) -> bool | |||
There was a problem hiding this comment.
I find the naming a bit confusing here. Not every plugin is added to the hashmap. At the moment it also contains plugin groups.
Maybe is_unique_plugin_added would fit better?
|
Closing in favor of #6411; we can reopen if others feel strongly that this complexity is warranted. |
# Objective - Make it impossible to add a plugin twice - This is going to be more a risk for plugins with configurations, to avoid things like `App::new().add_plugins(DefaultPlugins).add_plugin(ImagePlugin::default_nearest())` ## Solution - Panic when a plugin is added twice - It's still possible to mark a plugin as not unique by overriding `is_unique` - ~~Simpler version of~~ #3988 (not simpler anymore because of how `PluginGroupBuilder` implements `PluginGroup`)
# Objective - Make it impossible to add a plugin twice - This is going to be more a risk for plugins with configurations, to avoid things like `App::new().add_plugins(DefaultPlugins).add_plugin(ImagePlugin::default_nearest())` ## Solution - Panic when a plugin is added twice - It's still possible to mark a plugin as not unique by overriding `is_unique` - ~~Simpler version of~~ bevyengine#3988 (not simpler anymore because of how `PluginGroupBuilder` implements `PluginGroup`)
split from #2988