fix: added plugin registry

[pietrosophya]

Objective

Rewrite the plugin handling for 2 main reasons:

• introduce a new init state, in which plugins can be initialized (e.g. linked), before being built
• build plugins inside the event loop

This is a requirement to allow winit to create windows before plugins are built, which is needed by #13366 (which will unbreak Android on the latest Rust version).

TODO:

• write PR description
• write rust docs
• check all examples
• check all platforms

Testing

• Did you test these changes? If so, how?
• Are there any parts that need more testing?
• How can other people (reviewers) test your changes? Is there anything specific they need to know?
• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

---

Alternatives to be defined

• app.update() requires app.update_and_clean_plugins() to run, or some added plugins won't be run correctly (specifically, they won't run the build step, but only the init one). One alternative would be to change the build step in many plugins as init, but I don't have a strong opinion. I think the pre-requisite on app.update() is valid and can be enforced. Another option could be to initialize plugins inside app.update() if they are not initialized.

• the initial plugin step is now init instead of build: I choose this instead of keeping build as the initial one only for naming reasons (I found it hard to list 5 different step names). Another option, that enforces a migration, could be not to use build at all.

• add_plugins should be called only inside the init step: this could enforce consistency and ease the creation of a dependency graph. Ideally with an easier API to return the list of sub plugins.

• sub apps should be created only before the finalize phase, or even better in a well-defined plugin step, to enforce validation.

• accessing sub_apps should be done only inside the finalize phase.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

• What changed as a result of this PR?
• If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings
• Stick to one or two sentences. If more detail is needed for a particular change, consider adding it to the "Solution" section
  • If you can't summarize the work, your change may be unreasonably large / unrelated. Consider splitting your PR to make it easier to review and merge!

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

• If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
• Simply adding new functionality is not a breaking change.
• Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

[pietrosophya]

I'll try to add a full example, that maybe explains also what each state purpose is. I left this part out until there's a general agreement for the PR, if that makes sense.

[pietrosophya]

This is one of those cases that should really not happen, or it would be a bug in the plugin registry management. Would still an option be a better choice?

[pietrosophya]

I think we should not call build here.

[pietrosophya]

3. require_subapps feels a bit tacked on and overly specific. Can we integrate that into one of the ready_for_x methods instead?

The main idea here is that we can add plugins and sub plugins, but some of these depend on sub apps, and they might not be added by other plugins.

Generally speaking, require_sub_apps asks for optional prerequisites, without which the plugin will skip the finalize step, while ready_X asks for mandatory prerequisites, without which the full plugin configuration will hang (this is another part I don't like, but I couldn't find a better lifecycle yet).

Pretty strongly in favor of the general direction of these changes, and this is needed for the 0.14 release due to the need to upgrade Winit versions and thus unbreak Android.

There's a fair bit of cleanup to do before this is mergeable:

1. Docs need to be expanded on, especially around the plugin lifecycle. I can help write them, as long as you can explain the model to me somehow :)

As in the previous comments, I can tackle some of these docs (but I'd love some help 😊 ). The general idea is:

• the app starts, and the registry is idle
• plugins are added, and initialized immediately => registry is init
• after all plugins are added, the app update_and_clean_plugins (either directly, or through small steps, one cycle at a time) => registry is done

Many constraints could be enforced, like:

• app_plugins should be called only inside init
• update cannot be called until plugins are cleaned
• sub apps can be added in certain steps only (before finalize/require_sub_apps)
• sub apps can be accessed in finalize only

but all this needs extra brain activity and could be postponed.

2. Extraneous changes (trivial helper methods on App / SubApp for example) should be reverted to make this easier to review.

I added these only to simplify calls inside ready_X... I can create another PR (sort of pre-pre-winit 0.30 😄 ) but it should be trivial enough, if that makes sense.

3. I like the finish -> finalize rename, but we can make the migration easier with a deprecation.

see below

4. I think we might want to deprecate and rename Plugin::build in favor of something clearer and as a prompt to encourage users to reconsider their plugin's lifecycle. Still chewing on names though.

I'm not sure 100% about this one, not for finish/finalize, but for init/build. To make things clearer, I think we should:

• make build call init internally
• deprecate build
• add an extra step in place of build (setup?)
• make finish call finalize internally
• deprecate finish

[pietrosophya]

Lots to talk about :) General impressions on the architecture:

1. I really like the explicit plugin lifecycle, and your changes to allow plugins to operate before the event loop starts make sense to me.

There are at least these parts that I'd like to solve, but not sure how for all:

• simplify ready methods => I don't like that I need to check the availability of a sub app if it's declared as required
• avoid exposing update and cleanup in the Plugin trait (these methods shouldn't be overwritten) => maybe free functions?
• ready_X can check for methods
• plugin dependency cycles (A adds B that adds A) => this should be straightforward now, but is it the same as the plugin duplication?

2. I'm strongly in favor of the plugin registry. I've wanted one of these for years to solve plugin ordering dependencies (Plugin Dependencies #69) and app order independence (Order independence of App construction #1255). We should consider making this read-only public for debugging purposes, probably in a future PR.

Easily doable, I'll fix this.

[alice-i-cecile]

plugin dependency cycles (A adds B that adds A) => this should be straightforward now, but is it the same as the plugin duplication?

Definitely leave this for future work that solves #69.

avoid exposing update and cleanup in the Plugin trait (these methods shouldn't be overwritten) => maybe free functions?

Yeah, just private free functions is fine.

simplify ready methods => I don't like that I need to check the availability of a sub app if it's declared as required

Yeah, this needs more brain power.

I'm not sure 100% about this one, not for finish/finalize, but for init/build. To make things clearer, I think we should:

• make build call init internally
• deprecate build
• add an extra step in place of build (setup?)
• make finish call finalize internally
• deprecate finish

Good, I like that design! And yeah, now that I think about it most of the current plugin building should actually be done before the event loop starts. setup is a good name too.

[alice-i-cecile]

%cWARN%c crates/bevy_gizmos/src/lib.rs:151%c bevy_sprite feature is enabled but bevy_sprite::SpritePlugin was not detected. Are you sure you loaded GizmoPlugin after SpritePlugin? color: orange; background: #444 color: gray; font-style: italic color: inherit
%cWARN%c crates/bevy_gizmos/src/lib.rs:157%c bevy_pbr feature is enabled but bevy_pbr::PbrPlugin was not detected. Are you sure you loaded GizmoPlugin after PbrPlugin? color: orange; background: #444 color: gray; font-style: italic color: inherit

Desktop and mobile builds look fine here, but the web examples are failing with this error.

[alice-i-cecile]

I prefer going forward with the more minimal fix in Sophya#5 for 0.14 to reduce risk and rush, so I'm taking this out of the milestone. I still want to pursue this though, but it can have more time to bake.

[Brezak]

How do you handle non unique plugins? You don't seem to handle the case where 2 plugins have the same name but different states.

[pietrosophya]

I pushed a change and some new tests to show the behavior.

[bas-ie]

Adding the fancy new S-Needs-Help here as @pietrosophya indicated above that some help with docs would be nice to get this one across the line.