godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
86.99k stars 19.51k forks source link

[GDNative] Misleading godot_x_new_copy/operator_equal functions for array/dictionary/pool arrays #35542

Open touilleMan opened 4 years ago

touilleMan commented 4 years ago

I order to copy an array (same thing for a dictionary and pool array) you should use godot_array_new_copy array... but if you want to copy the array you should use godot_array_duplicate ! :trollface:

The difference comes from the fact godot_array is a cheap structure pointing on a ref counted vector. Using godot_array_new_copy only copy the cheap structure where godot_array_duplicate also copy the vector. In the first case we end up with a single array referenced by two godot_array variables, in the second we have two totally different arrays each referenced by their own godot_array variable.

This is even worst for pool arrays given they don't provide duplicate function, so user reading the api is really enclined to think godot_pool_x_array_new_copy is the way to do a duplication !

I personally got bitten by this multiple times, so I guess we should try to clarify a bit the naming here.

I guess renaming godot_x_new_copy into godot_x_new_reference would be enough, what do you think ?

touilleMan commented 4 years ago

There is a similar issue with godot_x_operator_equal given it doesn't actually do internal values comparison but only compare if the underlying refcounted structure holding the values is the same.

So we could also rename godot_x_operator_equal into godot_x_references_equal (and also maybe provide a real godot_x_operator_equal that use value comparison ?)

touilleMan commented 4 years ago

35816 tries to tackle on the godot_dictionary_operator_equal

KoBeWi commented 3 years ago

Can anyone still reproduce this bug in Godot 3.2.3 or any later release? (although it doesn't look like a bug...)