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

Exported list property not properly visible in Editor #603

Open
gabryon99 opened this issue Apr 8, 2024 · 6 comments
Open

Exported list property not properly visible in Editor #603

gabryon99 opened this issue Apr 8, 2024 · 6 comments

Comments

@gabryon99
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@MartinHaeusler
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

CedNaru commented Apr 12, 2024

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
Copy link
Contributor

chippmann commented May 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants