godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.7k stars 528 forks source link

Basis is wrong, Different results in GDExtension vs GDScript #1416

Closed TokisanGames closed 6 months ago

TokisanGames commented 6 months ago

Godot version

4.2.1-stable

godot-cpp version

4.1.3 631cd5f and godot-4.2.1-stable

System information

Windows 11/64, RTX 3070, Vulkan

Issue description

GDExtension swaps some variables in Basis and gives different (wrong) results compared to GDScript in Godot-cpp 4.1.3 and 4.2.1. Inserting and printing the individual x/y/z vectors is fine. However, when printing the Basis, using it, orthonormalizing it, using it in a Transform3D, the end result is wrong.

// GLSL
var b: Basis 
b[0] = Vector3(0.572943, 0.640776, 0)
b[1] = Vector3(-0.640776, 0.572943, -0.511021)
b[2] = Vector3(0, 0, 1)
print(b[0])
print(b[1])
print(b[2])
print(b)

// Output:
(0.572943, 0.640776, 0)
(-0.640776, 0.572943, -0.511021)
(0, 0, 1)
[X: (0.572943, 0.640776, 0), Y: (-0.640776, 0.572943, -0.511021), Z: (0, 0, 1)]
// C++
Basis b; 
b[0] = Vector3(0.572943, 0.640776, 0);
b[1] = Vector3(-0.640776, 0.572943, -0.511021);
b[2] = Vector3(0, 0, 1);
UtilityFunctions::print(b[0]);
UtilityFunctions::print(b[1]);
UtilityFunctions::print(b[2]);
UtilityFunctions::print(b);

//Output:
(0.572943, 0.640776, 0)
(-0.640776, 0.572943, -0.511021)
(0, 0, 1)
[X: (0.572943, -0.640776, 0), Y: (0.640776, 0.572943, 0), Z: (0, -0.511021, 1)]
//                                                    ^ `y.z` and `z.y` are swapped.

I can manually swap y.z and z.y, however this negates x.y and whether I insert x.y as positive or negative, I cannot get it to turn positive again.

Transform3D xform;
xform.origin = Vector3(x * .5f, 0.f, z * .5f);
Vector3 normal = _storage->get_normal(xform.origin);
normal = normal.normalized();
xform.basis[1] = normal;
xform.basis[0] = -xform.basis[2].cross(normal);

// manually swap z.y and z.y
real_t tmp = xform.basis[1].z;
xform.basis[1].z = xform.basis[2].y;
xform.basis[2].y = tmp;

// After the above swap, x.y is negative, but this does nothing
xform.basis[0].y *= -1;

May be related to issue #426 and PR #859


I was able to get it to work by using the axes constructor. Even though the stored axes are wrong, I'm able to get the same orthonormalized result in the basis as the GDScript version.

Vector3 z_axis = Vector3(0.f, 0.f, 1.f);
Vector3 crossp = -z_axis.cross(normal);
xform.basis = Basis(crossp, normal, z_axis);

So, at the very least, the documentation is missing or inaccurate as it clearly states using [] is the same as accessing the axes directly and gives no indication that any thing else is needed. There's even a note that C# is different from GDScript, but that link says nothing about this.

What works in GDScript should also work in GDExtension. A passible, but tacky alternative is documentation on how it should be done differently in GDExtension.

AThousandShips commented 6 months ago

See the latest documentation, it has been updated to clarify this

dsnopek commented 6 months ago

What works in GDScript should also work in GDExtension. A passible, but tacky alternative is documentation on how it should be done differently in GDExtension.

godot-cpp aims to be compatible with the internal engine API used by Godot modules, and this is how Basis works in the engine (which is different than how than it works in GDScript).

I understand that this is confusing - it confused me when I started contributing to the engine too! I'm glad this is represented in the docs now, as @AThousandShips points out. But it's not something we can change in Godot 4.x for compatibility reasons.

TokisanGames commented 6 months ago

Closing since its by design and documented here and in docs.

The documentation should also suggest using the axes constructor: Basis(x_axis, y_axis, z_axis);