Enable EditorPlugin added by modules and GDExtensions (reverted)

[raulsntos]

Before this PR, plugins coming from modules/GDExtensions were never enabled so the virtual method _enable_plugin was never called. This can be confusing behavior for users implementing EditorPlugin coming from addons/scripting. I assume GDExtension plugins are considered always enabled, so I expect the _enable_plugin to be called exactly once when they are initially added.

MRP: GDExtensionEditorPluginCpp.zip

[kisg]

To me the main question is: Why do we need separate methods to enable / disable plugins from extensions in the first place? The normal add_editor_plugin() / remove_editor_plugin() already has a way to enforce enabling the plugin using its p_config_changed parameter (I think this is what Raul mentioned earlier.)

I see in the code that for some reason GDExtension-based EditorPlugins are handled differently now, but I think it would be better conceptually, if plugins defined by GDExtensions would be handled the same way as plugins defined in modules.

So maybe we should just refactor this and expose the normal add_editor_plugin() / remove_editor_plugin() over the GDExtension / Godot API.

[dsnopek]

Thanks, this looks good to me!

[dsnopek]

@kisg:

In Godot itself, editor plugins are typically registered via EditorPlugins::add_by_type<T>(), which doesn't allow you to set anything for the p_config_changed parameter. It's the same API on the GDExtension side.

There's a handful of cases where editor plugins are registered by directly calling EditorNode::get_singleton()->add_editor_plugin(): I count 7 instances, versus the 66 instances of the other.

It seems those 7 cases are doing that because EditorPlugins::add_by_type<T>() only works before EditorNode is constructed, so in the cases where the plugin is added after, they need to call EditorNode::add_editor_plugin() directly.

Perhaps we change EditorPlugins::add_by_type<T>() to work even after EditorNode has been created, so we could standardize on it everywhere?

[kisg]

@dsnopek

After looking at the code in more detail, my suggestion would be to turn the EditorPlugins class into a GDCLASS and expose it early to GDExtensions. The class would be exposed as ABSTRACT, with only a couple of static methods. Its purpose would be twofold:

• Collect plugin registration requests while the EditorNode singleton is not yet available
• Directly add plugins/remove plugins if the EditorNode singleton has been already initialized.

At the same time we should make EditorNode::add/remove_editor_plugin private (and make EditorPlugins a friend)

API wise, we can keep the add_by_type<> method but extend it to work like this (pseudocode):

if (!EditorNode::get_singleton()) {
   // Execute current logic with adding the class to a deferred creation list
} else {
  EditorNode::get_singleton()->add_editor_plugin(memnew(T), true); // We request the plugin to be enabled
}

We can also have the add_editor_plugin() and remove_editor_plugin() method pairs in 2 variants:

1. The ones similar to the current extension plugin registration

void EditorPlugins::add_editor_plugin_by_name(StringName name, bool enable);
void EditorPlugins::remove_editor_plugin_by_name(StringName name);

2. The ones similar to the current in-engine plugin registration

void EditorPlugins::add_editor_plugin(EditorPlugin* plugin, bool enable);
void EditorPlugins::remove_editor_plugin(EditorPlugin* plugin);

All of these methods can be used prior to the creation of EditorNode or after it.

One final method that is only exposed to EditorNode:

void EditorPlugins::add_registered_editor_plugins(EditorNode* node);

This would be called by EditorNode during its construction. It will call the node->add_editor_plugin(..., true) for each pre-registered plugin.

The advantages of this refactoring:

• A single place to add/remove editor plugins, which is decoupled from EditorNode.
• Can be used at any time (early and late registration of editor plugins).
• Exposed to GDExtensions as a standard abstract class, no special handling required. The bindings for it will be automatically generated.
• No need for special handling in the GDExtension API.

[dsnopek]

@kisg:

Exposed to GDExtensions as a standard abstract class, no special handling required. The bindings for it will be automatically generated.

I don't think we want this, though. This would make this class and its methods available to script, which we specifically didn't want to do, which is why we added functions to gdextension_interface.h.

