utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
562 stars 38 forks source link

Exported list property not properly visible in Editor #603

Open gabryon99 opened 3 months ago

gabryon99 commented 3 months ago

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

MartinHaeusler commented 3 months ago

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.

chippmann commented 3 months ago

Yeah. I'm unsure what to do here;

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

MartinHaeusler commented 3 months ago

@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.

piiertho commented 3 months ago

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.

CedNaru commented 3 months ago

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.

chippmann commented 2 months ago

@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.