godot
godot copied to clipboard
Add list initialization to Array, Variant, and TypedArray
- 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
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.
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.
[...] 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 Array
s in Variant
more explicit (Variant v = Variant(Array({...});
)?
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' };
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 [...]
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.
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?