Allow GDExtensions to add editor plugins

[dsnopek]

Fixes godotengine/godot-cpp#640

This is an alternative to PR #65592 (but just the GDExtension part) based on some feedback that PR received from the GDExtension team back in December.

It adds two new functions to the GDExtensionInterface struct (editor_add_plugin and editor_remove_plugin) which both take a StringName representing a class registered in ClassDB (since all GDExtension classes need to be registered in ClassDB).

This is used in the companion godot-cpp PR: godotengine/godot-cpp#1114

It allows you to use EditorPlugins::add_by_type<MyEditorPlugin>() in your GDExtension, in much the same way as you would in a module.

If this is called during Godot's initialization process, it's just adding the class name to a list, and the actual editor plugins will be created later in EditorNode::EditorNode(), in the same place the built-in editor plugins are created. If it's called after the editor is already initialized, then it'll call straight into EditorNode::get_singleton()->add_extension_editor_plugin().

This is currently a draft because:

• I still need to do the godot-cpp changes and actually test it :-)
• If PR Rework GDExtension interface from a struct to loading function pointers #76406 is merged, then this'll need to get reworked to use the new way of registering GDExtension interface functions. That PR is more important, so I'm fine saving this one until that one is resolved.

For the moment, I'd mainly like to see what others think of this general approach!

UPDATE: This is now ready for review!

[dsnopek]

I've rebased this now that PR #76406 has been merged, made the companion godot-cpp PR (godotengine/godot-cpp#1114) and actually tested that this works. :-)

This is now ready for review!

[BastiaanOlij]

Discussed this with @dsnopek after last meeting. This seems like a sound solution within the constraints we currently have.

My only remaining remark is that this is worth a wider discussion around our limited access to singletons. The "proper" way for this is that we do have access to the EditorNode singleton during registration so we can register the plugin just like core modules do.
We can't because in the current approach we're piggy backing singleton access on singletons being registered with the scripting engine and this doesn't happen until all classes are registered.

Imho we should change this part of godot-cpp and make the actual get_singleton() functions of these classes callable from godot-cpp. That is potentially a breaking change on the API but worth it as now we will simply be able to call the correct functions.

cc @vnen

[dsnopek]

Thanks, @BastiaanOlij!

Just quick clarification on this point:

The "proper" way for this is that we do have access to the EditorNode singleton during registration so we can register the plugin just like core modules do.

Core modules actually don't access the EditorNode singleton during registration to add their editor plugins, because even for modules, the EditorNode singleton won't be created until later.

Modules register a creation function during registration (via EditorPlugins::add_by_type<>()), and then in EditorNode's constructor, it calls all the creation functions. This PR does basically the same thing, but instead of registering a creation function, it's registering the name of the class in ClassDB (and then EditorNode's constructor creates them just like for modules).

[BastiaanOlij]

Ah gotcha, so really in this case we should be use exposing static functions but thats not really possible in this case because it's using a template.

Sigh, some things just don't survive the bridge :)

Yeah I'm all ok with this going ahead. I just hope we get around to doing the other thing some day too :)

[anunknowperson]

I am waiting for this, because until it isn't merged, it is impossible to create advanced plugins with GDExtension (
For now, I have compiled the engine with this PR by myself.

[anunknowperson]

@dsnopek also i can't compile this PR with MSVC, I get this error:
.\core/extension/gdextension_interface_dump.gen.h(1589): fatal error C1091: compiler constraint: string length exceeds 65535 bytes

It compiles fine with MinGW. Should it be like this?

[anunknowperson]

I tried to create simple 3d editor:

test_editor.h

#ifndef TEST_EDITOR_PLUGIN_H
#define TEST_EDITOR_PLUGIN_H


#include <godot_cpp/classes/editor_plugin.hpp>
#include <godot_cpp/classes/editor_interface.hpp>
#include <godot_cpp/classes/ref.hpp>
#include <godot_cpp/classes/input_event.hpp>


namespace godot {

class TestEditorPlugin : public EditorPlugin {
	GDCLASSTestEditorPlugin EditorPlugin);


protected:
	void _notification(int p_what);

public:

	static void _bind_methods();

	virtual int _forward_3d_gui_input(Camera3D* p_camera, const Ref<InputEvent>& p_event) override;
	virtual String _get_plugin_name() const override { return "Test"; }
	virtual bool _has_main_screen() const override { return false; }
	virtual void _edit(Object* p_object) override;
	virtual bool _handles(Object* p_object) const override;
	virtual void _make_visible(bool p_visible) override;


	TestEditorPlugin ();
	~TestEditorPlugin ();

};


}

#endif 

test_editor.cpp contains empty implementations.

When I start a project and create any node, the editor hangs without any errors in the console or anywhere.

[dsnopek]

also i can't compile this PR with MSVC, I get this error:

This was fixed in PR #77137 - I'll rebase this PR to include it in a moment.

When I start a project and create any node, the editor hangs without any errors in the console or anywhere.

Hm, I did an even simpler test when working on this, and that worked for me. Later, I'll try with your test class and see what happens.

[anunknowperson]

@dsnopek Minimal reproduction project:
src.zip

[anunknowperson]

Video:

Untitled.mp4

There are also some errors in the console, but they appear regardless of whether there is any extension or not. (And they don't seem to affect anything, because when no extension is added to the project, nothing is freezes.)

Exactly same code rewritten to GDScript works without any problems.

[dsnopek]

Thanks! I'm able to reproduce the issue with your MRP.

It seems to be related to the _handles(Object *) method: if I remove that method from the class, then the editor doesn't hang.

I'll see if I can debug further...

[dsnopek]

@anunknowperson The hanging appears to be a regression caused by this godot-cpp PR: godotengine/godot-cpp#1044 - if I reverse the changes that were committed there, then it doesn't hang, and the editor starts up normally!

For the details: running the MRP in the debugger, it looks like we're hanging on _instance_binding_mutex.lock() in Object::get_instance_binding(). Stepping up through the stack trace and inspecting all the pointers, in the transition from Godot to godot-cpp we seem to end up in a state where we're treating a random chunk of memory as if it were an Object, but it definitely isn't. I think the mutex hangs forever because isn't actually a valid mutex.

A related PR for godot-cpp (godotengine/godot-cpp#1045) caused a similar regression, where it fixed calling normal methods, but broke calling virtual methods. This looks to be the same thing! When calling virtual methods, Godot is passing an Object ** but godot-cpp (after the afformentioned PR) is treating it as Object *.

In any case, that bug is out-of-scope for this PR! As far as I can tell, this PR is correctly registering the editor plugin.

[anunknowperson]

@dsnopek thanks!
In other words, to make it work for now, I need to revert #1044? Or will it add some new problems?

[dsnopek]

In other words, to make it work for now, I need to revert godotengine/godot-cpp#1044?

Yes.

Or will it add some new problems?

Well, it did fix a legitimate bug, so you'll be getting that bug back, unfortunately. So, it really depends on if your project runs into that bug too.

The regressions from godotengine/godot-cpp#1044 and godotengine/godot-cpp#1045 are critical and need to get fixed before Godot 4.1. I'm personally planning to focus my attention on them after the feature freeze next week - until then, we gotta try and finish up any new features (like this editor plugin PR). After the feature freeze, we'll have another 4 weeks to work on bugs. :-)

[BastiaanOlij]

Discussed privately and in the GDExtension meeting. This seems like a clean way to add the ability to register editor plugins properly. Ready to be merged

[akien-mga]

Thanks!