Change Array.erase API to return true iff successful

[lowagner]

Describe the project you are working on

i'm working on a 3d metroidvania with ball rolling mechanics (https://bsky.app/profile/lowagner.bsky.social).

Describe the problem or limitation you are having in your project

the Array API has erase(value: Variant): void, but it'd be nice to return the success value of this method (i.e., as a bool). the Dictionary API does something similar for erase(key: Variant): bool, returning true if successful and false if not.

the problem space is having a "named stack" to push/pop from when changing physics parameters (e.g., gravity_scale). if you "dash", you want to scale gravity to zero; if you're doing a "jump", you want to scale gravity in a different way. and that's fine if dash and jump are mutually exclusive, but now if you dash into a boost area, you want to keep track of which gravity adjustments come from where so you don't reset gravity_scale to zero after dashing into a boost (because boost sees gravity_scale as 0 from the dash, and assumes that's what the player wants to return to after getting boosted). here's a relevant slice of the API:

class_name NamedStack
extends Resource

var _stack_names: Array[String] = []
var _stack_values := Dictionary() # [String, Variant]

func _init(callable := func(_ns: NamedStack): pass):
	callable.call(self)

func push(name: String, value):
	if _stack_values.has(name):
		return false
	_stack_names.append(name)
	_stack_values[name] = value
	return true

## returns true if successful
func pop(name: String) -> bool:
	var index := _stack_names.find(name)
	if index < 0:
		return false
	_stack_names.pop_at(index)
	if not _stack_values.erase(name):
		push_error("broken invariant, _stack_values should have had key %s" % name)
	return true

how to get the most recent value (or even a list of all active values) is left as an exercise to the reader.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

adding a bool return value to the Array.erase API means that we can switch this logic:

func pop(name: String) -> bool:
	var index := _stack_names.find(name)
	if index < 0:
		return false
	_stack_names.pop_at(index)
	if not _stack_values.erase(name):
		push_error("broken invariant, _stack_values should have had key %s" % name)
	return true

to this logic:

func pop(name: String) -> bool:
	if not _stack_names.erase(name):
		return false
	if not _stack_values.erase(name):
		push_error("broken invariant, _stack_values should have had key %s" % name)
	return true

which is IMO more readable and clear.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

this should be a very straightforward change to the Array API: https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/variant/array.h#L162

the internal vector implementation already returns true/false for success/failure:
https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/templates/vector.h#L80-L87

so we just need to return it here: https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/variant/array.cpp#L329

bool Array::erase(const Variant &p_value) {
...
   return _p->array.erase(value);
}

there's probably some similar logic to fix in C# as well, though i'm not a C# user.

i don't know if there's anything we need to do to the binding API: https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/variant/variant_call.cpp#L2355

but maybe the bind_method macro is taking care of that.

If this enhancement will not be used often, can it be worked around with a few lines of script?

it can be worked around with a few lines of code, but the lines of code feel very verbose and less clear in intent than a simple return array.erase(value).

var index := array.find(value)
if index < 0:
	return false
array.pop_at(index)
return true

Is there a reason why this should be core and not an add-on in the asset library?

it's already present in the underlying vector API, so we're just exposing it to the GDScript API.

[Calinou]

Changing the return value from void to non-void will break GDExtension compatibility at least. Script compatibility is probably not affected as the return value was usually discarded.

[lowagner]

that seems kinda brittle on the GDExtension side -- @Calinou is there a way to make this change and make it less likely for future API changes (e.g., from void to non-void) to break GDExtension compatability?

not that i expect this to happen a lot, but if we feel like it's an API improvement we shouldn't need to worry too much about breaking compatibility...

[AThousandShips]

We can handle compatibility breaking in a number of areas (though I don't know if these specific methods can register compatibility methods, they didn't use to be able to at least, only for Object types)

But that doesn't mean it's safe to do so necessarily, GDExtension can be used with various languages some of which might not deal with this neatly (they might not allow return value discarding for example) so compatibility is important to maintain compatibility, it's not something we should do lightly

[KoBeWi]

it can be worked around with a few lines of code, but the lines of code feel very verbose and less clear in intent than a simple return array.erase(value).

You can wrap these lines in a method, so it becomes e.g. if Utils.erase_from_array(array, element):.

[lowagner]

having a consistent/similar API between containers like Array and Dictionary is still nice in the long run, though. and it's exposed internally so it's just making that available externally as well.

are there some docs to read up on for the compatibility concerns that you mention? I thought GDExtension was mostly C++.

[AThousandShips]

and it's exposed internally so it's just making that available externally as well.

No it is not, Array.erase returns void: void erase(const Variant &p_value);

GDExtension is not just C++, it's for a number of different language extensions, see GDExtension

But this would require implementing a whole new system for handling compatibility breakage that we don't need otherwise, which is quite a lot of additional work, there's no way to bind compatibility methods to Array currently, which would be necessary for this to be possible in the first place (in 4.x that is)

We have such a system for Object but not for Array or other methods bound in the core system

[lowagner]

and it's exposed internally so it's just making that available externally as well.

No it is not, Array.erase returns void: void erase(const Variant &p_value);

mentioned in the OP:

this should be a very straightforward change to the Array API: https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/variant/array.h#L162

the internal vector implementation already returns true/false for success/failure:
https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/templates/vector.h#L80-L87

but maybe one more link: Array.cpp uses Vector internally, so you'd just need to return this result here: https://github.com/godotengine/godot/blob/3f56b3b23916ff54a1715cb8444304ce3c50283d/core/variant/array.cpp#L329

sounds like it would be hard to do for 4.x (unless we add compatibility methods to Arrays -- would there be other reasons for that to be a feature request?). would there be any desire for it in 5.x?