godotengine / godot

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

Packed array properties of noncustom resources can't be resized #80950

Closed OffsetMOSFET closed 2 months ago

OffsetMOSFET commented 1 year ago

Godot version

4.1.stable

System information

Godot v4.1.stable - Windows 10.0.19044 - Vulkan (Forward+) - dedicated Quadro P2200 () - Intel(R) Xeon(R) Gold 5222 CPU @ 3.80GHz (8 Threads)

Issue description

For Godot (not custom) Resources, a property that is a PackedArray cannot be resized directly. Copying, resizing, then setting the copy currently works as a work-around.

This behavior is different from custom Resource classes that are built in GDScript instead of C++. These allow resizing of PackedArray properties directly.

Steps to reproduce

Put the attached bug_script.gd code a Control node of a new scene, copy the custom class script gd_custom_class.gd as its own file, then run the scene. The output will mark the point of discrepency.

Output:

Property init size: 0
Propety resized size: 0 (Expected 10)
Array init size: 0
Array resized size: 10
Property reassigned size 10
-
Property init size: 0
Propety resized size: 0 (Expected 11)
Array init size: 0
Array resized size: 11
Property reassigned size 11
-
Property init size: 0
Propety resized size: 0 (Expected 12)
Array init size: 0
Array resized size: 12
Property reassigned size 12
-
Property init size: 0
Propety resized size: 13 (Expected 13)
Array init size: 0
Array resized size: 13
Property reassigned size 13

Minimal reproduction project

bug_script.gd

extends Control

var res0 := StreamPeerBuffer.new()
var res1 := OggPacketSequence.new()
var res2 := ArrayOccluder3D.new()
var res3 := GDCustomClass.new()

func _ready():
    print("Property init size: " + str(res0.data_array.size()))
    res0.data_array.resize(10)
    print("Propety resized size: " + str(res0.data_array.size()) + " (Expected 10)")
    var new_array0 := PackedByteArray()
    print("Array init size: " + str(new_array0.size()))
    new_array0.resize(10)
    print("Array resized size: " + str(new_array0.size()))
    res0.data_array = new_array0
    print("Property reassigned size " + str(res0.data_array.size()))

    print("-")

    print("Property init size: " + str(res1.granule_positions.size()))
    res1.granule_positions.resize(11)
    print("Propety resized size: " + str(res1.granule_positions.size()) + " (Expected 11)")
    var new_array1 := PackedInt64Array()
    print("Array init size: " + str(new_array1.size()))
    new_array1.resize(11)
    print("Array resized size: " + str(new_array1.size()))
    res1.granule_positions = new_array1
    print("Property reassigned size " + str(res1.granule_positions.size()))

    print("-")

    print("Property init size: " + str(res2.indices.size()))
    res2.indices.resize(12)
    print("Propety resized size: " + str(res2.indices.size()) + " (Expected 12)")
    var new_array2 := PackedInt32Array()
    print("Array init size: " + str(new_array2.size()))
    new_array2.resize(12)
    print("Array resized size: " + str(new_array2.size()))
    res2.indices = new_array2
    print("Property reassigned size " + str(res2.indices.size()))

    print("-")

    print("Property init size: " + str(res3.packed_byte_array.size()))
    res3.packed_byte_array.resize(13)
    print("Propety resized size: " + str(res3.packed_byte_array.size()) + " (Expected 13)")
    var new_array3 := PackedByteArray()
    print("Array init size: " + str(new_array3.size()))
    new_array3.resize(13)
    print("Array resized size: " + str(new_array3.size()))
    res3.packed_byte_array = new_array3
    print("Property reassigned size " + str(res3.packed_byte_array.size()))

gd_custom_class.gd

class_name GDCustomClass
extends Resource

@export var packed_byte_array : PackedByteArray
dalexeev commented 1 year ago

I don't think there is a bug in GDScript. Godot properties in C++ are just setter and getter. Arrays (including Packed*Arrays) are passed by reference. Unlike objects, they do not have signals to track external changes.

The built-in classes you listed do not return a direct reference to the data array (they use other structures under the hood). When the getter is called, a new PackedByteArray is created and data from this structure is written to it, and when the setter is called, the array is validated and written to the structure.

Here is an example in GDScript:

var data: PackedByteArray:
    set(value):
        _data = value.duplicate()
    get:
        return _data.duplicate()

var _data: PackedByteArray

Accordingly, you call resize() not on the internal data structure, but only on the temporary array returned by the getter.

I agree that the current API misleading and probably this should not be exposed as data property, but should be two separate methods set_data() and get_data(). However, we can't remove the property due to compatibility. We should add a note to the documentation for such properties.

