Allow more than one EditorPlugin per addon, support GDExtension classes
Implements https://github.com/godotengine/godot-proposals/issues/3782
I initially wanted to work on allowing GDExtension to register EditorPlugins (without the need for a proxy script), but because I also need more than one, I figured the proposal above could be done at the same time. So it could also solve https://github.com/godotengine/godot-cpp/issues/640
This adds support for a new editor_plugins key in plugin.cfg, which contains an array of strings. Each string can either be a path to a script (if it contains a .), or a class name present in ClassDB. The latter allows extensions to register EditorPlugins.
I left the old script key for backward compatibility, but could be removed eventually, or kept for convenience.
The way this work also means addons implemented using GDExtension can be turned on and off the like script-based ones.
There are a few issues:
- GDExtension classes inheriting
EditorPluginhave to be registered inClassDBfor this to work. But it seems it would expose them to the Add Node dialog for example, which is not desired. Maybe it needs a new argument toregister_classin the GDExtension API, unless that already exists? - How do extension class instances cope with a library getting unloaded? (for example, when Godot looses focus, if the library is configured that way). The question is already valid for classes in the edited scene tree, not just plugins.
- How can we limit class names to extensions? Currently you can specify a class name that happens to be a core class, but it isn't intentional. Maybe need to add
ClassDB::is_extension_class?
Needs rebase. Also might be worth splitting this in 2 PRs if GDExtension part has some concerns.
Splitting this would be weird, because GDExtension support is done through the feature itself, as a result of handling class names and not just scripts. If I were to split, I'd have no way to make another PR without pretty much rewriting this one. Also, most of GDExtension concerns aren't directly about the PR itself, but about in-editor GDExtension classes in general. I mentionned them here for awareness.
We approved the feature in the GDExtension PR review meeting. The comment by @KoBeWi should be addressed though, as compatibility features should always be surrounded by #ifndef DISABLE_DEPRECATED.
Another feedback is that the logic for selecting either script or editor_plugins should likely be reversed, so that editor_plugins takes precedence over script when both are defined. Also, if both are defined, Godot should give you a warning.
Regarding the support of class names in the list, we are not really convinced this is the best way to implement it. Modules usually register themselves as plugins by calling EditorPlugins::add_by_type. We are thinking a better to solve this situation would be to expose this function (or some EditorPlugins::register_extension_class()) to extension themselves so that we can register a class that way. If this function was exposed, it make support for class names in the editor_plugins list unnecessary, simplifying things a bit. Also, we could maybe rename it to editor_plugins_scripts to make it clear it does not support class names ? WDYT ?
I was thinking of this alternative of just exposing two functions to add and remove plugins, but it seems the function we currently use in modules wouldn't really work on its own. Besides, there is no remove_by_type.
The difference with modules is that modules registering plugins with add_by_type cannot be turned on and off and can only be added on startup of the editor (which makes sense since they cannot be removed from the engine as well without recompiling it). While if they are part of this config registration, they can be turned off, and also that can happen anytime.
If we go for exposing EditorPlugins::add_by_type, it does not add anything right away, instead it registers inside a list (limited to 128 elements with a never-decreasing count) which is used later in the editor's startup. That logic would have to change because it looks like it was written only for core and modules plugins that are always added on startup and can never be removed until the editor closes. Extensions can be added anytime and removed anytime (well, currently not, but we should try not to assume that since other devs are figuring out how to do it).
Regarding not exposing editor classes, I would not couple the fact they are not exposed with EditorPlugin::add_by_type, since not every class registered that way is necessarily a plugin. Perhaps it's a matter of being able to specify a flag when registering classes with the regular ClassDB function? (also I'm assuming they need to be in ClassDB due to how GDExtension works, while in modules they just aren't registered at all)
(I can fix the PR but I'd like to be sure of the design)
For the GDExtension part of this, I see at least three possible approaches (the last two are just restating what @Zylann describes above in their last comment):
- We could add an
add_editor_plugin()andremove_editor_plugin()toEditorInterface. After the editor is initialized, it should be safe to add or remove editor plugins at any time, since we do it with addons. The challenge here is can a GDExtension call this method after theEditorNodeis initialized and ready? I suspect thatGDEXTENSION_INITIALIZATION_EDITORmay happen beforeEditorNodeinitialized, and if that's the case, this wouldn't work (at least without adding another initialization level, exGDEXTENSION_INITIALIZATION_EDITOR_READY) - Mark the editor plugin classes as editor plugins in
ClassDBand then haveEditorNodeloop through them after doing the built-in ones. This could also allow us to attempt to remove editor plugins when the class is being removed. And if we try to add such classes after the editor is already initialized, it could just add them right away. - Actually expose
EditorPlugins::add_by_type()via a new GDExtension interface function, and have editor plugins from GDExtensions get added the same way as the built-in ones. This option (as @Zylann points out as well) has the disadvantage of not allowing a way to unregister the editor plugins, and registration would only work at editor startup. If a GDExtension is enabled or disabled sometime in the middle of the editor running, it wouldn't work.
So, I think option nr 1 or 2 are the better way to go.
Does anyone have a preference? Or, any other implementation ideas?
Despite what I wrote in my last comment yesterday, I ended up trying out an implementation that is closer to option nr 3, but done in away to avoid it's downsides (ie. fully supporting adding and removing editor plugins at any time, including engine initialization/deinitialization).
Here's the draft PR: #77010
I did start to implement the other options but abandoned them for various reasons:
- Option nr 1: there were issues with initialization order, which would have required adding a new initialization level, and thought that would probably be rejected. And I started to feel that the API would be confusing on
EditorInterface, which already has aset_plugin_enabled()andis_plugin_enabledfunction (where "plugin" actually means "addon"). - Option nr 2: marking the classes as editor plugins in
ClassDBwould have worked, but the only thing we really need fromClassDB, is that it already exists early enough in Godot's initialization process. We could easily avoid adding new code toClassDBby just keeping a list onEditorPluginswhich would also exist early enough. So, I figured it'd be best not add anything to core.