JavaClassWrapper: Improve handling of typed array arguments

[dsnopek]

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]

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]

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

[syntaxerror247]

The code looks good to me. I prefer the current approach (treating Java varargs methods as having a final array parameter), but I'd like to hear @m4gr3d's preference and leave the approval to him.

[m4gr3d]

Looks great!

The changes are small and scoped enough they should be safe to include in 4.4. Leaving it up to @akien-mga to decide.

[m4gr3d]

We should check that c is valid before proceeding.

[dsnopek]

We would have checked this earlier on line 224.

This process goes in two phase: a validation phase, where it reports errors about any of the provided arguments, and then a calling phase where (if there were no validation errors) it marshals the data into the Java types and actually calls the method. This would have gotten caught in the validation phase.

However, it's no problem to add if (c) { ... }, so I have done that in my latest push :-)

[dsnopek]

The changes are small and scoped enough they should be safe to include in 4.4.

The ship may have sailed on Godot 4.4, but it'd fine for this to end up in 4.4.1.

[dsnopek]

In my latest push, I added one of the same efficiency improvements as I did in PR #103375, where JNI fills some the arrays (the ones where the types match) rather than us going element by element. I have a test project that comprehensively tests all the conversion branches in this one, so I'm pretty confident about this update!

[m4gr3d]

Does the copy logic not work for short?

[dsnopek]

Not with the way that I decided to convert from PackedInt32Array: it's set up to treat each item in the PackedInt32Array as representing a single short.

If we decided that each item in the PackedInt32Array represented 2 shorts (with one of them shifted two bytes to the left) then we could copy directly. However, that didn't seem like the most friendly for developers - my guess is that they would assume it was the former.

Although, that was just a judgement call I made! If anyone thinks otherwise, let's discuss :-)

[dsnopek]

Actually, thinking about this a little more, I don't think we actually could treat each item in PackedInt32Array as representing two shorts: how would you pass an array with an odd number of items? There'd be no way to indicate that the last element didn't exist.

I think the way I'm doing the conversion is really the only way to do it if we use PackedInt32Array. If we wanted to pack them tighter, we could use PackedByteArray, but that would be so tricky to use from GDScript.

I'm happy with the judgement call I made here: it optimizes for ease of use by the developer, not performance or lower memory usage. I don't think APIs that take short[] are super common anyway, so odds are this conversion won't get used that much

[syntaxerror247]

Yeah, I don't really remember the last time I used short[].

[Repiteo]

Thanks!

[akien-mga]

Cherry-picked for 4.4.1.