In any case, I think this is becoming a tangent. The main point of this PR is to call enable_plugin() on editor plugins registered in modules and GDExtensions, which previously was only done for editor plugins registered by scripts.

I do see your point that we could use the 2nd argument of EditorNode::add_editor_plugin() in the implementation though. I'll add some code review about that in a moment.

However, your proposal mostly concerns cleaning up how EditorPlugins::add_by_type() is implemented, which isn't directly related to this PR, and so we should move discussion of that to a new issue or PR. That said, I think we could solve the core problem (that there are a few cases where we calling EditorNode::add_editor_plugin() directly rather than using EditorPlugins::add_by_type()) in simpler way than proposed. But let's discuss that on the new issue/PR :-)

[dsnopek]

As @kisg points out, we could alternatively implement this by doing:

Suggested change

	add_editor_plugin(plugin);
	plugin->enable_plugin();
	add_editor_plugin(plugin, true);

If we do that, though, we'd probably want to change the name of that argument, since the name is currently p_config_changed, which doesn't make sense in this context.

[raulsntos]

Yeah, I was thinking that, even though p_config_changed is currently only used to determine if enable_plugin() is called, it may be used for more stuff in the future and I didn't want to change its meaning. But I'm happy to change it if we're fine with that.

[akien-mga]

The current logic for p_config_changed seems to be a toggle for on/off:

void EditorNode::set_addon_plugin_enabled(const String &p_addon, bool p_enabled, bool p_config_changed) {
...
	if (!p_enabled) {
		EditorPlugin *addon = addon_name_to_plugin[addon_path];
		remove_editor_plugin(addon, p_config_changed);
...
		return;
	}
...
	add_editor_plugin(ep, p_config_changed);
...
}

add_editor_plugin enables the plugin if p_config_changed, and remove_editor_plugin disables the plugin if p_config_changed.

So I think we can rely on that behavior, but I'd rename the bool in add_editor_plugin and remove_editor_plugin respectively to p_enable_plugin and p_disable_plugin.

That's being said I'm a bit confused about the difference between adding and enabling, and removing and disabling. Especially the latter two - can one remove a plugin without it being also disabled?

Edit: Shouldn't be blocking for this PR though IMO, this can be tweaked later.

[akien-mga]

CC @KoBeWi

[raulsntos]

One can add a plugin but not enable it (see the Activate now checkbox when creating GDScript plugins). You can also always enable/disable plugins from project settings.

GDExtension and module plugins don't really allow enabling/disabling plugins¹, I think it's implied that these plugins are always enabled when added and disabled when removed. But before this PR the _enable_plugin and _disable_plugin virtual methods were never called.

The motivation behind this PR was to maintain consistency with the current scripting behavior in GDExtension plugins. This is important to me since I want to avoid behavior changes for the .NET bindings when moving from scripting to GDExtension.

I agree about removing without disabling being confusing, I'd assume removing always imply disabling. I forgot about disabling in this PR though, because the .NET plugin I'm working on doesn't override _disable_plugin so I didn't notice.

Footnotes

1. This may change in the future for GDExtensions, since the GDExtension team wants to allow enabling/disabling GDExtensions in project settings. ↩

[raulsntos]

When you open a project, the plugins listed in project.godot are added, but the enabled callback is not called.

Oh, then this PR is wrong.

[raulsntos]

So should this be reverted then?

[KoBeWi]

You mean the whole PR or just this change? (or it's the same, idk)
I described the expected behavior, so the current one should be tweaked if it's different.

[raulsntos]

Yeah, it's the same. If we don't want GDExtension EditorPlugins to have _enable_plugin called when Godot is launched, then we should revert the PR. But that means _enable_plugin will never be called for GDExtension EditorPlugins since we have no way of enabling them (I guess they are considered to be pre-enabled when they are added).

[KoBeWi]

There is enter tree callback called on editor launch, so this use-case is covered. _enable_plugin is for when user enables a plugin, and if GDExtensions can't be toggled, this callback is not for them.

[dsnopek]

Same here.

[akien-mga]

Thanks!