Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exported (non-static) Arrays are not duplicated when instancing. #16478

Open
sturnclaw opened this issue Feb 7, 2018 · 22 comments
Open

Exported (non-static) Arrays are not duplicated when instancing. #16478

sturnclaw opened this issue Feb 7, 2018 · 22 comments

Comments

@sturnclaw
Copy link
Contributor

sturnclaw commented Feb 7, 2018

Godot version: Godot 3.0

OS/device including version: Solus Linux / X11

Issue description: When exporting an array variable in GDScript, the array is not duplicated upon instancing of the node. This behavior conflicts with all other properties, and is contrary to the OOP model Godot uses. The array is not static (or class-wide), but belongs to the instance, and so every instance's copy should be unique. Currently, unless manually duplicated, modifications to the array affect all instances of the class.

This issue may be present with Dictionaries as well, I have not tested them.

Steps to reproduce: Write this script:

extends Node

export var test = []

# This way too, it's not bound to explicit array constructor
# export(Array) var test

func _ready():
    test.append("test %s" % self)
    print(test) 

Save a node with the script a as scene, instance two copies at runtime, and add them to the tree.

Expected result:

["test Node:XXXX"]
["test Node:YYYY"]

Result:

["test Node:XXXX"]
["test Node:XXXX", "test Node:YYYY"]

Minimal reproduction project: See above.

@letheed
Copy link
Contributor

letheed commented Feb 7, 2018

This is the expected behavior according to the doc. Though I don't know the reason for that.

@sturnclaw
Copy link
Contributor Author

sturnclaw commented Feb 8, 2018

Ahh, forgot about that bit of the doc. Still think it's backwards and could be improved, so I'll leave this open for discussion.

@letheed
Copy link
Contributor

letheed commented Feb 8, 2018

Yeah, there may be a good reason for it, but I agree it's definitely surprising. I wonder if it would be possible to find more use for the static keyword. GDScript is very much a class language. Would make sense to have static variables.

@RabbitB
Copy link

RabbitB commented Feb 9, 2018

I would agree that this behavior is something that should be reserved for static variables. Although the current behavior is documented, it runs contrary to what's expected by most people, and how it would function in any other OO language I can think of.

@MutantOctopus
Copy link

I have to agree with this. I understand, in a general sense, why this occurs (Arrays are a standard datatype, not a Resource, and are handled differently by the engine), but it's incredibly unintuitive and makes exported arrays have very limited utility.

The particular example where I ran into this issue is a breakout clone; Bricks have a "layers" array which stores the textures for each level of brick as it gets hit (red -> yellow -> green -> purple -> blue -> destroyed). I have two sets of brick sprites (regular and glossy), and intended to have two versions of the brick scene, one with the regular version and one with the glossy version, so as that I could swap the scenes if I decided I preferred one or the other.

It would seem like a tidy solution to this issue would be to create a Resource wrapper for Arrays, allowing you to save a .res or .tres that contains the array and could be slotted into the Inspector, but I wouldn't have any idea how to go about implementing that.

@sturnclaw
Copy link
Contributor Author

Additionally, and ancillary to this, when instancing a node in the editor with an exported array on it, the array is not duplicated, leading changes to the array on one (child) node to be applied across all instances and even the original scene. This is an unacceptable behavior, for very obvious reasons.

@williamd1k0
Copy link
Contributor

This behavior conflicts with all other properties, and is contrary to the OOP model Godot uses.

I can't see what exactly is your point because it's just an expected behavior in OOP.

You can try to reproduce it using Python.

image

You can "avoid" this behavior initializing the array in the init/ready method.

image

In the export var case, you can just leave the variable empty (null), so in the following code the array will be unique:

extends Node

export(Array) var test

func _ready():
	if test == null: # if the array is "empty" in the inspector it will be kept as null
		test = []
	test.append("test %s" % self)
	print(test)

However, I think it's an inconsistency in gdscript implementation because both var and const should share objects created before initialization. In this case only const is being shared.

@rminderhoud
Copy link
Contributor

I just tested @williamd1k0's method and it does indeed work. Explicitly constructing the array in the _ready() function seems to be the magic.

I have the following

|- Spatial
|--- Spatial (+script) [Unique: Array set in editor, Not Unique: Array set in editor]
|--- Spatial (+script) [Unique/Not Unique: Null in editor]
extends Node

export (Array) var unique;
export (Array) var not_unique = [];

func _ready():
	if unique == null: 
		unique = [];
	
	if not_unique == null:
		not_unique = [];
		
	unique.append("test %s" % self);
	not_unique.append("test %s" % self);
	print("==Start Node==");
	print(unique);
	print(not_unique);
==Start Node==
[abc, 1a3, awd, test [Spatial:963]]
[Not Unique, test [Spatial:963]]
==Start Node==
[test [Spatial:964]]
[test [Spatial:964]]

@bojidar-bg
Copy link
Contributor

Likely will be still unsolved after #17382, but that PR might be a step towards fixing it.

@Keetz
Copy link
Contributor

Keetz commented Mar 14, 2018

