Remove bevy_dynamic_plugin

[bjorn3]

It is hard to use the current dynamic plugin system. It doesn't allow
hot code swapping as several OSes don't allow unloading dylibs. It
requires bevy itself to be used as dylib and it requires all other
crates shared between the main game and the plugin to be dylibs.

In the future we may get better plugin support. Whether based on wasm,
or something else. Until then, remove the current dynamic plugin to
avoid making any promises about the current ability to use dynamic
plugins.

If anyone wants to keep using the current dynamic plugin support despite
all it's issues, they can copy the less than 100 lines of code this PR
deletes.

[bjorn3]

I didn't find any current users of this functionality on github.

[DJMcNab]

I've long since wanted to tear this out.

[mockersf]

it's also mentioned in

bevy/docs/cargo_features.md

Line 22 in 9a7852d

|bevy_dynamic_plugin|Plugin for dynamic loading (using [libloading](https://crates.io/crates/libloading)).|

[bjorn3]

Fixed

[cart]

Yeah I'm open to removing this for now as we don't currently have significant motivating use cases.
I think there might be a place for this style of dynamic loading in editor experiences though (in lieu of something like wasm plugins ... which will take some serious engineering effort). It would allow people to compile the editor exactly once on their machine, then compile and load editor plugins at runtime (without recompiling the editor or the plugin). That seems pretty valuable to me.

[cart]

(of course unloading plugins should require an editor restart for safety)

[cart]

And if we agree that this provides value for a specific set of scenarios that we plan to encounter in the near future, is there a good reason to remove it in the interim? This isn't exactly a maintenance burden. Why not enable this use case for people that might encounter the need?

[alice-i-cecile]

is there a good reason to remove it in the interim?

The code is unsafe and unhelpful for users (but in an attractive way).

It also limits our design thinking, where there's an instinct to build on top of an abstraction we already have, rather than thinking things through from the start.

Why not enable this use case for people that might encounter the need?

Since I joined the project back in 0.2, I have never encountered a user who was using this.

If anyone wants to keep using the current dynamic plugin support despite
all it's issues, they can copy the less than 100 lines of code this PR
deletes.

[cart]

The code is unsafe and unhelpful for users (but in an attractive way).

I would argue that this is helpful, for the exact scenario mentioned above (loading features compiled separately from each other at the start up of a pre-compiled app). That isn't particularly niche. In addition to making editor experiences better for minimal additional effort, this would also enable things like "loading a dlc downloaded from the internet / distributed separately from the main game". I consider that to be very attractive and very helpful.

It also limits our design thinking, where there's an instinct to build on top of an abstraction we already have, rather than thinking things through from the start.

This is the result of a reasonable amount of design thinking. It is currently our best (and only) solution to the problem that doesn't require massive internal reworks and/or compromised UX. We are still free to design new things and I don't think keeping this in meaningfully limits that conversation. And I doubt we will see anything better that doesn't:

1. Re-engineer apis to use lowest common denominator C-FII
2. Add a scripting compatibility layer (almost certainly C-FFI)
3. Push the onus on users to serialize and deserialize all run time state (see @bjorn3's work in Support reloading games while preserving windows and other resources #901)

None of these are likely to happen any time soon.

[alice-i-cecile]

Alright, I'm relatively convinced: I'm in favor of leaving this in for now; I'll chew on how we could better document and expose this API.

Thanks for providing context on the design; that was very much missing for the rest of us I think.

[bjorn3]

I agree that it would be nice to have dynamic loading in the future, but the current version is missing safety guards like checking that the dynamic feature of bevy is used and that the bevy version matches and is compiled with the same rustc version. In addition I think the api exposed to create plugins is not the right api. So for the time being I think we shouldn't pretend that we have builtin support when it is only half implemented.

[cart]

I agree that the safety checks you mentioned are important. Checking for the dynamic feature seems like a relatively straightforward iterative change (export a const in the top level bevy crate that is true when dynamic and false otherwise. we could then throw that in a default function impl for a DynamicPlugin trait, also exported from the top level bevy crate). Unless I'm missing something, checking the rustc version would be a bit more involved, as it would require incorporating build scripts to store the environment / compiler info in a rust file, which could then be built into the binary.

But every solution I can think of atm would still require "trusting" the user to implement the dynamic trait apis correctly for the crate / use the safety tools we provide them. This type of dynamic loading seems like it will always have a level of unsafety to it. It feels like the best we can do is provide as many safety nets as possible / push users in the right direction.

I personally think these could be iterative improvements to what we have now, given that the fundamental approach used wouldn't be changing. So my current take is this: if somebody is going to pick up the ball now to tackle implementing a safer version of dynamic loading (that doesn't involve massive Bevy rearchitectures or introducing massive user-facing complexity), and their plan is fundamentally incompatible with the approach in this crate, I'm down to remove it now. But if nobody volunteers (or somebody does, but they plan on using the same approach), I'd prefer to leave this as is, add appropriate warnings about safety and constraints asap, and then extend this with additional safety nets as we find strategies that work.

[bjorn3]

Unless I'm missing something, checking the rustc version would be a bit more involved, as it would require incorporating build scripts to store the environment / compiler info in a rust file, which could then be built into the binary.

Assuming bevy is built by cargo, the libbevy_dylib-*.so has a unique name depending on which rustc and bevy version is used. It doesn't help against modified source of a local bevy checkout with identical version number though.

But if nobody volunteers (or somebody does, but they plan on using the same approach), I'd prefer to leave this as is, add appropriate warnings about safety and constraints asap, and then extend this with additional safety nets as we find strategies that work.

👍

[leElvyn]

Hi !
I see in this thread that one of the major argument for the removal of this feature is that no one is currently using it. However, it might be really useful for feather-rs, a game server project written in rust, that uses ECS as it's architecture (https://github.com/feather-rs/feather). While the master branch currently doesn't implement bevy, one of it's fork does : https://github.com/Iaiao/feather/tree/bevy, and it uses the dynamic plugin system to allow users to extend the default functionalities of the server by simply dragging a file in a directory, and not having to recompile the whole server. You can see how it is currently implemented here : https://github.com/Iaiao/feather/blob/a89b786c8b0738f8e92434e443f37ed025b7becb/src/main.rs#L62.
While we are still looking at multiple options for the way plugins will be implemented, and we aren't sure this is the definitive solution, i can imagine that we aren't the only ones to look into a plugin system like this.

[alice-i-cecile]

Closing this out for now; I don't think removing this is a priority until we do a proper review of the space.