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

JavaClassWrapper: Improve handling of typed array arguments #102817

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 13, 2025

Fixes #102712

There are 3 issues that come from that:

  • Method selection doesn't take typed arrays (including the Packed*Array types) into account when selected an overloaded method
  • [x] vararg methods aren't supported
  • Arrays of JavaObjects aren't supported (this one I noticed while working on this)

So far, this PR only fixes the first one, which is why this is currently marked as DRAFT.

UPDATE: All the issues are fixed now in my testing!

UPDATE 2: I ended up deciding to remove the explicit support for varargs - see this comment.

@dsnopek dsnopek added this to the 4.x milestone Feb 13, 2025
@dsnopek dsnopek requested a review from a team as a code owner February 13, 2025 18:47
@dsnopek dsnopek marked this pull request as draft February 13, 2025 18:47
@dsnopek dsnopek force-pushed the java-class-wrapper-array-improvements branch 2 times, most recently from d405381 to 19b3254 Compare February 13, 2025 23:23
@dsnopek dsnopek marked this pull request as ready for review February 13, 2025 23:24
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 13, 2025

I've now got all the issues fixed in my testing!

However, this does change quite a large amount of code, and will need a thorough review.

@dsnopek dsnopek changed the title [DRAFT] JavaClassWrapper: Improve handling of array arguments JavaClassWrapper: Improve handling of array arguments Feb 13, 2025
@dsnopek dsnopek force-pushed the java-class-wrapper-array-improvements branch from 19b3254 to 874e835 Compare February 14, 2025 14:37
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 14, 2025

After sleeping on this, I've decided to remove the explicit varargs support, and just treat Java varargs methods as having a final array parameter (which is contrary to what I originally wrote here).

While implementing Java varargs as GDScript varargs makes for more idiomatic GDScript code, it creates problems with selecting the correct overridden method, since varargs in GDScript are untyped whereas in Java they are typed. This would make it impossible to distinguish between these two methods from GDScript:

        @JvmStatic
        fun testMethod(int: Int, vararg args: String): String {
            return "String varargs: " + args.contentToString()
        }

        @JvmStatic
        fun testMethod(int: Int, vararg args: Int): String {
            return "Int varargs: " + args.contentToString()
        }

... whereas, if we treat that final argument as a normal array, you can use either Array[String] or Array[int] in GDScript to target one or the other.

So, that's how it works now in my latest push! (I still have previous varargs approach in a branch, in case we want to go back to it.)

This also makes the PR much smaller and easier to review, perhaps making it more likely we could sneak it in for Godot 4.4?

cc @m4gr3d @syntaxerror247

@dsnopek dsnopek changed the title JavaClassWrapper: Improve handling of array arguments JavaClassWrapper: Improve handling of typed array arguments Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using JavaClassWrapper, calling a method with multiple overloaded parameters will cause an error.
2 participants