Skip to content

Commit

Permalink
fix re-adding a plugin to a plugin group (bevyengine#2039)
Browse files Browse the repository at this point in the history 
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
  • Loading branch information
@mockersf @exjam
2 people authored and exjam committed May 22, 2022
1 parent 8872af9 commit fdcee7c
Show file tree
Hide file tree
Showing 2 changed files with 181 additions and 30 deletions.
6 changes: 4 additions & 2 deletions crates/bevy_app/src/app.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,8 +779,10 @@ impl App {
/// There are built-in [`PluginGroup`]s that provide core engine functionality.
/// The [`PluginGroup`]s available by default are `DefaultPlugins` and `MinimalPlugins`.
///
/// # Examples
/// To customize the plugins in the group (reorder, disable a plugin, add a new plugin
/// before / after another plugin), see [`add_plugins_with`](Self::add_plugins_with).
///
/// ## Examples
/// ```
/// # use bevy_app::{prelude::*, PluginGroupBuilder};
/// #
Expand All @@ -805,7 +807,7 @@ impl App {
/// Can be used to add a group of [`Plugin`]s, where the group is modified
/// before insertion into a Bevy application. For example, you can add
/// additional [`Plugin`]s at a specific place in the [`PluginGroup`], or deactivate
/// specific [`Plugin`]s while keeping the rest.
/// specific [`Plugin`]s while keeping the rest using a [`PluginGroupBuilder`].
///
/// # Examples
///
Expand Down
205 changes: 177 additions & 28 deletions crates/bevy_app/src/plugin_group.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{App, Plugin};
use bevy_utils::{tracing::debug, HashMap};
use bevy_utils::{tracing::debug, tracing::warn, HashMap};
use std::any::TypeId;

/// Combines multiple [`Plugin`]s into a single unit.
Expand All @@ -15,7 +15,8 @@ struct PluginEntry {

/// Facilitates the creation and configuration of a [`PluginGroup`].
/// Provides a build ordering to ensure that [`Plugin`]s which produce/require a [`Resource`](bevy_ecs::system::Resource)
/// are built before/after dependent/depending [`Plugin`]s.
/// are built before/after dependent/depending [`Plugin`]s. [`Plugin`]s inside the group
/// can be disabled, enabled or reordered.
#[derive(Default)]
pub struct PluginGroupBuilder {
plugins: HashMap<TypeId, PluginEntry>,
Expand All @@ -39,51 +40,68 @@ impl PluginGroupBuilder {
}
}

/// Appends a [`Plugin`] to the [`PluginGroupBuilder`].
pub fn add<T: Plugin>(&mut self, plugin: T) -> &mut Self {
self.order.push(TypeId::of::<T>());
self.plugins.insert(
// Insert the new plugin as enabled, and removes its previous ordering if it was
// already present
fn upsert_plugin_state<T: Plugin>(&mut self, plugin: T, added_at_index: usize) {
if let Some(entry) = self.plugins.insert(
TypeId::of::<T>(),
PluginEntry {
plugin: Box::new(plugin),
enabled: true,
},
);
) {
if entry.enabled {
warn!(
"You are replacing plugin '{}' that was not disabled.",
entry.plugin.name()
);
}
if let Some(to_remove) = self
.order
.iter()
.enumerate()
.find(|(i, ty)| *i != added_at_index && **ty == TypeId::of::<T>())
.map(|(i, _)| i)
{
self.order.remove(to_remove);
}
}
}

/// Adds the plugin [`Plugin`] at the end of this [`PluginGroupBuilder`]. If the plugin was
/// already in the group, it is removed from its previous place.
pub fn add<T: Plugin>(&mut self, plugin: T) -> &mut Self {
let target_index = self.order.len();
self.order.push(TypeId::of::<T>());
self.upsert_plugin_state(plugin, target_index);
self
}

/// Configures a [`Plugin`] to be built before another plugin.
/// Adds a [`Plugin`] in this [`PluginGroupBuilder`] before the plugin of type `Target`.
/// If the plugin was already the group, it is removed from its previous place. There must
/// be a plugin of type `Target` in the group or it will panic.
pub fn add_before<Target: Plugin, T: Plugin>(&mut self, plugin: T) -> &mut Self {
let target_index = self.index_of::<Target>();
self.order.insert(target_index, TypeId::of::<T>());
self.plugins.insert(
TypeId::of::<T>(),
PluginEntry {
plugin: Box::new(plugin),
enabled: true,
},
);
self.upsert_plugin_state(plugin, target_index);
self
}

/// Configures a [`Plugin`] to be built after another plugin.
/// Adds a [`Plugin`] in this [`PluginGroupBuilder`] after the plugin of type `Target`.
/// If the plugin was already the group, it is removed from its previous place. There must
/// be a plugin of type `Target` in the group or it will panic.
pub fn add_after<Target: Plugin, T: Plugin>(&mut self, plugin: T) -> &mut Self {
let target_index = self.index_of::<Target>();
self.order.insert(target_index + 1, TypeId::of::<T>());
self.plugins.insert(
TypeId::of::<T>(),
PluginEntry {
plugin: Box::new(plugin),
enabled: true,
},
);
let target_index = self.index_of::<Target>() + 1;
self.order.insert(target_index, TypeId::of::<T>());
self.upsert_plugin_state(plugin, target_index);
self
}

/// Enables a [`Plugin`].
///
/// [`Plugin`]s within a [`PluginGroup`] are enabled by default. This function is used to
/// opt back in to a [`Plugin`] after [disabling](Self::disable) it.
/// opt back in to a [`Plugin`] after [disabling](Self::disable) it. If there are no plugins
/// of type `T` in this group, it will panic.
pub fn enable<T: Plugin>(&mut self) -> &mut Self {
let mut plugin_entry = self
.plugins
Expand All @@ -93,7 +111,11 @@ impl PluginGroupBuilder {
self
}

/// Disables a [`Plugin`], preventing it from being added to the [`App`] with the rest of the [`PluginGroup`].
/// Disables a [`Plugin`], preventing it from being added to the [`App`] with the rest of the
/// [`PluginGroup`]. The disabled [`Plugin`] keeps its place in the [`PluginGroup`], so it can
/// still be used for ordering with [`add_before`](Self::add_before) or
/// [`add_after`](Self::add_after), or it can be [re-enabled](Self::enable). If there are no
/// plugins of type `T` in this group, it will panic.
pub fn disable<T: Plugin>(&mut self) -> &mut Self {
let mut plugin_entry = self
.plugins
Expand All @@ -103,7 +125,8 @@ impl PluginGroupBuilder {
self
}

/// Consumes the [`PluginGroupBuilder`] and [builds](Plugin::build) the contained [`Plugin`]s.
/// Consumes the [`PluginGroupBuilder`] and [builds](Plugin::build) the contained [`Plugin`]s
/// in the order specified.
pub fn finish(self, app: &mut App) {
for ty in self.order.iter() {
if let Some(entry) = self.plugins.get(ty) {
Expand All @@ -115,3 +138,129 @@ impl PluginGroupBuilder {
}
}
}

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

struct PluginA;
impl Plugin for PluginA {
fn build(&self, _: &mut App) {}
}

struct PluginB;
impl Plugin for PluginB {
fn build(&self, _: &mut App) {}
}

struct PluginC;
impl Plugin for PluginC {
fn build(&self, _: &mut App) {}
}

#[test]
fn basic_ordering() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add(PluginC);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginB>(),
std::any::TypeId::of::<PluginC>(),
]
)
}

#[test]
fn add_after() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add_after::<PluginA, PluginC>(PluginC);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginC>(),
std::any::TypeId::of::<PluginB>(),
]
)
}

#[test]
fn add_before() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add_before::<PluginB, PluginC>(PluginC);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginC>(),
std::any::TypeId::of::<PluginB>(),
]
)
}

#[test]
fn readd() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add(PluginC);
group.add(PluginB);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginC>(),
std::any::TypeId::of::<PluginB>(),
]
)
}

#[test]
fn readd_after() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add(PluginC);
group.add_after::<PluginA, PluginC>(PluginC);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginC>(),
std::any::TypeId::of::<PluginB>(),
]
)
}

#[test]
fn readd_before() {
let mut group = PluginGroupBuilder::default();
group.add(PluginA);
group.add(PluginB);
group.add(PluginC);
group.add_before::<PluginB, PluginC>(PluginC);

assert_eq!(
group.order,
vec![
std::any::TypeId::of::<PluginA>(),
std::any::TypeId::of::<PluginC>(),
std::any::TypeId::of::<PluginB>(),
]
)
}
}

0 comments on commit fdcee7c

Please sign in to comment.