GDNative linear indexing of Basis returns rows, expected columns (axis)

[kakoeimon]

Operating system or device, Godot version, GPU Model and driver (if graphics related):
Manjaro 64bit, Godot Beta1

Issue description:

Seems that the values from GetGlobalTransform to be wrong. (Not sure thought)

Steps to reproduce:
Run the example project. It uses the Mono script for the player.
Clasic fps controls WASD and mouse to move.
Switch the script of the Player to Player.gd to see how it was used to work in Godot.

Link to minimal example project:
GlobalTransform.zip

P.S. I am not sure that the problem is in GetGlobalTransform but seem to me that only this method can ruin the movement.

[akien-mga]

CC @neikeq @Hinsbart

[kakoeimon]

After some more testings I realized that GetGlobalTransform acts ok, but the problem is in Vector3.cs
I think I found the problem but right now I cannot compile master to test it.

If I am right the problem is in godot/modules/mono/glue/cs_files/Vector3.cs
link:
https://github.com/godotengine/godot/blob/7f022a33a31ad276047744ef42a9fbd85f39a6b4/modules/mono/glue/cs_files/Vector3.cs
at the lines 270-276
public static Vector3 operator -(Vector3 vec) { vec.x = -vec.x; vec.y = -vec.y; vec.z = -vec.z; return vec; }

I think this must be removed.

[akien-mga]

If I am right the problem is in godot/modules/mono/glue/cs_files/Vector3.cs
link:
https://github.com/godotengine/godot/blob/7f022a33a31ad276047744ef42a9fbd85f39a6b4/modules/mono/glue/cs_files/Vector3.cs
at the lines 270-276
public static Vector3 operator -(Vector3 vec) { vec.x = -vec.x; vec.y = -vec.y; vec.z = -vec.z; return vec; }

I don't know, in my tests it works as it should:

Vector3 vec0 = new Vector3(5.3f, -2.0f, -0.087f);
GD.Print(vec0);
Vector3 vec1 = -vec0;
GD.Print(vec1);
Vector3 vec2 = new Vector3();
vec2 -= vec0;
GD.Print(vec2);

outputs:

(5.3, -2, -0.087)
(-5.3, 2, 0.087)
(-5.3, 2, 0.087)

[kakoeimon]

Yes looks like I am wrong again.
I tested on my example project by replacing all the Vector3 operators and the problem remains, so the problem must be somewhere else.
Here is a better example project.
GlobalTransform.zip
It does not move but only rotates the object by using the mouse. (I removed the movement to be sure I am not messing up anything). I also created a gdscript and attached it to a child of the object which is doing the same thing abd outputs the results.
So now I get in the output : CS + direction
GD + direction.

I get very strange results.
For example if press only forward or backward the x value of CS is just negative of the GD.
If I press only left and right CS get switched values of x and z of the GD.

[kakoeimon]

@akien-mga I just made the same test by using Godot-Nim and I got the same strange output.
I believe this proves that the problem is inside GDNative.

[akien-mga]

I believe this proves that the problem is inside GDNative.

I don't think so as Mono doesn't use GDNative.

CC @endragor who may have a clue about what could be wrong in Godot-Nim.

[kakoeimon]

I don't think so as Mono doesn't use GDNative.

Are you sure about that? Seems too strange to have the same problem in Nim and C#, that's why I thought that Mono is using parts of GDNative.

Here is the example project (like the first example project) for Nim.
test_nim.zip

[endragor]

Try using axis instead of [] (aim.axis(2)). [] returns rows of the basis, and axis returns columns, similar to Godot's C++ API. I'm not sure how it works in GDScript.

[akien-mga]

I think what GDScript used is how it's defined in Variant::get: https://github.com/godotengine/godot/blob/master/core/variant_op.cpp#L2422-L2434

[akien-mga]

I'm not sure what are the established conventions for this, but in the context of Basis I think it makes sense for basis[i] to return a Vector3 with the column of the i-th coordinate (x, y or z), a.k.a. the "axis".

It seems to be consistent with linear indexing of matrices in Matlab at least: https://de.mathworks.com/help/matlab/math/matrix-indexing.html?requestedDomain=www.mathworks.com#f1-85511

[kakoeimon]

Perfect.

Just for future reference.
In Nim we have to replace basis[0] to basis.axis(0)
and in Mono to basis.GetAxis(0)

[akien-mga]

Quoting @reduz from IRC:

<reduz> yeah, Basis should return transposed axes and also accept them as constructor. I wasted a few hours yesterday trying to fix a bug because of that
<reduz> they are flipped in C++ for performance

[akien-mga]

That needs to be fixed before RC1 ideally.

[kakoeimon]

This looks like it is more problematic from what it seems.
I just found out that even the x y z of the basis suffer from the same problem.

Of course you can alternatively use GetAxis (e.g. for basis.z basis.GetAxis(2) ) but this must be corrected.

I tested this only in Mono.
This is and example project with the problem.

basis_problems.zip
Edit : It contains two objects, One uses GDScript and the othe Mono. Both are trying to move allong the local z axis.

Be careful with this cause if you do not rotate the objects to another angle from the initial one you will not see the problem. The objects in the example are rotated.

[reduz]

