Plugin dependency graph v2

[NthTensor]

This PR builds heavily off the initial work done by #7839. Thanks @maniwani <3.

Objective

• Implement Plugin Dependencies #69.
• Take a small step towards Order independence of App construction #1255.
• Provide a better way for one plugin to spoof/substitute another than with names.
• Continue to allow multiple instances of the same plugin.
• Retain as much of the current plugin API as possible.

Solution

The TL;DR is:

• Plugins are identified by TypeId.
• Plugins have a manifest which describes their relationship to other plugins.
• Plugin builds are now differed until App::run, and are sorted according to the dependency graph.
• Plugins can no longer add other plugins to the app in Plugin::build.
• The plugin collections infrastructure is now based around a growable set of plugins.

This PR contains a few features which are too closely related for me to split into seperate PRs. I recommend reviewers start with the changes in app.rs, then move on to read PluginSet::order in plugin.rs before taking a look at the rest of plugin.rs.

Manifests

Plugins may now implement Plugin::configure to specify relationships with other plugins.

impl Plugin for MyPlugin {
    fn build(&self, app: &mut App) {
        // Normal plugin stuff...
    }

    fn configure(&self, manifest: &mut PluginManifest) {
        manifest.add_dependency::<RequiredPlugin>(true);
        manifest.add_dependency::<OptionalPlugin>(false);
        manifest.add_substitution::<ReplacedPlugin>();
    }
}

PluginManifest::add_dependency tells the app to build another plugin before this one. If the dependency is marked required, the app will panic if the target plugin is disabled or not present.

PluginManifest::add_substitution disables the target plugin, if it is present, and makes all of it's dependencies depend on this plugin instead. It can be used for debugging, for overriding default engine behavior, or for plugin aliasing. A plugin cannot be overridden by multiple other plugins at the same time.

When dealing with manifests, we pretend like all instances of a non-unique plugin are the same. A dependency on a non-unique plugin means all instances of that plugin will be built first. All instances of a plugin are passed a reference to the same PluginManfiest in Plugin::configure. The same dependency can be specified multiple times, but the app will panic if, for example, one instance depends on a given plugin and another instance substitutes for it.

PluginManifest is basically just a HashMap<TypeId, PluginRelation>, but it also stores the type names of the targets to provide friendly errors. The goal is to allow easy extensibility, and to pave the way for Plugins to move into the ECS once relations are in.

Plugin Builds

The app now defers building plugins until App::build is called in App::run. Places where App:update is called directly must now call App::build first. The plugin life-cycle functions build, ready, finish and clean are now called in an order determined by PluginSet::order which respects plugin dependencies.

I have avoided changing any other aspects of the plugin life-cycle as much as possible. I think it would be worth re-evaluating when/if this is merged.

Plugin Sets

The elephant in the room is that PluginGroup is built to accommodate implicit plugin ordering, and makes no sense when plugins can be added in any order. This issue is confounded by the App now needing all plugins to be registered before App::build is called; Plugins like TransformPlugin which used to add ValidParentCheckPlugin in Plugin::build now need to be added together. The simplest solution would be to add them to a new TrandomPluginGroup, but plugin groups cant be added to plugin groups.

To solve these problems I introduced PluginSet, a correct-by-default growable collection of plugins. PluginSet takes over the existing functionality from App::add_plugins and implements Plugins, allowing sets to be added to other sets. App has been changed to hold a PluginSet internally, and PluginGroup now adds plugins directly to a PluginSet rather than a PluginGroupBuilder. Anything that implements Plugins can be turned into a PluginSet, so it is effectively the canonical representation of that trait. PluginSet is also in-charge of maintaining manifest information, and as I mentioned earlier is what determines the ordering of plugins for the App (through PluginSet::order).

The downside is that every PluginGroup now needs to be re-written to use the new API, but the upside is that the API is a little less verbose and far more flexible.

impl PluginGroup for DefaultPlugins {
    fn build(self, mut set: PluginSet) -> PluginSet {
        set = set.add_plugins((
            bevy_log::LogPlugin::default(),
            bevy_core::TaskPoolPlugin::default(),
            bevy_core::TypeRegistrationPlugin,
            bevy_core::FrameCountPlugin,
            bevy_time::TimePlugin,
            bevy_transform::TransformPluginGroup, // Adding a group to a group
            bevy_hierarchy::HierarchyPlugin,
            bevy_diagnostic::DiagnosticsPlugin,
            bevy_input::InputPlugin,
            bevy_window::WindowPlugin::default(),
            bevy_a11y::AccessibilityPlugin,
            IgnoreAmbiguitiesPlugin,
        ));
        
        // Feature flags ommited for length...
        
        set
    }
}

