bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.24k stars 3.57k forks source link

Deprecate current `bevy_dynamic_plugin` implementation #11969

Closed BD103 closed 5 months ago

BD103 commented 9 months ago

What problem does this solve or what need does it fill?

bevy_dynamic_plugin is likely unsound, or at the very least so dangerous that it should not be an officially sanctioned crate. It currently works by deriving bevy_app::DynamicPlugin onto a unit struct that implements Plugin. The code can then be built as a cdylib and loaded with bevy_dynamic_plugin::dynamically_load_plugin.

There are a few potential footguns / problems with this approach:

What solution would you like?

Mark dynamically_load_plugin and DynamicPluginExt::load_plugin as deprecated for Bevy 0.14. In Bevy 0.15, they will be removed and bevy_dynamic_plugin will be turned into an empty crate likely moved to bevy-crate-reservations.

I think the current implementation is just too unsafe to continue in the main Bevy library. As mentioned by Alice, it may be worth recommending dextrous_developer in the module or deprecation notes.

What alternative(s) have you considered?

Try to fix the above issues by exclusively using TypePath instead of TypeId and manually recreating the VTable struct. ctor is unavoidable, we just have to hope it won't be encountered in practice.

Additional context

Some discussion on Discord on bevy_dynamic_plugin, while I was working on #11622.

I think there is promise is dynamically loading plugins, so I would not be opposed to bevy_dynamic_plugin being re-introduced in the future, but I don't think the current implementation should be kept.

alice-i-cecile commented 9 months ago

FYI @cart. I know you pushed back on this in #3893 in the past.

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:

I think the landscape has meaningfully changed here on the editor side, between the existence of dextrous_developer and @coreh's remote protocol experiments. That said, I still don't have a good answer to the DLC use case you mentioned:

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.

Overall while I share these concerns I'd be fine to leave this around until we have a good sense of the architecture of the editor, and how editor extensions in particular will work.

lee-orr commented 9 months ago

I should note that I am biased here - as the maintainer of dexterous_developer - but I think the current version of this crate is worth revoking.

I also think that the solutions for dynamic loading in a development context should be different than the ones used for things like DLC and mods - the constraints around them are very different:

I do think it might be worth looking at adding stabby to Bevy as an optional feature for ABI stability, and then we can use that as a basis for both the bevy plugin in dexterous developer (or for integrating that plugin into bevy) and for a separate approach for handling release-capable dynamic libraries for mods and DLC.

BD103 commented 8 months ago

I just opened #12029. While it does not deprecate dynamically_load_plugin, it does encourage the use of several alternatives which may be potentially safer.