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.

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

Don't override default behaviour of methods if not explicitly overridden #14

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Beliar83
Copy link

This fixes #13 by using reflection.

…clared in the type to register to catch classes that inherit from another custom class.
…cept of protected and change them to public.
@Beliar83
Copy link
Author

Beliar83 commented Aug 19, 2024

I think we should also only match methods with the correct argument types (which would just be passing a "types" argument as an array with Type variables to GetMethod)

Copy link
Owner

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the Godot .NET bindings.

I think we should also only match methods with the correct argument types (which would just be passing a "types" argument as an array with Type variables to GetMethod)

How about checking that the methods are the same like godot-cpp does? Pseudo-code example:

if (typeof(T).GetMethod("_Ready") != typeof(Node).GetMethod("_Ready"))
{
    // ...
}

I see that MethodInfo implements the != operator, but I'm not sure how it compares the methods.

The MethodInfo retrieved from built-in types (like Node) are guaranteed to be unique, because ClassDB doesn't support overloads.

src/Godot.Bindings/NativeInterop/InteropUtils.cs Outdated Show resolved Hide resolved
{
internal static FrozenDictionary<StringName, Func<nint, GodotObject>> CreateHelpers { get; private set; }

internal static FrozenDictionary<StringName, Action<ClassDBRegistrationContext>> RegisterVirtualOverridesHelpers { get; private set; }
internal delegate void RegisterVirtualOverrideHelper([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicMethods)] Type type, ClassDBRegistrationContext context);
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't this also need DynamicallyAccessedMemberTypes.PublicMethods?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, must've missed that

src/Godot.Bindings/Bridge/ClassDB/ClassDB.cs Show resolved Hide resolved
@Beliar83
Copy link
Author

I think we should also only match methods with the correct argument types (which would just be passing a "types" argument as an array with Type variables to GetMethod)

How about checking that the methods are the same like godot-cpp does? Pseudo-code example:

if (typeof(T).GetMethod("_Ready") != typeof(Node).GetMethod("_Ready"))
{
    // ...
}

I see that MethodInfo implements the != operator, but I'm not sure how it compares the methods.

The MethodInfo retrieved from built-in types (like Node) are guaranteed to be unique, because ClassDB doesn't support overloads.

I am looking into something similar. From what I found there is a "GetBaseDeclaration()" method in MethodInfo that would either return the same Instance, if it is not an override or the Info of the base. Getting the Info of the base type might do the same, not sure if either is faster.

Co-authored-by: Raul Santos <raulsntos@gmail.com>
@Beliar83 Beliar83 marked this pull request as draft August 19, 2024 18:19
@Beliar83 Beliar83 marked this pull request as ready for review August 21, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default behaviour of virtual methods is overridden
2 participants