godot icon indicating copy to clipboard operation
godot copied to clipboard

Allow more than one EditorPlugin per addon, support GDExtension classes

Open Zylann opened this issue 3 years ago • 2 comments

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 EditorPlugin have to be registered in ClassDB for 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 to register_class in 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?

Zylann avatar Sep 09 '22 22:09 Zylann

Needs rebase. Also might be worth splitting this in 2 PRs if GDExtension part has some concerns.

KoBeWi avatar Nov 09 '22 12:11 KoBeWi

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.

Zylann avatar Nov 19 '22 13:11 Zylann

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 ?

groud avatar Dec 15 '22 12:12 groud

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)

Zylann avatar Dec 15 '22 15:12 Zylann

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):

  1. We could add an add_editor_plugin() and remove_editor_plugin() to EditorInterface. 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 the EditorNode is initialized and ready? I suspect that GDEXTENSION_INITIALIZATION_EDITOR may happen before EditorNode initialized, and if that's the case, this wouldn't work (at least without adding another initialization level, ex GDEXTENSION_INITIALIZATION_EDITOR_READY)
  2. Mark the editor plugin classes as editor plugins in ClassDB and then have EditorNode loop 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.
  3. 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?

dsnopek avatar May 11 '23 22:05 dsnopek

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 a set_plugin_enabled() and is_plugin_enabled function (where "plugin" actually means "addon").
  • Option nr 2: marking the classes as editor plugins in ClassDB would have worked, but the only thing we really need from ClassDB, is that it already exists early enough in Godot's initialization process. We could easily avoid adding new code to ClassDB by just keeping a list on EditorPlugins which would also exist early enough. So, I figured it'd be best not add anything to core.

dsnopek avatar May 12 '23 17:05 dsnopek