---

Changelog

• Plugins are now identified by TypeId.
• Plugins now have a manifest which describes their relationship to other plugins.
• Plugins builds are now differed until App::run, and are sorted according to the dependency graph.
• Plugins can no longer add other plugins to the app in Plugin::build.
• PluginSet introduced, and used as the basis for a reworked plugin infrastructure.

Migration Guide

• Most users will not need to migrate their code.
• Tests which directly call App::update must now call App:build first.
• Plugins which exploited Plugin::name to control how they were identified will need to use subs_for instead. Plugin::name has been removed.
• Functionality dependent on adding plugins in a specific order should now be use depends_on.
• Implementations of PluginGroup must be re-written to use PluginSets, and calls to PluginGroup::build should be removed.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[MiniaczQ]

Generic plugins were discussed on discord, with the solution being to depend on specific instances.
This will be weird for depending on plugins like rapier, where the generic is used for a really niche functionality.
I guess making the parent plugin generic over the same type is a must, even if it doesn't use it.

The part about union of non-unique plugins is a bit unclear to me.
Will the Plugin trait add a union(self, other: Self) function that combines instance values as defined by the plugin creator?

[NthTensor]

Great point, and interesting question!

Generic Plugins

I'm not familiar with rapier's plugin structure. In general, this will only let you depend on a single specific concrete plugin. However, if you really want to depend on any member of a group of related plugins it may be possible to do interesting things by abusing the loose-coupling provided by subs_for.

// This is a plugin we would like to specify dependencies upon, regardless of the specific `T`.
struct MyGenericPlugin<T> {
    // Plugin fields using T ...
};

// This marker trait doesn't implement `Plugin`, but plugins can still specify it as a dependency.
struct MyPluginMarker;

impl<T> Plugin for GenericPlugin<T> {
    // Normal plugin stuff that requires T ...
    fn subs_for(&self) -> Vec<TypeId> {
        vec![TypeId::of::<MyPluginMarer>()]
    }
}

I believe this should allow a dependency on any GenericPlugin<T>. Still need to double-check this is the case, and write a test.

struct MyDependentPlugin;

impl Plugin for MyDependentPlugin {
    // Normal plugin stuff that doesn't know about T ...
    fn depends_on(&self) -> Vec<TypeId> {
        vec![TypeId::of::<MyPluginMarker>()]
    }
}

This is a consequence of letting the user specify a TypeId directly. It should be possible to prevent this if we decide it is undesirable.

Non-unique plugins

My current plan for non-unique plugins is to pretend, for the purposes of ordering and dependency resolution, that they are a single plugin. If any instance of a non-unique plugin specifies a dependency, then all instances of that plugin will be treated as having that dependency. They occupy a single node in the dependency graph, using the union across all their dependency sets, and are all built at the same time. Otherwise their behavior shouldn't change.

I hope that's a bit clearer.

[MiniaczQ]

Generic Plugins

The marker is an interesting approach, I like the aliasing aspect of it.

To clarify what I thought the proposed solution on discord was:

struct PhysicsPlugin<T> {
    // `T` is used for custom user-data, not necessary for most users
};

impl<T> Plugin for MovementPlugin<T> {
    fn depends_on(&self) -> Vec<TypeId> {
        // `T` propagated to the other plugin
        vec![TypeId::of<PhysicsPlugin<T>>()]
    }
}

Non-unique plugins

Misunderstanding on my part, the unique plugin functionality is already implemented, and the plugin instances of the same types never interact with one another.
I now understand, you meant how it works on graph edges, thanks. 🙂

[NthTensor]

That's definitely the intended approach if both plugins are generic. I will add some examples up above.

[BD103]

What's your reasoning behind using a Vec for the return value?

I feel like most dependencies will be known at compile time, so this could be a &'static [TypeId] instead.

If you wanted to be able to dynamically specify dependencies, you could take advantage of Rust 1.75 and use impl Iterator<Item = TypeId>.

[NthTensor]

Thanks for the feedback! I'm not sure dependencies can always be known at compile time. Library authors might want to add flags to their plugin struct to enable or disable different functionality, and this might require them to change their dependencies.

Worth noting that I am currently reimplementing this PR from scratch, with a cleaner commit history. The new API I am playing with is:

fn configure(&self, &mut PluginManifest) -> {
    manifest.add_dependency::<Plugin>(true); // required dependency
    manifest.add_substitution::<Plugin>();
}

PluginManifest is just a thin wrapper over HashMap<TypeId, PluginRelation>. This design new significantly improves error reporting and simplifies logic about non-unique plugins. I believe it may also ease migration when/if we decide to transition plugins into the ECS as entities w/ relations.

[BD103]

Interesting! That approach definitely seems a lot easier to use and extend. (It also decreases heap allocations, which is good!) Is your rewrite on a public branch? I'm curious to see what else you changed. :)

[NthTensor]

I appreciate your interest! I have some uncommitted tests right now, but I will publish the new approach tomorrow or monday.

I hope to get the whole thing working later this week. This older approach started from dependencies and worked backwards, but I ran into issues with non-top-level plugin builds. My new approach starts from a general PluginSet type which takes over the App::add_plugins api and serves as the canonical representation of the Plugins<M> trait:

let mut first_set = PluginSet::new();
set_ab.add_plugins(PluginA);
let mut second_set = PluginSet::new();
second_set.add_plugins((PluginGroup, (PluginB, PluginC), set_ab));

This fixes a bunch of issues in this branch, mostly at the cost of changing the PluginGroup interface. All I have left is to stitch the two together.

[afonsolage]

I didn't took a look into the implementation itself (yet), but I like the idea of having a better plugin dependency.

My Use Case

What made me pick interest in this topic is the because I'm having a difficult time by managing plugin dependency with current plugin system.

In short I need to add a custom AssetSource and thus my plugins needs to ran before the AssetPlugin, but also I have another plugin and some logging that needs to be ran after the LogPlugin, since users likely just throw app.add_plugins(DefaultPlugins) it's hard to tell users to mess with the plugin insertion order.

Plugin Dependency API

Even tho I understand the proposed API I wonder if it is possible to make it match the Systems Dependency API, so instead of having add_dependency, add_substitution, would it be better to just use something like:

app.add_plugins((
    CustomAssetPlugin.before(AssetPlugin),
    FancyPlugin.after(LogPlugin),
    ConsoleDebugPlugin.add_if(is_debug_build),
));

That way it would be more straightforward for new users to just learn one API and would be less verbose to write IMO.

Plugin Substitution

I don't think it is a good idea to allow plugins to substitute others, this could cause a conflict and be hard to debug, because you don't know when some plugin that you added will substitute another plugin that you have added.

A solution for this would be plugins to allow some Settings associated type and plugin authors may add an enable flag or some form of configuration so plugins could interact with each other and have better cooperation.

[NthTensor]

Even tho I understand the proposed API I wonder if it is possible to make it match the Systems Dependency API

This is the goal of the PluginSet api. It sorta works right now, but there are some issues I need to iron out. Honestly not sure what version this is relative to my local branch.

I don't think it is a good idea to allow plugins to substitute others, this could cause a conflict and be hard to debug, because you don't know when some plugin that you added will substitute another plugin that you have added.

The idea is that substitution is a contract. You should only substitute for a plugin if they are truly interchangeable, and provide the exact same resources for dependencies (if perhaps different behavior). This enables a ton of interesting features for modularity, like the ability to say "I need a physics plugin but I don't care which one" or "this plugin wraps and modifies bevy_core_pipeline". I would be fine with splitting it out into a S-Controversial follow-up if others feel strongly about this.

[knutsoned]

This seems like important functionality both for the internal team to be able to try out new features or their variants without a full fork, and for the ecosystem to be able to contribute with lower barriers and less work for reviewers.

It also lays the foundation for defining the internal API of Bevy as a network of contracts, and for supporting dependency injection of internal functionality across crate and feature boundaries.

Basically, the ECS seems like a big black box and there is a lot of just taking another plugin's word for it that the data that just disappeared into the aether will be handled properly. A thin layer of formality as suggested by this PR could alleviate this concern in the short term by adding a slightly more fine-grained system of expressing dependencies than features. After some iterating, it could be developed into something that resembles more of a platform with a reference implementation engine that is quite usable out of the box, versus just the engine part.

I also suspect that this is a good candidate for gradual adoption. I'd suggest to identify where this is most useful (physics springs to mind) and prototype how this actually works with that concrete example.

[NthTensor]

This PR has promise but it far too large and sweeping. I'm going to try to break the useful bits into a few smaller incremental changes.