AThousandShips commented 1 year ago

This is documented for Gradient similar can be applied elsewhere

dalexeev commented 1 year ago

<member .* type="(Packed.*Array|Array|Dictionary|.*\[\])"

235 matches, some false positives ``` doc/classes/AnimationLibrary.xml: 1 58: 3: doc/classes/ArrayOccluder3D.xml: 2 24: 3: 28: 3: doc/classes/AudioStreamWAV.xml: 1 23: 3: doc/classes/CPUParticles2D.xml: 3 153: 3: 156: 3: 159: 3: doc/classes/CPUParticles3D.xml: 3 158: 3: 162: 3: 165: 3: doc/classes/CharFXTransform.xml: 1 21: 3: doc/classes/CodeEdit.xml: 6 458: 3: 467: 3: 470: 3: 494: 3: 507: 3: doc/classes/CodeHighlighter.xml: 3 119: 3: 125: 3: 128: 3: doc/classes/CollisionPolygon2D.xml: 1 26: 3: doc/classes/CollisionPolygon3D.xml: 1 22: 3: doc/classes/ConcavePolygonShape2D.xml: 1 16: 3: doc/classes/ConvexPolygonShape2D.xml: 1 24: 3: doc/classes/ConvexPolygonShape3D.xml: 1 16: 3: doc/classes/EditorFileDialog.xml: 1 72: 3: doc/classes/FileDialog.xml: 1 73: 3: doc/classes/Font.xml: 1 338: 3: doc/classes/FontFile.xml: 2 585: 3: 623: 3: doc/classes/FontVariation.xml: 2 49: 3: 71: 3: doc/classes/Gradient.xml: 2 80: 3: 91: 3: doc/classes/HeightMapShape3D.xml: 1 13: 3: doc/classes/Image.xml: 1 566: 3: doc/classes/Label.xml: 2 71: 3: 74: 3: doc/classes/Label3D.xml: 1 118: 3: doc/classes/Line2D.xml: 1 80: 3: doc/classes/LineEdit.xml: 1 286: 3: doc/classes/LinkButton.xml: 1 21: 3: doc/classes/MultiMesh.xml: 5 91: 3: 93: 3: 96: 3: 107: 3: 110: 3: doc/classes/NavigationObstacle2D.xml: 1 64: 3: doc/classes/NavigationObstacle3D.xml: 1 71: 3: doc/classes/NavigationPathQueryResult2D.xml: 4 21: 3: 24: 3: 27: 3: 30: 3: doc/classes/NavigationPathQueryResult3D.xml: 4 21: 3: 24: 3: 27: 3: 30: 3: doc/classes/OccluderPolygon2D.xml: 1 18: 3: doc/classes/PackedScene.xml: 1 107: 3: doc/classes/PhysicsPointQueryParameters3D.xml: 1 21: 3: doc/classes/PhysicsRayQueryParameters2D.xml: 1 37: 3: doc/classes/PhysicsRayQueryParameters3D.xml: 1 37: 3: doc/classes/PhysicsShapeQueryParameters2D.xml: 1 21: 3: doc/classes/PhysicsShapeQueryParameters3D.xml: 1 21: 3: doc/classes/PhysicsTestMotionParameters2D.xml: 2 16: 3: 19: 3: doc/classes/PhysicsTestMotionParameters3D.xml: 2 16: 3: 19: 3: doc/classes/Polygon2D.xml: 5 74: 3: 92: 3: 96: 3: 114: 3: 117: 3: doc/classes/PolygonOccluder3D.xml: 1 14: 3: doc/classes/PortableCompressedTexture2D.xml: 1 55: 3: doc/classes/ProjectSettings.xml: 78 280: 3: 904: 3: 996: 3: 1000: 3: 1004: 3: 1008: 3: 1012: 3: 1016: 3: 1020: 3: 1024: 3: 1028: 3: 1032: 3: 1036: 3: 1040: 3: 1044: 3: 1048: 3: 1052: 3: 1056: 3: 1060: 3: 1064: 3: 1068: 3: 1072: 3: 1076: 3: 1080: 3: 1084: 3: 1087: 3: 1094: 3: 1098: 3: 1102: 3: 1105: 3: 1109: 3: 1112: 3: 1115: 3: 1118: 3: 1121: 3: 1124: 3: 1128: 3: 1131: 3: 1135: 3: 1138: 3: 1142: 3: 1146: 3: 1150: 3: 1153: 3: 1157: 3: 1160: 3: 1164: 3: 1168: 3: 1172: 3: 1176: 3: 1180: 3: 1183: 3: 1187: 3: 1190: 3: 1195: 3: 1199: 3: 1203: 3: 1207: 3: 1211: 3: 1215: 3: 1219: 3: 1222: 3: 1226: 3: 1229: 3: 1233: 3: 1237: 3: 1241: 3: 1245: 3: 1249: 3: 1252: 3: 1256: 3: 1259: 3: 1263: 3: 1267: 3: 1270: 3: 1274: 3: 1278: 3: 1282: 3: doc/classes/RDFramebufferPass.xml: 4 14: 3: 20: 3: 23: 3: 26: 3: doc/classes/RDPipelineColorBlendState.xml: 1 12: 3: doc/classes/RDPipelineMultisampleState.xml: 1 27: 3: doc/classes/RDShaderSPIRV.xml: 5 45: 3: 48: 3: 51: 3: 54: 3: 57: 3: doc/classes/ResourceImporterBMFont.xml: 1 18: 3: doc/classes/ResourceImporterDynamicFont.xml: 5 28: 3: 44: 3: 57: 3: 63: 3: 66: 3: doc/classes/ResourceImporterImageFont.xml: 2 17: 3: 28: 3: doc/classes/ResourceImporterScene.xml: 1 15: 3: doc/classes/RichTextLabel.xml: 2 554: 3: 595: 3: doc/classes/ShapeCast2D.xml: 1 141: 3: doc/classes/ShapeCast3D.xml: 1 148: 3: doc/classes/Shortcut.xml: 1 34: 3: doc/classes/StreamPeerBuffer.xml: 1 53: 3: doc/classes/SystemFont.xml: 1 25: 3: doc/classes/TextEdit.xml: 1 1190: 3: doc/classes/TextMesh.xml: 1 51: 3: doc/classes/Window.xml: 1 614: 3: modules/csg/doc_classes/CSGPolygon3D.xml: 1 50: 3: modules/gltf/doc_classes/GLTFAccessor.xml: 2 18: 3: 20: 3: modules/gltf/doc_classes/GLTFMesh.xml: 2 10: 3: 12: 3: modules/gltf/doc_classes/GLTFNode.xml: 1 36: 3: modules/gltf/doc_classes/GLTFSkeleton.xml: 2 52: 3: 54: 3: modules/gltf/doc_classes/GLTFSkin.xml: 4 47: 3: 49: 3: 51: 3: 53: 3: modules/gltf/doc_classes/GLTFState.xml: 4 272: 3: 282: 3: 284: 3: 290: 3: modules/minimp3/doc_classes/AudioStreamMP3.xml: 1 18: 3: modules/ogg/doc_classes/OggPacketSequence.xml: 2 20: 3: 23: 3: modules/openxr/doc_classes/OpenXRAction.xml: 1 21: 3: modules/openxr/doc_classes/OpenXRActionMap.xml: 2 90: 3: 93: 3: modules/openxr/doc_classes/OpenXRActionSet.xml: 1 35: 3: modules/openxr/doc_classes/OpenXRIPBinding.xml: 1 44: 3: modules/openxr/doc_classes/OpenXRInteractionProfile.xml: 1 28: 3: modules/regex/doc_classes/RegExMatch.xml: 2 44: 3: 47: 3: modules/websocket/doc_classes/WebSocketMultiplayerPeer.xml: 2 54: 3: 69: 3: modules/websocket/doc_classes/WebSocketPeer.xml: 2 154: 3: 167: 3: platform/android/doc_classes/EditorExportPlatformAndroid.xml: 1 259: 3: platform/ios/doc_classes/EditorExportPlatformIOS.xml: 3 147: 3: 153: 3: 159: 3: platform/macos/doc_classes/EditorExportPlatformMacOS.xml: 14 22: 3: 62: 3: 102: 3: 189: 3: 195: 3: 201: 3: 207: 3: 213: 3: 219: 3: 225: 3: 231: 3: 237: 3: 243: 3: 249: 3: platform/windows/doc_classes/EditorExportPlatformWindows.xml: 1 53: 3: ```
AThousandShips commented 1 year ago

Should probably be documented on each of the packed types as well

OffsetMOSFET commented 1 year ago

Thank you for the clear explanation.

OffsetMOSFET commented 2 months ago

Seeing that the API for stable has been updated to note that the arrays are copies, I will close this.

dalexeev commented 2 months ago

However, this note is missing for arrays and dictionaries. Also, there are some exceptions where built-in pass-by-reference properties takes modification into account, but this is difficult to detect automatically. CC @AThousandShips

AThousandShips commented 2 months ago

Arrays and dictionaries are a case by case thing as some are passed directly and some are copied, so some manual thing would need to be added