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

VariantArrays received from Godot don't have the correct generic type. #705

Open
CedNaru opened this issue Sep 29, 2024 · 0 comments
Open

Comments

@CedNaru
Copy link
Member

CedNaru commented Sep 29, 2024

When sending a VariantArray from Godot to the JVM, we simply send the pointer to the native object. Once the JVM read the pointer from the buffer, it creates a VariantArray.
It's an issue for 2 things, virtual method callbacks implemented in script and used by Godot and registered properties.
In both case, the Kotlin code expect to receive a VariantArray that use the correct type.

As a reminded, the generic parameter of a VariantArray is not just an erased type. We actually use its class to obtain the correct VariantConverter:

@Suppress("unused", "UNCHECKED_CAST")
class VariantArray<T> : NativeCoreType, MutableCollection<T> {

    internal var variantConverter: VariantConverter

    @PublishedApi
    internal constructor(handle: VoidPtr, converter: VariantConverter) {
        variantConverter = converter
        _handle = handle
        MemoryManager.registerNativeCoreType(this, VariantParser.ARRAY)
    }
}

That VariantConverter matters when using the class whenever we want to set/get a value in that VariantArray. When it happens we use the buffer to communicate the value and for that we need the correct converter so it behaves properly.
Sadly, all the VariantConverted received contains the Any converter. While it shouldn't cause bugs, as far as I investigated, it will degrade performances (already bad with VariantArray and Dictionary). The Any converter is flexible, but it has to check the class of each parameter sent and fetch the "true" converter from it, instead of doing the conversion directly when using another Converter.

I first tried to fix it by sending the type of the Array though the buffer and create the correct VariantArray instance from it. But it can only works for callbacks, not for registered properties.
The reason for it is that we allow a wide selection of type to use as generic parameters in a VariantArray, which include using narrow types like Int or Byte. The issues are that the VariantArray in native Godot only know about the raw VariantType, which only include Long for integers.
So if a script export a VariantArray<Byte> and a value is set for it in the inspector, Godot will only see a VariantArray. But the Long converter is not like Any, it will try to take a Byte value and use it like if it was a Long without any other check which will result in a InvalidTypeException.

To solve that part, we will need to change the registration code so it does the KtProperty created does the conversion automatically for us instead of the VariantConverter. It goes back to an old conversation we had about how we should properly handle "true" godot types + converted types. I suggest thinking about it alongside #692 as it impacts the same code.

@CedNaru CedNaru changed the title VariantArray received from Godot don't have the correct type. VariantArray received from Godot don't have the correct generic type. Sep 29, 2024
@CedNaru CedNaru changed the title VariantArray received from Godot don't have the correct generic type. VariantArrays received from Godot don't have the correct generic type. Sep 29, 2024
@utopia-rise utopia-rise deleted a comment Sep 29, 2024
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

No branches or pull requests

1 participant