godot-kotlin-jvm icon indicating copy to clipboard operation
godot-kotlin-jvm copied to clipboard

Exported list property not properly visible in Editor

Open gabryon99 opened this issue 1 year ago • 6 comments

If the user creates a new exported list property in a class, this won't be seen in the editor as such. For example, the following code causes this behavior:

@RegisterClass
class LevelManager {
    @Export
    @RegisterProperty
    var levels: List<Int> = listOf()
    // Other properties...
}

image

While, using VariantArrays, everything works as expected:

@RegisterClass
class LevelManager {
    @Export
    @RegisterProperty
    var levels: VariantArray<Int> = VariantArray()
    // Other properties...
}

image

gabryon99 avatar Apr 08 '24 10:04 gabryon99

As far as I know, it has to be VariantArray specifically because that's the only type that is understood by Godot. Kotlins List interface is unknown on the Godot side.

MartinHaeusler avatar Apr 10 '24 20:04 MartinHaeusler

Yeah. I'm unsure what to do here;

  • either we implicitly register these collections as a VariantArray under the hood -> imposes performance problems as lists need to be converted to variant array and back each time which is a very inefficient operation as each entry needs to be copied one by one with each entry creating a jni call
  • Do not allow these to be registered (but needs to be allowed for enum BitFlags and the likes)
  • Do nothing and leave as is

Personally i think the "right" approach would be to prevent registration of collections which are not VariantArrays.

chippmann avatar Apr 11 '24 06:04 chippmann

@chippmann While I don't think that this is critical at all (just sticking to VariantArray is perfectly acceptable to me), a thought crossed my mind. It may be completely stupid (I'm not too familiar with the internals of the native interface).

Could it be an option to let the class VariantArray implement the kotlin.List<T> (or perhaps even kotlin.MutableList<T>) interface? That way, if we receive a VariantArray from Godot (like we do now already) we could assign it to a property of type kotlin.List<T> directly without further modification. A real conversion from List<T> to VariantArray<T> would then only be necessary if the programmer manually assigns a concrete list instance (e.g. java.util.ArrayList) to the property and it needs to be passed from Kotlin to C++; as long as the value is defined in the C++ / Godot Editor side (which I suspect is the primary case) nothing needs to be converted at all, and kotlin developers can keep using their familiar kotlin.List interfaces.

I just wanted to put the idea out there.

MartinHaeusler avatar Apr 11 '24 17:04 MartinHaeusler

There is a problem with creating an auto conversion: Let's imagine someone registers an ArrayList to engine, then engine resends back a list to put in this variable. As engine only knows VariantArray, this would change the type of the instance stored in property.
I think this lead to side effects that can be pretty hard to debug.

piiertho avatar Apr 12 '24 06:04 piiertho

Since 4.0, the engine know the type inside a VariantArray, it's actually the only case of generic handled by Godot.

But I agree that using List is tricky for performances. Cedric mentioned we have this feature to handle bitfelds. But in that case, I would rather remove this weird List hack and create a proper GodotBitField type instead.

CedNaru avatar Apr 12 '24 15:04 CedNaru

@chippmann While I don't think that this is critical at all (just sticking to VariantArray is perfectly acceptable to me), a thought crossed my mind. It may be completely stupid (I'm not too familiar with the internals of the native interface).

Could it be an option to let the class VariantArray implement the kotlin.List<T> (or perhaps even kotlin.MutableList<T>) interface? That way, if we receive a VariantArray from Godot (like we do now already) we could assign it to a property of type kotlin.List<T> directly without further modification. A real conversion from List<T> to VariantArray<T> would then only be necessary if the programmer manually assigns a concrete list instance (e.g. java.util.ArrayList) to the property and it needs to be passed from Kotlin to C++; as long as the value is defined in the C++ / Godot Editor side (which I suspect is the primary case) nothing needs to be converted at all, and kotlin developers can keep using their familiar kotlin.List interfaces.

I just wanted to put the idea out there.

@MartinHaeusler Sorry completely forgot this reply of yours. VariantArray already implements MutableCollection<T>. So your case should already work today. And i think you are right, we might just be able to register it under the hood as variant array without there being a perf penalty (directly) but the hidden cost of the list being a variantarray instead of being a "real" kotlin list type still remains and IMO still is dangerous as it still is not nearly as performant as a kotlin collection and might go unnoticed.

Still definitely worth a consideration.

Regarding the Bit field case; i agree with @CedNaru here. We should remove these hacks and create dedicated types for them. After that we can talk about implicit collection conversions in the registration. But as long as these hacks remain, this endeavor is futile.

chippmann avatar May 13 '24 20:05 chippmann