godot icon indicating copy to clipboard operation
godot copied to clipboard

Add list initialization to Array, Variant, and TypedArray

Open kitbdev opened this issue 1 year ago • 1 comments

  • closes https://github.com/godotengine/godot-proposals/issues/1357 (unless any other types need it)
  • salvages #50504
  • related #50493

Arrays use Vector list initialization to set its internal Vector. Variant becomes an array type. TypedArrays<> must have a type to use the list initialization.

Added initializer list tests for Array, TypedArray, Variant, and PackedInt32Array. Packed Arrays are typedefs for Vector, so they had this functionality before.

I didn't change any code to use initializer lists in this PR. In another PR, I think build_array() can be replaced with initializer lists in a few test files.

kitbdev avatar Dec 10 '23 23:12 kitbdev

Thanks for working on this @kitbev. I think this would make working with Arrays in the engine code much nicer. My only comment so far is whether we would rather have the argument be an initializer list or a Vector<Variant>. Note that having both would be an issue I think because then an expression like Array({1, 2}) becomes ambiguous about whether it invokes the initializer list constructor or the vector constructor. I would argue in favor of the Vector<Variant> route (unless there are any performance reasons I'm not considering) because there are other places in the codebase where it would be useful to directly convert a vector to an array.

nlupugla avatar Apr 28 '24 18:04 nlupugla

[...] an expression like Array({1, 2}) becomes ambiguous about whether it invokes the initializer list constructor or the vector constructor [...]

Would making the Vector constructor explicit avoid it being a candidate in such an expression?

Aside, I like this kind of changes to take Godot's C++ to the 21st century. I'm not sure about adding this kind of constructor to Variant. What are your thoughts in keeping initializing Arrays in Variant more explicit (Variant v = Variant(Array({...});)?

RandomShaper avatar May 02 '24 09:05 RandomShaper

Rebased to fix merge conflict.

vector constructor

Variant already has constructors for Packed*Array and a few other Vectors including Vector<Variant>, so Array doesn't need its own constructor for it.

What are your thoughts in keeping initializing Arrays in Variant more explicit

Personally, I don't think it is needed and I like the ability to nest initializer lists for Array and Variant without it. The Variant initializer list was made just for this purpose. If we decide it would be better, I could remove it and make it explicit.

Array nested = { { 0, 1 }, 'a' };
Array nested2 = { Array({ 0, 1 }), 'a' };

kitbdev avatar May 02 '24 20:05 kitbdev

I'm not sure about the status of this conern:

[...] an expression like Array({1, 2}) becomes ambiguous about whether it invokes the initializer list constructor or the vector constructor [...]

RandomShaper avatar May 06 '24 09:05 RandomShaper

Personally, I'd prefer the explicitness, but it might be an odd outlier if that's already how it works for some of the other types.

nlupugla avatar May 06 '24 13:05 nlupugla

I'm not sure about the status of this conern: [...] an expression like Array({1, 2}) becomes ambiguous about whether it invokes the initializer list constructor or the vector constructor [...]

Array doesn't have a vector constructor, so there is no issue. And I'm pretty sure it would use the initializer list in that case if it did.

As for Variant, where there is an initializer list constructor and vector constructors (including Packed*Arrays since they're vector typedefs):

Variant({ 0.0, 1.0 }); // Variant uses initializer_list constructor.
Variant(Vector({ 0.0, 1.0 })); // Variant uses PackedFloat64Array constructor.

Variant({ 0.0, "b" }); // Variant uses initializer_list constructor.
Variant(Vector<Variant>({ 0.0, "b" })); // Variant uses Vector<Variant> constructor.
Variant(Array({ 0.0, 1.0 })); // Variant uses Array constructor.
Variant(PackedFloat32Array({ 0.0, 1.0 })); // Variant uses PackedFloat32Array constructor.

So the initializer list constructor gets priority. Previously, it would fail to compile with more than one instance of constructor "Variant::Variant" matches the argument list:....

The first two are actually not the same since they have different Variant types, so they won't be equal. The first is type Array and the second is type PackedFloat64Array. So maybe it should be explicit? Or some more logic should be done in the Variant initializer list constructor to make it a packed type? Or maybe it is fine as is, since it is the same as using the Array constructor?

kitbdev avatar Sep 03 '24 16:09 kitbdev