godotengine / godot

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

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

Open Web-eWorks opened 6 years ago

Web-eWorks commented 6 years ago

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 commented 6 years ago

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

Web-eWorks commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

Web-eWorks commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Keetz commented 6 years ago

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

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

Web-eWorks commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

akien-mga commented 4 years ago

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

cliedelt commented 2 years ago

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

Calinou commented 2 years ago

Related to https://github.com/godotengine/godot/issues/20436 and https://github.com/godotengine/godot/issues/48038.

FeralBytes commented 2 years ago

This just wasted hours of my time! Just to work around this bad "Feature", I have to check if any of the arrays are empty and then set them too = [].duplicate() the engine should do this on it's own.

akien-mga commented 1 year ago

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

ricmzn commented 12 months ago

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, https://github.com/godotengine/godot-docs/issues/5640 should be extended to 4.X docs as well.

Edit: the issue also happens in 4.2 dev 5

DissonantVoid commented 8 months ago

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 commented 8 months ago

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 commented 1 month ago

Still valid in 4.3 RC3