In 2.x the expected behaviour addressed here, was the actual behaviour.

Unfortunately I opened a duplicate of this, but take a look #17507

@sturnclaw
Copy link
Contributor Author

sturnclaw commented Mar 14, 2018

@williamd1k0: I can't see what exactly is your point because it's just an expected behavior in OOP. You can try to reproduce it using Python.

You are correct that this is an expected behavior in Python, but that's because variables declared with that syntax reside in the class, not the instance, and are initialized at declaration time.

What you describe is the realm of static member variables in almost all OO languages (C++, C#, D, etc.), which brings me to my next point; In GDScript, static member variables are explicitly disallowed:

A function can be declared static. When a function is static it has no access to the instance member variables or self. Ref

Static functions are allowed, but not static members (this is in the spirit of thread safety, since scripts can be initialized in separate threads without the user knowing). In the same way, member variables (including arrays and dictionaries) are initialized every time an instance is created. Ref

In GDScript, the correct behavior is that all variables are instance-local and should be initialized upon initialization of the script (i.e. just before the _init() method).

@bojidar-bg: Likely will be still unsolved after #17382, but that PR might be a step towards fixing it.

If it does what I think it does, it should fix half the problem.

The ideal solution (AFAIK) is to make instances have a copy-on-write property in the editor, and otherwise duplicate the value at runtime. This will allow changes to the parent scene's copy to properly propagate, while allowing every instance their own copy.

@Keetz
Copy link
Contributor

Keetz commented Mar 15, 2018

In GDScript, the correct behavior is that all variables are instance-local and should be initialized upon initialization of the script (i.e. just before the _init() method).

This!

I can't see why specifically array should work differently from everything else. All other exported variables and node specific properties (Size, Position and so on) are instance-local!

And again, this behaviour has changed since 2.x, what is the logic behind this change?

@raymoo
Copy link
Contributor

raymoo commented Mar 15, 2018

Well, the array variable is an instance-local reference to a shared array, so it's still technically instance-local

@akien-mga
Copy link
Member

Still valid in 3.2.3 and the current master branch (even in GDScript "2.0").

@cliedelt
Copy link

This design decision is just confusing. Why is every variable local except of arrays? Does not make any sense.

@Calinou
Copy link
Member

Calinou commented Mar 14, 2022

Related to #20436 and #48038.

@FeralBytes

This comment was marked as off-topic.

@akien-mga
Copy link
Member

This is fixed in 4.0, but still valid in 3.x.

@akien-mga akien-mga added the bug label Mar 31, 2023
@akien-mga akien-mga added this to the 3.x milestone Mar 31, 2023
@ricmzn
Copy link
Contributor

ricmzn commented Sep 29, 2023

If this was fixed in 4.0, it seems there was a regression at some point:
Screenshot_20230929_181410

extends Node

@export var array: Array[String]

func _ready():
	array.append(get_name())
	await get_tree().process_frame
	print("array in %s: " % get_name(), array)

<start> was added in the inspector in the NonEmptyArray scene to trigger the issue. I expected NonEmptyArray to print ["<start>", "NonEmptyArrayX"] instead of all the nodes, like in the EmptyArray case (sans <start>).

Full project: ArrayRegression.zip

If intentional, godotengine/godot-docs#5640 should be extended to 4.X docs as well.

Edit: the issue also happens in 4.2 dev 5

@DissonantVoid
Copy link
Contributor

Can confirm in 4.1.3. even while being documented earlier this behavior is a huge gotcha that feels out of place compared to how a user would expect a variable to behave.

Also I'd suggest updating the docs with a warning about this until it's fixed because as of now there is no mention of it which only adds to the confusion

@goblinJoel
Copy link

goblinJoel commented Jan 14, 2024

For comparison (on 3.5.3), this behavior for exported default array values is not the same as function default array arguments, which are a pretty similar idea. I agree with most in this thread that export defaults should behave the same way as regular var defaults and add function argument defaults to the list.

I bring this up because of the comparison to Python back when this issue was first discussed. In Python (at least the version I remember), using a mutable value like foo=[] for a function's default argument would cause changes within the function to carry over to future calls. (I personally found this very astonishing -- requiring some explanation -- and usually inconvenient.) Yet in GDScript...

func test_array_default(foo := []):
	foo.append(randi())
	print(foo)

func _ready():
	for i in 10:
		test_array_default()
	for i in 10:
		test_array_default([i])

...it works intuitively. Example output showing that the default arg array is new every time, just like explicitly passing a new array:

[3161026589]
[2668139190]
[4134715227]
[4204663500]
[1099556929]
[3154986942]
[1519752278]
[1353875733]
[3960688578]
[1798784612]
[0, 546127059]
[1, 2642119551]
[2, 517644291]
[3, 2465219394]
[4, 1109682597]
[5, 1097442878]
[6, 2390510176]
[7, 3860691271]
[8, 4253311206]
[9, 1086767338]

It only accumulates values if you pass the same array every time (not shown here) instead of a new one -- and the default is new each time.

@djrain
Copy link

djrain commented Aug 14, 2024

Still valid in 4.3 RC3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests