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

C++ A deep copy (COW) of TypedArray is sometimes incorrectly made #89191

Closed
allenwp opened this issue Mar 5, 2024 · 3 comments · Fixed by #89197
Closed

C++ A deep copy (COW) of TypedArray is sometimes incorrectly made #89191

allenwp opened this issue Mar 5, 2024 · 3 comments · Fixed by #89197

Comments

@allenwp
Copy link
Contributor

allenwp commented Mar 5, 2024

Tested versions

Reproducible on 7d2ca2d and also on 4.2-stable 46dc277

System information

Godot v4.3.dev (7d2ca2d) - Windows 10.0.22621 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 980 Ti (NVIDIA; 31.0.15.3598) - 13th Gen Intel(R) Core(TM) i7-13700K (24 Threads)

Issue description

When casting from TypedArray to Array and back again, a deep copy (COW) will be performed. This seems to also affect nested Array occurrences, thus making it impossible(?) to extract a reference to a TypedArray as a TypedArray from an Array or TypedArray.

Steps to reproduce

A simple example is the following. Even though untypedArray in this example knows that it is, internally, a typed array, it will still perform a COW deep copy.

TypedArray<int> typedArray;
typedArray.push_back(1);

Array untypedArray = typedArray;
TypedArray<int> typedArrayCopy = untypedArray; // COW

untypedArray[0] = 2;
typedArrayCopy[0] = 3;

WARN_PRINT(typedArray[0]); // Prints 2
WARN_PRINT(untypedArray[0]); // Prints 2
WARN_PRINT(typedArrayCopy[0]); // Prints 3

This behaviour arguably becomes more problematic when dealing with nested Array scenarios:

TypedArray<int> typedArray;
typedArray.push_back(1);

Array parentArray;
parentArray.push_back(typedArray);

Array untypedArray = parentArray[0];
TypedArray<int> typedArrayCopy = parentArray[0];

untypedArray[0] = 2;
typedArrayCopy[0] = 3;

WARN_PRINT(typedArray[0]); // Prints 2
WARN_PRINT(untypedArray[0]); // Prints 2
WARN_PRINT(typedArrayCopy[0]); // Prints 3

This second example demonstrates that it is difficult (and maybe impossible?) to extract a TypedArray from an Array (or TypedArray) and put it back into a TypedArray container without doing a COW that may not be desirable.

GDScript

This behaviour does not affect GDScript. In GDScript, I can do the following and a COW deep copy will never occur:

var typedArray: Array[int];
typedArray.push_back(1);

var untypedArray: Array = typedArray;
var typedArrayCopy: Array[int] = untypedArray;

untypedArray[0] = 2;
typedArrayCopy[0] = 3;

print(typedArray[0]); # Prints 3
print(untypedArray[0]); # Prints 3
print(typedArrayCopy[0]); # Prints 3
var typedArray: Array[int];
typedArray.push_back(1);

var parentArray: Array;
parentArray.push_back(typedArray);

var untypedArray: Array = parentArray[0];
var typedArrayCopy: Array[int] = parentArray[0];

untypedArray[0] = 2;
typedArrayCopy[0] = 3;

print(typedArray[0]); # Prints 3
print(untypedArray[0]); # Prints 3
print(typedArrayCopy[0]); # Prints 3

Minimal reproduction project (MRP)

(See above code snippets. Put them in something that you know will be called in the engine like register_core_singletons() of register_core_types.cpp)

allenwp added a commit to allenwp/godot-vector-display that referenced this issue Mar 5, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Mar 5, 2024

I'm not sure this is a bug, you're operating on extended types and casting them to their parent types, that's not type safe, you're losing typing information, that's why it works with GDScript because it doesn't operate on them in the same way, I'd say

What happens if you do:

TypedArray<int> typedArrayCopy;
typedArrayCopy = untypedArray;

If that does work then it's a bug and the issue is here:

_FORCE_INLINE_ TypedArray(const Variant &p_variant) :
Array(Array(p_variant), Variant::OBJECT, T::get_class_static(), Variant()) {
}
_FORCE_INLINE_ TypedArray(const Array &p_array) :
Array(p_array, Variant::OBJECT, T::get_class_static(), Variant()) {
}

I'll test and look later and if that is the problem I can fix the relevant code

@allenwp
Copy link
Contributor Author

allenwp commented Mar 5, 2024

What happens if you do:

TypedArray<int> typedArrayCopy;
typedArrayCopy = untypedArray;

Using the code you provided instead does not perform a deep copy!

@AThousandShips
Copy link
Member

Then I'll write some fixes for this, it should work, will see!

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

Successfully merging a pull request may close this issue.

2 participants