fix re-adding a plugin to a plugin group (#2039)

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

@@ -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
     ///

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<TypeId, PluginEntry>,
@@ -39,51 +40,68 @@ impl PluginGroupBuilder {
         }
     }
 
-    /// Appends a [`Plugin`] to the [`PluginGroupBuilder`].
+    // Insert the new plugin as enabled, and removes its previous ordering if it was
-    pub fn add<T: Plugin>(&mut self, plugin: T) -> &mut Self {
+    // already present
-        self.order.push(TypeId::of::<T>());
+    fn upsert_plugin_state<T: Plugin>(&mut self, plugin: T, added_at_index: usize) {
-        self.plugins.insert(
+        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(
+        self.upsert_plugin_state(plugin, target_index);
-            TypeId::of::<T>(),
-            PluginEntry {
-                plugin: Box::new(plugin),
-                enabled: true,
-            },
-        );
         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>();
+        let target_index = self.index_of::<Target>() + 1;
-        self.order.insert(target_index + 1, TypeId::of::<T>());
+        self.order.insert(target_index, TypeId::of::<T>());
-        self.plugins.insert(
+        self.upsert_plugin_state(plugin, target_index);
-            TypeId::of::<T>(),
-            PluginEntry {
-                plugin: Box::new(plugin),
-                enabled: true,
-            },
-        );
         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
@@ -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
@@ -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::<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>(),
+            ]
+        )
+    }
+}