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

can only add a plugin once #3988

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 103 additions & 6 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use bevy_ecs::{
world::World,
};
use bevy_utils::{tracing::debug, HashMap};
use std::fmt::Debug;
use std::{any::TypeId, fmt::Debug};

#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;
Expand Down Expand Up @@ -57,6 +57,7 @@ pub struct App {
/// A container of [`Stage`]s set to be run in a linear order.
pub schedule: Schedule,
sub_apps: HashMap<Box<dyn AppLabel>, SubApp>,
plugins: HashMap<std::any::TypeId, Option<&'static str>>,
}

/// Each `SubApp` has its own [`Schedule`] and [`World`], enabling a separation of concerns.
Expand Down Expand Up @@ -100,6 +101,7 @@ impl App {
schedule: Default::default(),
runner: Box::new(run_once),
sub_apps: HashMap::default(),
plugins: HashMap::default(),
}
}

Expand Down Expand Up @@ -368,7 +370,6 @@ impl App {
stage_label: impl StageLabel,
system: impl IntoSystemDescriptor<Params>,
) -> &mut Self {
use std::any::TypeId;
assert!(
stage_label.type_id() != TypeId::of::<StartupStage>(),
"add systems to a startup stage using App::add_startup_system_to_stage"
Expand Down Expand Up @@ -403,7 +404,6 @@ impl App {
stage_label: impl StageLabel,
system_set: SystemSet,
) -> &mut Self {
use std::any::TypeId;
assert!(
stage_label.type_id() != TypeId::of::<StartupStage>(),
"add system sets to a startup stage using App::add_startup_system_set_to_stage"
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

where
T: Plugin,
{
self.plugins.contains_key(&TypeId::of::<T>())
}

/// Adds a single [`Plugin`].
///
/// One of Bevy's core principles is modularity. All Bevy engine features are implemented
Expand All @@ -767,11 +775,41 @@ impl App {
where
T: Plugin,
{
debug!("added plugin: {}", plugin.name());
debug!("added plugin {}", plugin.name());
if plugin.is_unique() {
self.register_plugin(&std::any::TypeId::of::<T>(), plugin.name(), None);
}
plugin.build(self);
self
}

/// Checks that a plugin has not already been added to an application. It will panic with an
/// helpful message the second time a plugin is being added.
pub(crate) fn register_plugin(
&mut self,
plugin_type: &TypeId,
plugin_name: &str,
from_group: Option<&'static str>,
) {
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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

(None, Some(existing_from_group)) => panic!(
mockersf marked this conversation as resolved.
Show resolved Hide resolved
"Plugin \"{}\" was already added with group \"{}\"",
plugin_name, existing_from_group
),
(Some(from_group), None) => panic!(
"Plugin \"{}\" from group \"{}\" was already added",
plugin_name, from_group
),
(Some(from_group), Some(existing_from_group)) => panic!(
"Plugin \"{}\" from group \"{}\" was already added with group \"{}\"",
plugin_name, from_group, existing_from_group
),
};
}
}

/// Adds a group of [`Plugin`]s.
///
/// [`Plugin`]s can be grouped into a set by using a [`PluginGroup`].
Expand All @@ -794,9 +832,15 @@ impl App {
/// .add_plugins(MinimalPlugins);
/// ```
pub fn add_plugins<T: PluginGroup>(&mut self, mut group: T) -> &mut Self {
if self.plugins.insert(TypeId::of::<T>(), None).is_some() {
panic!(
"Plugin Group \"{}\" was already added",
std::any::type_name::<T>()
);
}
let mut plugin_group_builder = PluginGroupBuilder::default();
group.build(&mut plugin_group_builder);
plugin_group_builder.finish(self);
plugin_group_builder.finish::<T>(self);
self
}

Expand Down Expand Up @@ -835,10 +879,17 @@ impl App {
T: PluginGroup,
F: FnOnce(&mut PluginGroupBuilder) -> &mut PluginGroupBuilder,
{
if self.plugins.insert(TypeId::of::<T>(), None).is_some() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

panic!(
"Plugin Group \"{}\" was already added",
std::any::type_name::<T>()
);
}

let mut plugin_group_builder = PluginGroupBuilder::default();
group.build(&mut plugin_group_builder);
func(&mut plugin_group_builder);
plugin_group_builder.finish(self);
plugin_group_builder.finish::<T>(self);
self
}

Expand Down Expand Up @@ -928,3 +979,49 @@ fn run_once(mut app: App) {
/// frame is over.
#[derive(Debug, Clone, Default)]
pub struct AppExit;

#[cfg(test)]
mod tests {
use crate::{App, Plugin};

struct PluginA;
impl Plugin for PluginA {
fn build(&self, _app: &mut crate::App) {}
}
struct PluginB;
impl Plugin for PluginB {
fn build(&self, _app: &mut crate::App) {}
}
struct PluginC<T>(T);
impl<T: Send + Sync + 'static> Plugin for PluginC<T> {
fn build(&self, _app: &mut crate::App) {}
}
struct PluginD;
impl Plugin for PluginD {
fn build(&self, _app: &mut crate::App) {}
fn is_unique(&self) -> bool {
false
}
}

#[test]
fn can_add_two_plugins() {
App::new().add_plugin(PluginA).add_plugin(PluginB);
}

#[test]
#[should_panic]
fn cant_add_twice_the_same_plugin() {
App::new().add_plugin(PluginA).add_plugin(PluginA);
}

#[test]
fn can_add_twice_the_same_plugin_with_different_type_param() {
App::new().add_plugin(PluginC(0)).add_plugin(PluginC(true));
}

#[test]
fn can_add_twice_the_same_plugin_not_unique() {
App::new().add_plugin(PluginD).add_plugin(PluginD);
}
}
10 changes: 9 additions & 1 deletion crates/bevy_app/src/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,22 @@ use std::any::Any;
/// A collection of Bevy app logic and configuration.
///
/// Plugins configure an [`App`]. When an [`App`] registers a plugin,
/// the plugin's [`Plugin::build`] function is run.
/// the plugin's [`Plugin::build`] function is run. By default, a plugin
/// can only be added once to an [`App`]. If the plugin may need to be
/// added twice or more, the function [`is_unique`](Plugin::is_unique)
/// should be overriden to return `false`.
pub trait Plugin: Any + Send + Sync {
/// Configures the [`App`] to which this plugin is added.
fn build(&self, app: &mut App);
/// Configures a name for the [`Plugin`] which is primarily used for debugging.
fn name(&self) -> &str {
std::any::type_name::<Self>()
}
/// If the plugin can be meaningfully instantiated several times in an [`App`](crate::App),
/// override this method to return `false`.
fn is_unique(&self) -> bool {
true
}
}

/// A type representing an unsafe function that returns a mutable pointer to a [`Plugin`].
Expand Down
17 changes: 14 additions & 3 deletions crates/bevy_app/src/plugin_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use bevy_utils::{tracing::debug, HashMap};
use std::any::TypeId;

/// Combines multiple [`Plugin`]s into a single unit.
pub trait PluginGroup {
pub trait PluginGroup: 'static {
/// Configures the [`Plugin`]s that are to be added.
fn build(&mut self, group: &mut PluginGroupBuilder);
}
Expand Down Expand Up @@ -110,11 +110,22 @@ impl PluginGroupBuilder {
}

/// Consumes the [`PluginGroupBuilder`] and [builds](Plugin::build) the contained [`Plugin`]s.
pub fn finish(self, app: &mut App) {
pub fn finish<T: PluginGroup>(self, app: &mut App) {
for ty in self.order.iter() {
if let Some(entry) = self.plugins.get(ty) {
if entry.enabled {
debug!("added plugin: {}", entry.plugin.name());
debug!(
"added plugin {} from group {}",
entry.plugin.name(),
std::any::type_name::<T>()
);
if entry.plugin.is_unique() {
app.register_plugin(
ty,
entry.plugin.name(),
Some(std::any::type_name::<T>()),
);
}
entry.plugin.build(app);
}
}
Expand Down