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.

Sign up for GitHub

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

Enable EditorPlugin added by modules and GDExtensions (reverted) #90608

Merged
merged 1 commit into from
May 2, 2024
Merged

Enable EditorPlugin added by modules and GDExtensions (reverted) #90608

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion editor/editor_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3492,6 +3492,7 @@ void EditorNode::add_extension_editor_plugin(const StringName &p_class_name) {
EditorPlugin *plugin = Object::cast_to<EditorPlugin>(ClassDB::instantiate(p_class_name));
singleton->editor_data.add_extension_editor_plugin(p_class_name, plugin);
add_editor_plugin(plugin);
plugin->enable_plugin();
Comment on lines 3494 to +3495
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@akien-mga akien-mga May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

CC @KoBeWi

Copy link
Member Author

Choose a reason for hiding this comment

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

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 plugins1, 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. ↩

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

So should this be reverted then?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

void EditorNode::remove_extension_editor_plugin(const StringName &p_class_name) {
Expand Down Expand Up @@ -7217,7 +7218,9 @@ EditorNode::EditorNode() {
add_editor_plugin(memnew(AudioBusesEditorPlugin(audio_bus_editor)));

for (int i = 0; i < EditorPlugins::get_plugin_count(); i++) {
add_editor_plugin(EditorPlugins::create(i));
EditorPlugin *plugin = EditorPlugins::create(i);
add_editor_plugin(plugin);
plugin->enable_plugin();
Comment on lines +7222 to +7223
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

}

for (const StringName &extension_class_name : GDExtensionEditorPlugins::get_extension_classes()) {
Expand Down
Loading