Bound scripting langs should not imitate the C++ API, which is made the current way for faster vector-matrix multiplication.

GDScript already exposes the vectors as transposed.

[akien-mga]

Mono part is fixed by #16326, only GDNative left.

[tagcup]

As I explained in #16937

linear indexing of Basis returns rows, expected columns (axis)

you should be expecting rows if M[i][j] should refer to M_{ij} on paper. The first index is row, the second is column. Basis implementation got it right. It is Transform2D which is transposed and awkward, which has M[j][i] corresponding to M_{ij} on paper.

get_axis returns a column, not a row (because, to get the local x-axis of the transformation, you do M.(1,0,0) for example which gives you the first column, not row) so you shouldn't expect to be able to access it like M[i], just use the accessor function.

[reduz]

They are regarding to how GLSL and OpenGL uses them, which is also how most people expect them to be.

…

On Thu, Mar 22, 2018 at 8:35 AM, tagcup ***@***.***> wrote: Columns/rows *are* swapped in Transform2D though — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z29tj8xYsw4BC5n-d3wVc9OhsFNQSks5tg8S3gaJpZM4Q9VmP> .

[tagcup]

No they aren't. The old version was like that, if you remember, and led to many bugs, and I fixed them

[tagcup]

Oh wait, you mean the OpenGL itself. Sorry. Yeah, could be.

[tagcup]

@reduz you're totally right about it, OpenGL is for whatever reason column major: https://www.opengl.org/archives/resources/faq/technical/transformations.htm

I see where you're coming from now. Actually you did mention this back then I totally forgot. But they also admit this choice in OpenGL caused endless confusion among programmers, so I think it is best to avoid and stick with the current sensible row major implementation

[reduz]

The confusion comes from DirectX being row major. I prefer we stick to column major simply for the fact we already are adopting all other OpenGL conventions, such as coordinate system, clip space range, shading language, etc.

…

On Thu, Mar 22, 2018 at 9:00 AM, tagcup ***@***.***> wrote: @reduz <https://github.com/reduz> you're totally right about it, OpenGL is for whatever reason column major: https://www.opengl.org/ archives/resources/faq/technical/transformations.htm I see where you're coming from now. Actually you did mention this back then I totally forgot. But they also admit this choice in OpenGL caused endless confusion among programmers, so I think it is best to avoid and stick with the current sensible row major implementation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z2xcjH9r1naxPVuO5xEabzY0wepg3ks5tg8q3gaJpZM4Q9VmP> .

[tagcup]

I'm guessing their reasoning for this awkward choice is because it made get_axis simpler, which they probably use a lot internally, and they're trying to rationalize that by saying if you transposed all the maths in the world it'll work OK so it's the same, just a matter of convention, you just need to flip the entire math literature upside down! ("Column-major versus row-major is purely a notational convention. Note that post-multiplying with column-major matrices produces the same result as pre-multiplying with row-major matrices.")

[reduz]

When they made this choice, vector instruction sets were not common, so I can understand it. DirectX swapped it because it could take better advantage of SSE. On the hardware level (GPUs) it doesn't really matter. In any case, will prefer to stay true to the OpenGL convetion, so bound languages will be column major.

…

On Thu, Mar 22, 2018 at 9:09 AM, tagcup ***@***.***> wrote: I'm guessing their reasoning for this awkward choice is because it made get_axis simpler, which they probably use a lot internally, and they're trying to rationalize that by saying if you transposed all the maths in the world it'll work OK so it's the same, just a matter of convention, you just need to flip the entire math literature upside down! ("Column-major versus row-major is purely a notational convention. Note that post-multiplying with column-major matrices produces the same result as pre-multiplying with row-major matrices.") — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z28Re0YLZ03Msv-cc32D-Ap5iuNpCks5tg8yigaJpZM4Q9VmP> .

[tagcup]

Actually I found that OpenGL do both column major and row major. There's apparently a transpose flag just for that https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glUniform.xhtml

[tagcup]

So I don't see any single argument for column major.

Not to mention bound languages don't interact with OpenGL

[reduz]

Yes, this is OpenGL4 though, which we don't use and pretty much no one uses. Again, this will not change.

…

On Thu, Mar 22, 2018 at 9:14 AM, tagcup ***@***.***> wrote: So I don't see any single argument for column major. Not to mention bound languages don't interact with OpenGL — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z2-7HivBw3KO6Alh3oxT0g2fYE67vks5tg83ugaJpZM4Q9VmP> .

[tagcup]

No it's not OpenGL4. and in fact, you're using that function already in Godot and explicitly settings the transpose flag to false here.

Also the fact that you don't use something doesn't mean the entire world isn't using it

[tagcup]

Whatever suit yourself then

[akien-mga]

This should be fully fixed (= behaves like GDScript) in Mono after #23833, but I don't know the status for GDNative.

[neikeq]

This was incorrectly marked as fixed for C#. #26203 fixes it.

[akien-mga]

Thanks @neikeq.

@karroffel What was the conclusion in the end? This needs to be fixed in C++ bindings and not GDNative?

[karroffel]

@akien-mga exactly, the GDNative C API doesn't actually expose the indexing, so it's only a binding issue

[akien-mga]

Alright, moved to godotengine/godot-cpp#241.