Plugins own their settings. Rework PluginGroup trait. (#6336)

# Objective

Fixes #5884 #2879
Alternative to #2988 #5885 #2886

"Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems:

1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead)
2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed.

(1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem.

## Solution

Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources.

Plugins are now configured like this:

```rust
app.add_plugin(AssetPlugin {
  watch_for_changes: true,
  ..default()
})
```

PluginGroups are now configured like this:

```rust
app.add_plugins(DefaultPlugins
  .set(AssetPlugin {
    watch_for_changes: true,
    ..default()
  })
)
```

This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons:
* ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong!
* This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it).
* I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints.

Author: cart

crates/bevy_app/Cargo.toml

@@ -24,6 +24,7 @@ bevy_utils = { path = "../bevy_utils", version = "0.9.0-dev" }
 # other
 serde = { version = "1.0", features = ["derive"], optional = true }
 ron = { version = "0.8.0", optional = true }
+downcast-rs = "1.2.0"
 
 
 [target.'cfg(target_arch = "wasm32")'.dependencies]

crates/bevy_app/src/app.rs

@@ -1,4 +1,4 @@
-use crate::{CoreStage, Plugin, PluginGroup, PluginGroupBuilder, StartupSchedule, StartupStage};
+use crate::{CoreStage, Plugin, PluginGroup, StartupSchedule, StartupStage};
 pub use bevy_derive::AppLabel;
 use bevy_derive::{Deref, DerefMut};
 use bevy_ecs::{
@@ -838,7 +838,8 @@ impl App {
     /// The [`PluginGroup`]s available by default are `DefaultPlugins` and `MinimalPlugins`.
     ///
     /// 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).
+    /// before / after another plugin), call [`build()`](PluginGroup::build) on the group,
+    /// which will convert it to a [`PluginGroupBuilder`](crate::PluginGroupBuilder).
     ///
     /// ## Examples
     /// ```
@@ -847,61 +848,9 @@ impl App {
     /// App::new()
     ///     .add_plugins(MinimalPlugins);
     /// ```
-    pub fn add_plugins<T: PluginGroup>(&mut self, mut group: T) -> &mut Self {
-        let mut plugin_group_builder = PluginGroupBuilder::default();
-        group.build(&mut plugin_group_builder);
-        plugin_group_builder.finish(self);
-        self
-    }
-
-    /// Adds a group of [`Plugin`]s with an initializer method.
-    ///
-    /// 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 using a [`PluginGroupBuilder`].
-    ///
-    /// # Examples
-    ///
-    /// ```
-    /// # use bevy_app::{prelude::*, PluginGroupBuilder};
-    /// #
-    /// # // Dummies created to avoid using `bevy_internal` and `bevy_log`,
-    /// # // which pulls in too many dependencies and breaks rust-analyzer
-    /// # pub mod bevy_log {
-    /// #     use bevy_app::prelude::*;
-    /// #     #[derive(Default)]
-    /// #     pub struct LogPlugin;
-    /// #     impl Plugin for LogPlugin{
-    /// #        fn build(&self, app: &mut App) {}
-    /// #     }
-    /// # }
-    /// # struct DefaultPlugins;
-    /// # impl PluginGroup for DefaultPlugins {
-    /// #     fn build(&mut self, group: &mut PluginGroupBuilder){
-    /// #         group.add(bevy_log::LogPlugin::default());
-    /// #     }
-    /// # }
-    /// #
-    /// # struct MyOwnPlugin;
-    /// # impl Plugin for MyOwnPlugin {
-    /// #     fn build(&self, app: &mut App){;}
-    /// # }
-    /// #
-    /// App::new()
-    ///      .add_plugins_with(DefaultPlugins, |group| {
-    ///             group.add_before::<bevy_log::LogPlugin, _>(MyOwnPlugin)
-    ///         });
-    /// ```
-    pub fn add_plugins_with<T, F>(&mut self, mut group: T, func: F) -> &mut Self
-    where
-        T: PluginGroup,
-        F: FnOnce(&mut PluginGroupBuilder) -> &mut PluginGroupBuilder,
-    {
-        let mut plugin_group_builder = PluginGroupBuilder::default();
-        group.build(&mut plugin_group_builder);
-        func(&mut plugin_group_builder);
-        plugin_group_builder.finish(self);
+    pub fn add_plugins<T: PluginGroup>(&mut self, group: T) -> &mut Self {
+        let builder = group.build();
+        builder.finish(self);
         self
     }
 

crates/bevy_app/src/plugin.rs

@@ -1,11 +1,13 @@
+use downcast_rs::{impl_downcast, Downcast};
+
 use crate::App;
 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.
-pub trait Plugin: Any + Send + Sync {
+pub trait Plugin: Downcast + 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.
@@ -14,6 +16,8 @@ pub trait Plugin: Any + Send + Sync {
     }
 }
 
+impl_downcast!(Plugin);
+
 /// A type representing an unsafe function that returns a mutable pointer to a [`Plugin`].
 /// It is used for dynamically loading plugins.
 ///

crates/bevy_app/src/plugin_group.rs

@@ -3,16 +3,26 @@ use bevy_utils::{tracing::debug, tracing::warn, HashMap};
 use std::any::TypeId;
 
 /// Combines multiple [`Plugin`]s into a single unit.
-pub trait PluginGroup {
+pub trait PluginGroup: Sized {
     /// Configures the [`Plugin`]s that are to be added.
-    fn build(&mut self, group: &mut PluginGroupBuilder);
+    fn build(self) -> PluginGroupBuilder;
+    /// Sets the value of the given [`Plugin`], if it exists
+    fn set<T: Plugin>(self, plugin: T) -> PluginGroupBuilder {
+        self.build().set(plugin)
+    }
 }
 
 struct PluginEntry {
     plugin: Box<dyn Plugin>,
     enabled: bool,
 }
 
+impl PluginGroup for PluginGroupBuilder {
+    fn build(self) -> PluginGroupBuilder {
+        self
+    }
+}
+
 /// 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. [`Plugin`]s inside the group
@@ -25,7 +35,7 @@ pub struct PluginGroupBuilder {
 
 impl PluginGroupBuilder {
     /// Finds the index of a target [`Plugin`]. Panics if the target's [`TypeId`] is not found.
-    fn index_of<Target: Plugin>(&mut self) -> usize {
+    fn index_of<Target: Plugin>(&self) -> usize {
         let index = self
             .order
             .iter()
@@ -68,9 +78,27 @@ impl PluginGroupBuilder {
         }
     }
 
+    /// Sets the value of the given [`Plugin`], if it exists.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the [`Plugin`] does not exist.
+    pub fn set<T: Plugin>(mut self, plugin: T) -> Self {
+        let entry = self.plugins.get_mut(&TypeId::of::<T>()).unwrap_or_else(|| {
+            panic!(
+                "{} does not exist in this PluginGroup",
+                std::any::type_name::<T>(),
+            )
+        });
+        entry.plugin = Box::new(plugin);
+        self
+    }
+
     /// 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 {
+    // This is not confusing, clippy!
+    #[allow(clippy::should_implement_trait)]
+    pub fn add<T: Plugin>(mut self, plugin: T) -> Self {
         let target_index = self.order.len();
         self.order.push(TypeId::of::<T>());
         self.upsert_plugin_state(plugin, target_index);
@@ -80,7 +108,7 @@ impl PluginGroupBuilder {
     /// 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 {
+    pub fn add_before<Target: Plugin, T: Plugin>(mut self, plugin: T) -> Self {
         let target_index = self.index_of::<Target>();
         self.order.insert(target_index, TypeId::of::<T>());
         self.upsert_plugin_state(plugin, target_index);
@@ -90,7 +118,7 @@ impl PluginGroupBuilder {
     /// 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 {
+    pub fn add_after<Target: Plugin, T: Plugin>(mut self, plugin: T) -> Self {
         let target_index = self.index_of::<Target>() + 1;
         self.order.insert(target_index, TypeId::of::<T>());
         self.upsert_plugin_state(plugin, target_index);
@@ -102,7 +130,7 @@ impl PluginGroupBuilder {
     /// [`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. If there are no plugins
     /// of type `T` in this group, it will panic.
-    pub fn enable<T: Plugin>(&mut self) -> &mut Self {
+    pub fn enable<T: Plugin>(mut self) -> Self {
         let mut plugin_entry = self
             .plugins
             .get_mut(&TypeId::of::<T>())
@@ -116,7 +144,7 @@ impl PluginGroupBuilder {
     /// 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 {
+    pub fn disable<T: Plugin>(mut self) -> Self {
         let mut plugin_entry = self
             .plugins
             .get_mut(&TypeId::of::<T>())
@@ -152,7 +180,9 @@ impl PluginGroupBuilder {
 pub struct NoopPluginGroup;
 
 impl PluginGroup for NoopPluginGroup {
-    fn build(&mut self, _: &mut PluginGroupBuilder) {}
+    fn build(self) -> PluginGroupBuilder {
+        PluginGroupBuilder::default()
+    }
 }
 
 #[cfg(test)]
@@ -177,10 +207,10 @@ mod tests {
 
     #[test]
     fn basic_ordering() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add(PluginC);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add(PluginC);
 
         assert_eq!(
             group.order,
@@ -194,10 +224,10 @@ mod tests {
 
     #[test]
     fn add_after() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add_after::<PluginA, PluginC>(PluginC);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add_after::<PluginA, PluginC>(PluginC);
 
         assert_eq!(
             group.order,
@@ -211,10 +241,10 @@ mod tests {
 
     #[test]
     fn add_before() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add_before::<PluginB, PluginC>(PluginC);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add_before::<PluginB, PluginC>(PluginC);
 
         assert_eq!(
             group.order,
@@ -228,11 +258,11 @@ mod tests {
 
     #[test]
     fn readd() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add(PluginC);
-        group.add(PluginB);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add(PluginC)
+            .add(PluginB);
 
         assert_eq!(
             group.order,
@@ -246,11 +276,11 @@ mod tests {
 
     #[test]
     fn readd_after() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add(PluginC);
-        group.add_after::<PluginA, PluginC>(PluginC);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add(PluginC)
+            .add_after::<PluginA, PluginC>(PluginC);
 
         assert_eq!(
             group.order,
@@ -264,11 +294,11 @@ mod tests {
 
     #[test]
     fn readd_before() {
-        let mut group = PluginGroupBuilder::default();
-        group.add(PluginA);
-        group.add(PluginB);
-        group.add(PluginC);
-        group.add_before::<PluginB, PluginC>(PluginC);
+        let group = PluginGroupBuilder::default()
+            .add(PluginA)
+            .add(PluginB)
+            .add(PluginC)
+            .add_before::<PluginB, PluginC>(PluginC);
 
         assert_eq!(
             group.order,

crates/bevy_asset/src/asset_server.rs

@@ -83,8 +83,8 @@ pub struct AssetServerInternal {
 /// # use bevy_asset::*;
 /// # use bevy_app::*;
 /// # let mut app = App::new();
-/// // AssetServerSettings must be inserted before adding the AssetPlugin or DefaultPlugins.
-/// app.insert_resource(AssetServerSettings {
+/// // The asset plugin can be configured to watch for asset changes.
+/// app.add_plugin(AssetPlugin {
 ///     watch_for_changes: true,
 ///     ..Default::default()
 /// });
@@ -293,7 +293,7 @@ impl AssetServer {
     /// `"CARGO_MANIFEST_DIR"` is automatically set to the root folder of your crate (workspace).
     ///
     /// The name of the asset folder is set inside the
-    /// [`AssetServerSettings`](crate::AssetServerSettings) resource. The default name is
+    /// [`AssetPlugin`](crate::AssetPlugin). The default name is
     /// `"assets"`.
     ///
     /// The asset is loaded asynchronously, and will generally not be available by the time

crates/bevy_asset/src/assets.rs

@@ -430,7 +430,7 @@ mod tests {
         struct MyAsset;
         let mut app = App::new();
         app.add_plugin(bevy_core::CorePlugin)
-            .add_plugin(crate::AssetPlugin);
+            .add_plugin(crate::AssetPlugin::default());
         app.add_asset::<MyAsset>();
         let mut assets_before = app.world.resource_mut::<Assets<MyAsset>>();
         let handle = assets_before.add(MyAsset);

crates/bevy_asset/src/debug_asset_server.rs

@@ -16,8 +16,7 @@ use std::{
 };
 
 use crate::{
-    Asset, AssetEvent, AssetPlugin, AssetServer, AssetServerSettings, Assets, FileAssetIo, Handle,
-    HandleUntyped,
+    Asset, AssetEvent, AssetPlugin, AssetServer, Assets, FileAssetIo, Handle, HandleUntyped,
 };
 
 /// A helper [`App`] used for hot reloading internal assets, which are compiled-in to Bevy plugins.
@@ -75,12 +74,10 @@ impl Plugin for DebugAssetServerPlugin {
                 .build()
         });
         let mut debug_asset_app = App::new();
-        debug_asset_app
-            .insert_resource(AssetServerSettings {
-                asset_folder: "crates".to_string(),
-                watch_for_changes: true,
-            })
-            .add_plugin(AssetPlugin);
+        debug_asset_app.add_plugin(AssetPlugin {
+            asset_folder: "crates".to_string(),
+            watch_for_changes: true,
+        });
         app.insert_non_send_resource(DebugAssetApp(debug_asset_app));
         app.add_system(run_debug_asset_app);
     }