From 82c031090b1742662f752227d47224f6dc7a6b58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Thu, 29 Apr 2021 00:21:54 +0200 Subject: [PATCH 1/4] fix bug, add test, add doc --- crates/bevy_app/src/plugin_group.rs | 231 ++++++++++++++++++++++++---- 1 file changed, 201 insertions(+), 30 deletions(-) diff --git a/crates/bevy_app/src/plugin_group.rs b/crates/bevy_app/src/plugin_group.rs index d9174a14ed1eb..7295d2fdcdabc 100644 --- a/crates/bevy_app/src/plugin_group.rs +++ b/crates/bevy_app/src/plugin_group.rs @@ -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,90 @@ impl PluginGroupBuilder { } } - /// Appends a [`Plugin`] to the [`PluginGroupBuilder`]. + // Removes a previous ordering of a plugin that has just been added at `added_at` index + fn remove_when_adding(&mut self, added_at: usize) { + if let Some(to_remove) = self + .order + .iter() + .enumerate() + .find(|(i, ty)| *i != added_at && **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.plugins.insert( - TypeId::of::(), - PluginEntry { - plugin: Box::new(plugin), - enabled: true, - }, - ); + if self + .plugins + .insert( + TypeId::of::(), + PluginEntry { + plugin: Box::new(plugin), + enabled: true, + }, + ) + .is_some() + { + self.remove_when_adding::(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, - }, - ); + if self + .plugins + .insert( + TypeId::of::(), + PluginEntry { + plugin: Box::new(plugin), + enabled: true, + }, + ) + .is_some() + { + self.remove_when_adding::(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::()); + if self + .plugins + .insert( + TypeId::of::(), + PluginEntry { + plugin: Box::new(plugin), + enabled: true, + }, + ) + .is_some() + { + self.remove_when_adding::(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 +133,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 +147,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 +160,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::(), + ] + ) + } +} From c058d6e10915778fe8a3058a3bf73d20c181bca6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Thu, 29 Apr 2021 21:40:12 +0200 Subject: [PATCH 2/4] add warn log when replacing a not-disabled plugin --- crates/bevy_app/src/plugin_group.rs | 74 ++++++++++++++++------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/crates/bevy_app/src/plugin_group.rs b/crates/bevy_app/src/plugin_group.rs index 7295d2fdcdabc..b442275f993eb 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. @@ -58,17 +58,19 @@ impl PluginGroupBuilder { pub fn add(&mut self, plugin: T) -> &mut Self { let target_index = self.order.len(); self.order.push(TypeId::of::()); - if self - .plugins - .insert( - TypeId::of::(), - PluginEntry { - plugin: Box::new(plugin), - enabled: true, - }, - ) - .is_some() - { + 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() + ); + } self.remove_when_adding::(target_index); } @@ -81,17 +83,19 @@ impl PluginGroupBuilder { pub fn add_before(&mut self, plugin: T) -> &mut Self { let target_index = self.index_of::(); self.order.insert(target_index, TypeId::of::()); - if self - .plugins - .insert( - TypeId::of::(), - PluginEntry { - plugin: Box::new(plugin), - enabled: true, - }, - ) - .is_some() - { + 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() + ); + } self.remove_when_adding::(target_index); } self @@ -103,17 +107,19 @@ impl PluginGroupBuilder { pub fn add_after(&mut self, plugin: T) -> &mut Self { let target_index = self.index_of::() + 1; self.order.insert(target_index, TypeId::of::()); - if self - .plugins - .insert( - TypeId::of::(), - PluginEntry { - plugin: Box::new(plugin), - enabled: true, - }, - ) - .is_some() - { + 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() + ); + } self.remove_when_adding::(target_index); } self From bfde7cce2d1e0a92edda21e0a2e6167d4095b19e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Sat, 1 May 2021 00:01:49 +0200 Subject: [PATCH 3/4] factor shared part Co-Authored-By: Niklas Eicker --- crates/bevy_app/src/plugin_group.rs | 70 +++++++++-------------------- 1 file changed, 21 insertions(+), 49 deletions(-) diff --git a/crates/bevy_app/src/plugin_group.rs b/crates/bevy_app/src/plugin_group.rs index b442275f993eb..75d450032aa7b 100644 --- a/crates/bevy_app/src/plugin_group.rs +++ b/crates/bevy_app/src/plugin_group.rs @@ -40,24 +40,9 @@ impl PluginGroupBuilder { } } - // Removes a previous ordering of a plugin that has just been added at `added_at` index - fn remove_when_adding(&mut self, added_at: usize) { - if let Some(to_remove) = self - .order - .iter() - .enumerate() - .find(|(i, ty)| *i != added_at && **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::()); + // 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 { @@ -71,9 +56,24 @@ impl PluginGroupBuilder { entry.plugin.name() ); } - self.remove_when_adding::(target_index); + 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 } @@ -83,21 +83,7 @@ impl PluginGroupBuilder { pub fn add_before(&mut self, plugin: T) -> &mut Self { let target_index = self.index_of::(); self.order.insert(target_index, TypeId::of::()); - 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() - ); - } - self.remove_when_adding::(target_index); - } + self.upsert_plugin_state(plugin, target_index); self } @@ -107,21 +93,7 @@ impl PluginGroupBuilder { pub fn add_after(&mut self, plugin: T) -> &mut Self { let target_index = self.index_of::() + 1; self.order.insert(target_index, TypeId::of::()); - 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() - ); - } - self.remove_when_adding::(target_index); - } + self.upsert_plugin_state(plugin, target_index); self } From ebe230e95ae3d6651c2a8f504ff8b86a8342293b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= <8672791+mockersf@users.noreply.github.com> Date: Thu, 30 Dec 2021 23:07:56 +0100 Subject: [PATCH 4/4] add links in doc --- crates/bevy_app/src/app.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 ///