Default behaviour of virtual methods is overridden

[Beliar83]

Godot version

4.3.rc2.official.3978628c6

Godot .NET packages version

8f75340

System information

Windows 11

.NET information

8.0.202

Issue description

Often the functions of Godot have a default when a virtual method to call is not implemented (Example: https://github.com/godotengine/godot/blob/739019e4e4a6e4763e37adfd9883a1c85d5f6249/core/io/resource_loader.cpp#L130)

Currently the virtual methods are always bound, which overrides that default, which could return something other than what the default implementation does, even when that would be sufficient.

Steps to reproduce

Derive from a class with virtual methods and only override the methods needed to be overridden. Observe the other methods not having their default behaviour.

Minimal reproduction project

--

[raulsntos]

Thanks for the report. It looks like godot-cpp only binds the virtual method if it's overridden:

https://github.com/godotengine/godot-cpp/blob/4b7661a60a4788cc7d5ce690cf8a5815b0aab511/binding_generator.py#L1568-L1575

template <typename T, typename B>
static void register_virtuals() {
    RefCounted::register_virtuals<T, B>();
    // [...]
    if constexpr (!std::is_same_v<decltype(&B::_exists),decltype(&T::_exists)>) {
        BIND_VIRTUAL_METHOD(T, _exists);
    }
    // [...]
}

Checking whether a virtual method is overridden in a C# class would probably require using reflection or source generators, and I'm trying to avoid both for class registration.

• Reflection probably won't be compatible with trimming (and by extension also NativeAOT).
• Source generators aren't available to some .NET languages (e.g.: F#) and I want to keep them optional.

[Beliar83]

Yeah, but unless the BindingsGenerator can create calls to or reimplement the default behaviour - and I don't think the extension json or header have information about that - the best way probably is to not bind them by default and then either find ways for the specific usage and language to automatically bind them, or have the person writing the extension bind them manually.

[Beliar83]

If I understand this correctly one can use the DynamicallyAccessedMembers Attribute to tell the trimmer that a generic method needs some information preserved, like Public Methods for this, but of course this would mean than no public method is trimmed on that class, which I am not sure is desirable.

[Beliar83]

This commit would have RegisterVirtualOverrides only register the actually overridden methods, but it would have to be called manually in something like the "BindMethods" function.
I am not sure how to have it automatically register them without either the methods to check being possibly trimmed - as I would lose the possibility to tell the Trimmer not to trim these - or unused methods of that class to not being trimmed when using something like source generators to do the actual registration.

[raulsntos]

If I understand this correctly one can use the DynamicallyAccessedMembers Attribute to tell the trimmer that a generic method needs some information preserved, like Public Methods for this, but of course this would mean than no public method is trimmed on that class, which I am not sure is desirable.

I'm also worried that using that attribute will prevent trimming too much. We'll have to test how much of a difference this makes.

This commit would have RegisterVirtualOverrides only register the actually overridden methods, but it would have to be called manually in something like the "BindMethods" function.

I think we should be able to call RegisterVirtualOverrides like before in ClassDB.RegisterClassCore<T>. We just need to add the if to the implementation, right?

So how about something like this?

internal unsafe static new void RegisterVirtualOverrides([DynamicallyAccessMembers(NonPublicMethods)] Type type, ClassDBRegistrationContext context)
{
    GodotObject.RegisterVirtualOverrides(context);
    if (type.GetMethod("_Process").DeclaringType != typeof(Node))
    {
        context.BindVirtualMethodOverride(MethodName._Process, static (Node __instance, double delta) =>
        {
            __instance._Process(delta);
        });
    }

    // ...

Then, we'd define the dictionary like this:

internal delegate void RegisterVirtualOverrideHelper([DynamicallyAccessedMembers(NonPublicMethods)] Type type, ClassDBRegistrationContext context);

internal static FrozenDictionary<StringName, RegisterVirtualOverrideHelper> RegisterVirtualOverridesHelpers { get; private set; }

I think that (and the required attributes in ClassDB) should be enough to annotate the type parameter to prevent trimming the methods, but I haven't tested it. Also, since the virtual methods are protected I guess we should be using DynamicallyAccessedMemberTypes.NonPublicMethods.

[Beliar83]

Okay, with the changes here this seems to work, though I get warnings that the Trimmer may not be able to guarantee it.

The changes from this are also needed for there to not be a null reference exception, but I am not sure how safe it actually is to just not write anything in that case. With null being a viable return value for some methods it should be handled in some way, though.

[Beliar83]

I had to add these changes to also catch when another class in dotnet inherits from a class in dotnet

[Beliar83]

I also had to add these changes since F# does not currently have protected methods and always overrides methods as public

[raulsntos]

I haven't had time to test those changes but I took a quick look and the code looks good to me.

The changes from this are also needed for there to not be a null reference exception

This change seems unrelated. But I think I know the issue you are referring to (see #2 (comment)). It's about Packed Arrays, right?

[Beliar83]

This change seems unrelated. But I think I know the issue you are referring to (see #2 (comment)). It's about Packed Arrays, right?

Actually I think I maybe wrong about that being needed for this. It is possible that I did not check this in isolation. I am going to recheck.

[Beliar83]

Okay, yeah, the commit with the null check is not needed for solving this issue, at least.