From fdcee7c9bdbbbb134d191b0c7356a80a45227a8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= Date: Mon, 9 May 2022 13:06:22 +0000 Subject: [PATCH] fix re-adding a plugin to a plugin group (#2039) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- crates/bevy_app/src/app.rs | 6 +- crates/bevy_app/src/plugin_group.rs | 205 ++++++++++++++++++++++++---- 2 files changed, 181 insertions(+), 30 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 265d0b391e7c0..7567200a22ba6 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -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}; /// # @@ -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 /// diff --git a/crates/bevy_app/src/plugin_group.rs b/crates/bevy_app/src/plugin_group.rs index d9174a14ed1eb..75d450032aa7b 100644 --- a/crates/bevy_app/src/plugin_group.rs +++ b/crates/bevy_app/src/plugin_group.rs @@ -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. @@ -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, @@ -39,51 +40,68 @@ impl PluginGroupBuilder { } } - /// Appends a [`Plugin`] to the [`PluginGroupBuilder`]. - pub fn add(&mut self, plugin: T) -> &mut Self { - self.order.push(TypeId::of::()); - self.plugins.insert( + // Insert the new plugin as enabled, and removes its previous ordering if it was + // already present + fn upsert_plugin_state(&mut self, plugin: T, added_at_index: usize) { + if let Some(entry) = self.plugins.insert( TypeId::of::(), 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::()) + .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(&mut self, plugin: T) -> &mut Self { + let target_index = self.order.len(); + self.order.push(TypeId::of::()); + 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(&mut self, plugin: T) -> &mut Self { let target_index = self.index_of::(); self.order.insert(target_index, TypeId::of::()); - self.plugins.insert( - TypeId::of::(), - 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(&mut self, plugin: T) -> &mut Self { - let target_index = self.index_of::(); - self.order.insert(target_index + 1, TypeId::of::()); - self.plugins.insert( - TypeId::of::(), - PluginEntry { - plugin: Box::new(plugin), - enabled: true, - }, - ); + let target_index = self.index_of::() + 1; + self.order.insert(target_index, TypeId::of::()); + 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(&mut self) -> &mut Self { let mut plugin_entry = self .plugins @@ -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(&mut self) -> &mut Self { let mut plugin_entry = self .plugins @@ -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) { @@ -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::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } + + #[test] + fn add_after() { + let mut group = PluginGroupBuilder::default(); + group.add(PluginA); + group.add(PluginB); + group.add_after::(PluginC); + + assert_eq!( + group.order, + vec![ + std::any::TypeId::of::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } + + #[test] + fn add_before() { + let mut group = PluginGroupBuilder::default(); + group.add(PluginA); + group.add(PluginB); + group.add_before::(PluginC); + + assert_eq!( + group.order, + vec![ + std::any::TypeId::of::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } + + #[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::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } + + #[test] + fn readd_after() { + let mut group = PluginGroupBuilder::default(); + group.add(PluginA); + group.add(PluginB); + group.add(PluginC); + group.add_after::(PluginC); + + assert_eq!( + group.order, + vec![ + std::any::TypeId::of::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } + + #[test] + fn readd_before() { + let mut group = PluginGroupBuilder::default(); + group.add(PluginA); + group.add(PluginB); + group.add(PluginC); + group.add_before::(PluginC); + + assert_eq!( + group.order, + vec![ + std::any::TypeId::of::(), + std::any::TypeId::of::(), + std::any::TypeId::of::(), + ] + ) + } +}