Bind Array and Packed*Array get and set functions

[aaronfranke]

trafficstars

These must be bound to ensure feature parity between both Array and Packed*Array, in-engine and GDExtension. EDIT: Also added set to Dictionary for consistency there too.

Current situation:

	In Engine	GDExtension & scripting
Array [] Get	✅	✅
Array [] Set	✅	✅
Array Func Get	✅	❌
Array Func Set	✅	❌
Packed*Arrays [] Get	✅	✅
Packed*Arrays [] Set	❌	✅
Packed*Arrays Func Get	✅	❌
Packed*Arrays Func Set	✅	✅

With this PR (assuming GDExtension C++ extension API JSON is updated):

	In Engine	GDExtension & scripting
Array [] Get	✅	✅
Array [] Set	✅	✅
Array Func Get	✅	✅
Array Func Set	✅	✅
Packed*Arrays [] Get	✅	✅
Packed*Arrays [] Set	❌	✅
Packed*Arrays Func Get	✅	✅
Packed*Arrays Func Set	✅	✅

This means that you can safely use .get() and .set() and have it be compatible with everything. Before this PR, you would have to use a mish-mash of the [] operator and the functions in order to make it work in both places (you'd have to use [] most places but you'd have to use the function for Packed*Arrays setting). I'm labeling as an enhancement because it could be worked around before but making this consistent will help out a lot.

For Array Func Get/Set, it was a simple matter of adding two bind_method lines and then writing the docs.

For Packed*Arrays Func Get in GDExtension, I had to add an extra shim function in the bindings to resolve this ambiguity:

more than one instance of overloaded function "vc_method_call" matches the argument list:C/C++(308)
variant_call.cpp(2393, 2): function template "void vc_method_call(R (T::*method)(P...), Variant *base, const Variant **p_args, int p_argcount, Variant &r_ret, const Vector<Variant> &p_defvals, Callable::CallError &r_error)" (declared at line 56)
variant_call.cpp(2393, 2): function template "void vc_method_call(R (T::*method)(P...) const, Variant *base, const Variant **p_args, int p_argcount, Variant &r_ret, const Vector<Variant> &p_defvals, Callable::CallError &r_error)" (declared at line 61)
variant_call.cpp(2393, 2): argument types are: (<unknown-type>, Variant *, const Variant **, int, Variant, const Vector<Variant>, Callable::CallError)

For the case of Packed*Array [] Set in engine: The only value returned by [] is const so it can't be used for setting. I'm not sure if I should change this so I've left it as-is in this PR. The error it gives if you try:

no operator "=" matches these operands C/C++(349) operand types are: const Color = Color

[aaronfranke]

Actually, backporting this to 3.x may fix a bug report I opened a long time ago: https://github.com/godotengine/godot/issues/40046

[Bromeon]

Before this PR, you would have to use a mish-mash of the [] operator and the functions in order to make it work in both places (you'd have to use [] most places but you'd have to use the function for Packed*Arrays setting).

Could you elaborate? Why couldn't one use this operator for packed array setting?

https://github.com/godotengine/godot/blob/99a7a9ccd60fbe4030e067b3c36d54b67737446d/core/extension/gdextension_interface.h#L2038-L2049

And for arrays, these? Plus the equivalent const versions for getting?

https://github.com/godotengine/godot/blob/99a7a9ccd60fbe4030e067b3c36d54b67737446d/core/extension/gdextension_interface.h#L2298-L2309

[aaronfranke]

@Bromeon This code doesn't compile:

PackedInt32Array arr;
arr.resize(5);
arr[0] = 7;

error: cannot assign to return value because function 'operator[]' returns a const value
        arr[0] = 7;
        ~~~~~~ ^
./core/templates/vector.h:97:23: note: function 'operator[]' which returns const-qualified type 'const int &' declared here
        _FORCE_INLINE_ const T &operator[](Size p_index) const { return _cowdata.get(p_index); }
                             ^~~

[Bromeon]

But this is something to be fixed in the C++ code... where does GDExtension come into play here?

It seems both mut/const index operators are already exposed, for both packed and normal arrays?

[aaronfranke]

@Bromeon As per the table in the OP, [] works in all cases in GDExtension, and the get/set methods work in all cases in the engine. I am trying to write code that works both in GDExtension and in the engine.

[Bromeon]

Sorry if I keep asking stupid questions :grimacing: maybe I'm missing something about the inner workings, but why can the missing get/set not be implemented via [] if that one is present? Or is it mostly because of the way how godot-cpp generates code, and it would feel more "natural" to have auto-generated get/set without special-casing it?

Because functionally, the API in GDExtension is already present, so duplicating it may raise more questions than it answers -- from the perspective of someone who writes a new binding. Or not?

[aaronfranke]

@Bromeon Sorry, what do you mean by implemented? get/set are already implemented, this PR exposes it. If Godot and godot-cpp don't expose get/set, you can't call them. godot-cpp doesn't generate get/set from [].

If you mean using [] instead of get/set, the [] operator doesn't work for setting on packed arrays in engine code, as mentioned above. So you can't use [] everywhere. And without this PR, you also can't use get/set everywhere.

Anyway, the stated goal of GDExtension is to allow code to be written similarly to engine code, extending the engine. This was the whole point of rebranding it from the old name GDNative. This PR brings us closer to that goal.

[dsnopek]

@aaronfranke I think the point that @Bromeon is trying to make, is that we don't necessarily need to bind Array's get() and set() functions in order for godot-cpp to have them, because we could manually write an implementation in godot-cpp that uses the already existing support for Array's [] operator which can both get and set values. We already do manually write some functions, so this wouldn't be totally out of place - this is something we could do. It depends on if we want to prioritize minimizing the interface or not. Personally, I could go either way.

This doesn't apply to the functions that are in GDExtension but missing Godot, though - those either need to be added to Godot or removed from godot-cpp in order to have parity.

[akien-mga]

So we're happy to merge as is? Needs a rebase.

[akien-mga]

Thanks!