Allow coexistence of GDScript and GDExtension virtual methods in the same object

[anrp]

…same object

Resolves godotengine/godot-cpp#1224

[dsnopek]

Thanks!

I skimmed the code, but I think I need to some more testing with this.

[dsnopek]

Everything is looking good in my testing, including with the MRP from issue godotengine/godot-cpp#1224

Great work!

[PgBiel]

So, I did some testing with a rather simple gdext project (thus, using Rust bindings - things should probably behave the same as with other bindings though, since very similar, if not identical, bugs have been observed compared to godot-cpp), and I wanted to share some of the results.

The source of the project is here: https://github.com/PgBiel/hello-gdext-wasm/tree/56916dabe21cc45eb721889a17d059d729384ccf (simply cd rust && cargo build to compile the extension).

Basically, the project contains a Player class created by the extension, subclassing Sprite2D. It overrides init, ready (prints "Rust is ready") and physics_process (rotates the node forever). Here I attempt to add a script to it and see the results.

Before this PR (4.2-beta1):

1. Without a script, everything works (prints "Rust is ready" and the node rotates, i.e. physics_process works).
2. By attaching a newly-created script (even when removing all of the default functions in it), nothing proceeds to happen ("Rust is ready" isn't printed and the node isn't rotated, so none of the extension's overrides are recognized anymore).
3. Explicitly overriding the ready function in GDScript, making it print Godot ready, results in only that being printed - Rust ready isn't printed and the node still doesn't rotate, indicating GDScript has indeed not only fully "disabled" but also overridden (the latter is, of course, to be expected) the extension's virtual method overrides.
4. Attempting to call super() from _ready in the script (in an attempt to recover the extension's behavior) results in Parser error: Cannot call the parent class' virtual function "_ready()" because it hasn't been defined.

With this PR:

1. Without a script, everything still works, like before.
2. By attaching a newly-created script, without overriding any functions (thus removing the default overrides), everything works ("Rust is ready" is printed and the node rotates), indicating this PR does make it so GDScript doesn't fully "disable" the extension's virtual methods anymore.
3. Explicitly overriding the ready function like before still makes it print Godot ready but not Rust is ready. Regardless, the node still rotates, so physics_process from the extension still works.
4. Attempting to call super() from GDScript's _ready results in the same parser error from before.

So I believe this PR fixes the very obvious bug of GDScript fully 'disabling' extensions' virtual method overrides even when the script does not override those particular methods at all, which is great!

Based on my debugging above, however, I don't think this will fully fix issues such as godotengine/godot-cpp#1022 just yet (since e.g. you can't even call super to opt into preserving the extension's behavior as supposedly the method "isn't defined"). But this PR's fix would already be a very important step forward in this regard by fixing (issues such as) godotengine/godot-cpp#1224 .

[ducklin5]

I've tested this PR and I'm also seeing the same results as @PgBiel. Thanks for the detailed report

[anrp]

[snip]

3. Explicitly overriding the `ready` function like before still makes it print `Godot ready` but not `Rust is ready`. Regardless, the node still rotates, so `physics_process` from the extension still works.

4. Attempting to call `super()` from GDScript's `_ready` results in the same parser error from before.

Based on what I see in core/os/main_loop.cpp void MainLoop::_bind_methods() I think _ready (and some others like _enter_tree) are not actually virtual functions so that is a correct error? (I've never done anything like that)

[PgBiel]

[snip]

3. Explicitly overriding the `ready` function like before still makes it print `Godot ready` but not `Rust is ready`. Regardless, the node still rotates, so `physics_process` from the extension still works.

4. Attempting to call `super()` from GDScript's `_ready` results in the same parser error from before.

Based on what I see in core/os/main_loop.cpp void MainLoop::_bind_methods() I think _ready (and some others like _enter_tree) are not actually virtual functions so that is a correct error? (I've never done anything like that)

Unfortunately, the exact same error (bar the method name) occurs with physics_process (apparently bound in the function you refer to). I believe ready is bound here:

godot/scene/main/node.cpp

Line 3527 in f8818f8

GDVIRTUAL_BIND(_ready);

The error I show seems to point to the code below - I don't know the full implications yet, but this seems to suggest to me that whatever it's trying to access with super() is a non-overridden virtual method (perhaps it's skipping the GDExtension's virtual method override and going straight for the native supertype's non-overridden impl?):

godot/modules/gdscript/gdscript_analyzer.cpp

Lines 3298 to 3303 in f8818f8

	if (get_function_signature(p_call, is_constructor, base_type, p_call->function_name, return_type, par_types, default_arg_count, method_flags)) {
	// If the method is implemented in the class hierarchy, the virtual flag will not be set for that MethodInfo and the search stops there.
	// Virtual check only possible for super() calls because class hierarchy is known. Node/Objects may have scripts attached we don't know of at compile-time.
	if (p_call->is_super && method_flags.has_flag(METHOD_FLAG_VIRTUAL)) {
	push_error(vformat(R"*(Cannot call the parent class' virtual function "%s()" because it hasn't been defined.)*", p_call->function_name), p_call);
	}

But this is getting a bit off-topic for the PR now; I believe we should move this discussion to a separate issue (Godot maintainers, feel free to guide us!). The following issue seems the most appropriate: #72034

The following issue also seems to be at least tangentially related, but doesn't mention native types: #82267

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