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

Dictionaries return copies of stored TypedArrays #1508

Closed
TokisanGames opened this issue Jun 26, 2024 · 9 comments
Closed

Dictionaries return copies of stored TypedArrays #1508

TokisanGames opened this issue Jun 26, 2024 · 9 comments
Labels
archived bug This has been identified as a bug

Comments

@TokisanGames
Copy link

TokisanGames commented Jun 26, 2024

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
Copy link
Member

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

@AThousandShips AThousandShips added bug This has been identified as a bug needs testing labels Jun 26, 2024
@TokisanGames
Copy link
Author

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

@TokisanGames TokisanGames changed the title Dictionaries return copies Dictionaries return copies of stored Arrays Jun 26, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Jun 26, 2024

Typed arrays are already tracked here:

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

@TokisanGames
Copy link
Author

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
Copy link
Member

ajreckof commented Jun 26, 2024

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
Copy link
Author

TokisanGames commented Jun 26, 2024

@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
Copy link
Member

ajreckof commented Jun 26, 2024

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)]]

@TokisanGames TokisanGames changed the title Dictionaries return copies of stored Arrays Dictionaries return copies of stored TypedArrays Jun 26, 2024
@AThousandShips
Copy link
Member

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
Copy link
Author

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

@TokisanGames TokisanGames closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived bug This has been identified as a bug
Projects
None yet
Development

No branches or pull requests

3 participants