Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow more than one EditorPlugin per addon, support GDExtension classes #65592

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Sep 9, 2022

Implements godotengine/godot-proposals#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 godotengine/godot-cpp#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 Zylann force-pushed the addon_plugins branch 2 times, most recently from feae179 to 6cb4537 Compare September 9, 2022 22:32
Vector<String> plugin_classes;
if (cf->has_section_key("plugin", "script")) {
// Backward compatibility
plugin_classes.push_back(cf->get_value("plugin", "script"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatibility code should be wrapped in #ifndef DISABLE_DEPRECATED

@KoBeWi
Copy link
Member

KoBeWi commented Nov 9, 2022

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

@Zylann
Copy link
Contributor Author

Zylann commented Nov 19, 2022

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.

Copy link

@Salvakiya Salvakiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. Having capability to use GDExtension in an EditorPlugin is essential.

return;
}
// if (plugin_classes.size() == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented out code can be removed.

plugin_classes.push_back(cf->get_value("plugin", "script"));

} else if (cf->has_section_key("plugin", "editor_plugins")) {
Array array = cf->get_value("plugin", "script");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a mistake. Should be Array array = cf->get_value("plugin", "editor_plugins");

@groud
Copy link
Member

groud commented Dec 15, 2022

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 ?

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2022

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)

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@dsnopek
Copy link
Contributor

dsnopek commented May 11, 2023

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
Copy link
Contributor

dsnopek commented May 12, 2023

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.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@KoBeWi KoBeWi modified the milestones: 4.3, 4.x Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants