godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.69k stars 527 forks source link

Dictionaries return copies of stored TypedArrays #1508

Closed TokisanGames closed 3 months ago

TokisanGames commented 3 months ago

Godot version

4.2.2-stable

godot-cpp version

godot-4.2.2-stable

System information

Windows 11/64, RTX 3070, Vulkan

Issue description

I am receiving copies of TypedArrays in gdextension when storing arrays indexed via Vector2i.

TypedArray: copy

Dictionary test_dict;
TypedArray<Transform3D> ta;
test_dict[Vector2i(0, 0)] = ta;
ta.push_back(Transform3D(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1));
UtilityFunctions::print("Array A ptr: ", uint64_t(&ta), " size: ", ta.size());

TypedArray<Transform3D> tb = test_dict[Vector2i(0, 0)];
tb.push_back(Transform3D(0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1));

UtilityFunctions::print("Array B ptr: ", uint64_t(&tb), " size: ", tb.size());
UtilityFunctions::print("Array A ptr: ", uint64_t(&ta), " size: ", ta.size());
Array A ptr: 81925230416 size: 1
Array B ptr: 81925230424 size: 2    // note different memory address
Array A ptr: 81925230416 size: 1   // should be size 2

Array: reference

Dictionary test_dict2;
Array aa;
test_dict2[Vector2i(0, 0)] = aa;
aa.push_back(Transform3D(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1));
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());

Array ab = test_dict2[Vector2i(0, 0)];
ab.push_back(Transform3D(0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1));

UtilityFunctions::print("Array B ptr: ", uint64_t(&ab), " size: ", ab.size());
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());
Array A ptr: 87499460384 size: 1
Array B ptr: 87499460400 size: 2
Array A ptr: 87499460384 size: 2

Image: reference

Dictionary test_image;
Ref<Image> img = Image::create(1, 1, true, Image::FORMAT_RGB8);
img->set_pixel(0, 0, Color(1, 1, 1));
UtilityFunctions::print("Image A ptr: ", uint64_t(img.ptr()), " (0,0): ", img->get_pixel(0, 0));
test_image[Vector2i(0, 0)] = img;

Ref<Image> b = test_image[Vector2i(0, 0)];
b->set_pixel(0, 0, Color(.5, .5, .5));
UtilityFunctions::print("Image B ptr: ", uint64_t(b.ptr()), " (0,0): ", b->get_pixel(0, 0));
UtilityFunctions::print("Image A ptr: ", uint64_t(img.ptr()), " (0,0): ", img->get_pixel(0, 0));
Image A ptr: 3032189040112 (0,0): (1, 1, 1, 1)
Image B ptr: 3032189040112 (0,0): (0.498, 0.498, 0.498, 1)  // same memory pointer
Image A ptr: 3032189040112 (0,0): (0.498, 0.498, 0.498, 1)  // correct value

I replaced the Godot dictionary with std::map and it works properly with Godot TypedArrays, so I am using that for now. Using a local map while building, then converting to a Godot dictionary for storage within a Godot resource.

AThousandShips commented 3 months ago

Does this happen when not using typed arrays? Given your example uses them only

TokisanGames commented 3 months ago

Arrays also copied, Images by reference. I have simplified the code above and made it testable.

AThousandShips commented 3 months ago

Typed arrays are already tracked here:

So let's focus on dictionary, does it happen without using TypedArray at all?

TokisanGames commented 3 months ago

Please review my changes above. It affects Arrays and TypedArrays. Not Image.

This is likely related to that other issue. It mentions an Array constructor that makes a copy. Perhaps Dictionary is triggering that constructor. The proposed PR only touches TypedArray so I'm guessing it won't fix this issue for Arrays in Dictionaries.

ajreckof commented 3 months ago
Dictionary test_dict2;
Array aa;
test_dict2[Vector2i(0, 0)] = aa;
aa.push_back(Transform3D(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1));
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());

TypedArray<Transform3D> ab = test_dict2[Vector2i(0, 0)];
ab.push_back(Transform3D(0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1));

UtilityFunctions::print("Array B ptr: ", uint64_t(&ab), " size: ", ab.size());
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());

would always trigger the constructor (and therefore a copy) since you are implicitly creating a typed array from an array can you try with arrays only something like :

Dictionary test_dict2;
Array aa;
test_dict2[Vector2i(0, 0)] = aa;
aa.push_back(Transform3D(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1));
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());

Array ab = test_dict2[Vector2i(0, 0)];
ab.push_back(Transform3D(0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1));

UtilityFunctions::print("Array B ptr: ", uint64_t(&ab), " size: ", ab.size());
UtilityFunctions::print("Array A ptr: ", uint64_t(&aa), " size: ", aa.size());
TokisanGames commented 3 months ago

@ajreckof Perhaps you saw an outdated version of this ticket. Please refresh and see that I tested TypedArray (copy), Array (copy) and Image (reference).

edit: oh wait, my code for Array is wrong though. Let me redo that section. edit 2: I had my second ab as a typedarray. Now both are Arrays and it passes by reference.

Ok, given that it's only related to TypedArray, I think this probably is just a duplicate. @AThousandShips do you think your PR will fix this ticket?

ajreckof commented 3 months ago

I tested with gdscript and can't reproduce so this is gdextension only

edit : code I tested with :

    var dict = {}
    var arr1 : Array[Transform3D]
    dict[0] = arr1
    var arr2 : Array[Transform3D] = dict.get(0)
    var arr3 : Array[Transform3D] = dict[0]
    arr1.push_back(Transform3D.IDENTITY)
    arr2.push_back(Transform3D.FLIP_X)
    print(dict)
    print(arr1)
    print(arr2)
    print(arr3)

result

{ 0: [[X: (1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)], [X: (-1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)]] }
[[X: (1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)], [X: (-1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)]]
[[X: (1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)], [X: (-1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)]]
[[X: (1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)], [X: (-1, 0, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)]]
AThousandShips commented 3 months ago

Ok, given that it's only related to TypedArray, I think this probably is just a duplicate. AThousandShips do you think your PR will fix this ticket?

Feel free to try it out, I'm not sure, but it does sound like a duplicate as it doesn't affect Array

TokisanGames commented 3 months ago

The PR does fix the issue with TypedArray. I can use std::map for now until 4.3 is stable. Thank you both for the help. This was a frustrating journey. The function I was writing now processes 230k instances in .34 seconds and runs ~1700x faster with the PR and not having to writing back to the dictionary.

Duplicate #1149 Fixed by #1483