Pool*Array instances should be references and not values in GDscript

[ficoos]

Operating system or device - Godot version:

Issue description:
Calling PoolStringArray.push_back() sometimes silently does nothing.

Steps to reproduce:

extends Node

class B:
	static func do_something_else(a):
		a.arr.push_back("I'm B")

class A:
	var arr = PoolStringArray()

	func do_something():
		arr.push_back("PRE")
		B.do_something_else(self)
		arr.push_back("POST")
		print(arr)

func _ready():
	var a = A.new()
	a.do_something()

outputs [PRE, POST] instead of [PRE, I'm B, POST]

Link to minimal example project:

[ficoos]

OK, did some digging the problem seems to be that void Variant::reference(const Variant &p_variant) just doesn't work as advertised. It actually invokes copy constructors.
The only reason it works is because types are either immutable (int, float) don't have methods that modify them just members (this doesn't create a new variant) (Vector2) or have copy constructors that don't copy but create reference (String, Dictionary).
PoolVector actually does COW (regular Vector doesn't) so during resize it copies the internal data in the copy.

I think @reduz needs to way in on this.
IMHO I don't think that copy constructors should not be used like this. This creates a lot of places that do reference count for their internal state. This is why smart pointers exist.

[ficoos]

OK, After understanding a bit more.
The issues is that Pool*Array are native Variant types (note Object or Ref) this means that they are values. This means that they are copied on assignment (like Vector, Int, String, ...)

I don't think it's they should be values for them as Array and Dictionary are not Values.
Changing the title to reflect this

[Zylann]

Just found this oddity as well, thankfully I was aware of other COW things but I thought they were due to be removed in GDScript for 3.0?

In Godot 2.1.3 and 3.0 master, the following test is a fail:

func _ready():
	test()

static func lol(a):
	a.resize(42)

static func test():
	var a = PoolVector3Array()
	lol(a)
	assert(a.size() == 42)
	print("Test succeeded ", a.size())

@reduz any way of dealing with this?

[reduz]

those are often sent to other threads and require locking for manipulation, so making them share data can lead to unexpected behavior. A solution may be to use an internal variant reference counter and making them shared only in GDScript..

[Zylann]

What about GDNative scripts that do mesh generation? Those also have to give PoolArrays back to Godot, but they can't lock them for performance AFAIK.

[reduz]

Zylann It should be perfectly possible to lock them in GDNative, would need to ask Karroffel to implement it

[reduz]

Still undecided on this, so kicking to 3.1

[garyo]

I've run afoul of this as well, just as a data point. Exposing a lock/unlock method so you can get mutable references in GDscript/GDNative would be one way? Seems pretty important to be able to get a reference to a PoolArray somehow.

[Zylann]

Read/write locks have been exposed to GDNative, but GDScript still doesn't have it (it's important to remember that even if GDScript doesn't touch them, it may pass them around quite often!)

[reduz]

I still think these should be reference counted when inside of variant and shared (but not by themselves when passing to a function). This is considerable amount of work to change, though so for now will kick to 3.2 unless anyone wants to do it.

[ghost]

Also ran into this with dictionaries.

extends Node2D

func _ready():
	
	var dict = {
		vecs = PoolVector2Array()
	}
	
	print(dict.vecs)
	dict.vecs.append(Vector2())
	print(dict.vecs)

[reduz]

This is scheduled to change after 3.1 is out.

…

On Thu, Jan 24, 2019, 10:39 avencherus ***@***.*** wrote: Also ran into this with dictionaries. extends Node2D func _ready(): var dict = { vecs = PoolVector2Array() } print(dict.vecs) dict.vecs.append(Vector2()) print(dict.vecs) [image: image] <https://user-images.githubusercontent.com/13004169/51681723-2e3df280-1fee-11e9-9606-3699934aad39.png> — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8409 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z25Rc3yD47e4TxhGbutPHqkK5Ddn2ks5vGbecgaJpZM4M90F6> .

[Calinou]

Is this still an issue with Packed*Array? cc @reduz

[akien-mga]

Yes this is fixed by #36311.

[Xrayez]

In some cases this doesn't work:

func _ready():
	var p2d = Polygon2D.new()
	var polygon = PackedVector2Array([Vector2(0, 0), Vector2(100, 0), Vector2(100, 100), Vector2(0, 100)])

	p2d.polygon = polygon
	print(p2d.polygon)
	p2d.polygon.push_back(Vector2(213, 123))
	print(p2d.polygon) # same as `polygon`

#	var poly_copy = polygon
#	poly_copy.push_back(Vector2(213, 123))
#	p2d.polygon = poly_copy

But It does work with dictionaries and other Variant containers as described above.

[sarchar]

I have stumbled upon a similar bug and curious if this is also related:

func _ready():
	var arr = [1, 2, 3, 4]
	var pba = PoolByteArray(arr)
	var buf = StreamPeerBuffer.new()
	buf.data_array = pba
	var data = buf.get_data(4)
	print(data[1].hex_encode())
	data[1].invert()
	print(data[1].hex_encode())

prints:

01020304
01020304

however,

func _ready():
	var arr = [1, 2, 3, 4]
	var pba = PoolByteArray(arr)
	var buf = StreamPeerBuffer.new()
	buf.data_array = pba
	var data = buf.get_data(4)
	var copy = data[1]
	print(copy.hex_encode())
	copy.invert()
	print(copy.hex_encode())

prints

01020304
04030201