godot-docs icon indicating copy to clipboard operation
godot-docs copied to clipboard

Confusing matrix row/col convention in shader language documentation

Open Flarkk opened this issue 3 years ago • 1 comments

Your Godot version: 3.5 and 4

Issue description: Documentation is confusing about the row / column convention for matrices creation and access in shaders.

In the data type section

mat{x} types are described as :

{x}x{x} matrix, in column major order.

The term “column major” normally refers to the internal storage layout of the matrix in memory (in this case, one column after another, left to right). No matter if it’s correct or not, this is an implementation detail the user should not have to care about.

👉 I suggest to remove this information from the chart.

Instead, what matters for the user is :

  1. the meaning of the indexing convention the matrix’ components (scalar or sub-vector) are accessed with
  2. the convention used when creating a matrix from a literal

That’s what im focusing on below

1. Access (“Members” section)

The doc says :

For matrices, use the m[column][row] indexing syntax to access each scalar, or m[idx] to access a vector by row index. For example, for accessing the y position of an object in a mat4 you use m[3][1].

2 suggestions here :

  • if godot follows glsl conventions, then m[column][row] is correct, but m[idx] to access a vector by row index is incorrect. 👉 I don’t know the right answer, but whatever it is it would be nice to explicit mention commonalities / differences with the glsl standard.
  • the example given right after sounds completely opaque to me (so I assume the same for most users !) 👉 I suggest to replace by anything more meaningful, or to simply remove it

2. Constructing section

The documentation doesn’t specify whether vecs count for lines or columns in the following statement :

mat2 m2 = mat2(vec2(1.0, 0.0), vec2(0.0, 1.0));

👉 Here again I don’t know the right answer but the doc should be explicit on that

URL to the documentation page: https://docs.godotengine.org/en/3.5/tutorials/shaders/shader_reference/shading_language.html

Flarkk avatar May 05 '22 13:05 Flarkk

See also https://github.com/godotengine/godot-docs/pull/5794.

cc @clayjohn @michael-nischt

Calinou avatar May 05 '22 17:05 Calinou

if godot follows glsl conventions, then m[column][row] is correct, but m[idx] to access a vector by row index is incorrect.

Yes, either the documentation (or worse, the language??) is contradicting itself. If it's column-major order and m[column][row] is correct, then in all likelihood, m[idx] should be column index, not row. If this is right, please change https://github.com/godotengine/godot-docs/blob/master/tutorials/shaders/shader_reference/shading_language.rst#members to:

For matrices, use the m[column][row] indexing syntax to access each scalar, or m[column] to access a vector by column index. For example, for accessing the y position of an object in a mat4 you use m[3][1] or m[3].y.

This NEEDS to be clarified and corrected in the documentation, or people will write buggy shaders.


I disagree with the statement below, however, because in shaders, performance is extremely important:

No matter if it’s correct or not, this is an implementation detail the user should not have to care about.

It would be really weird if accessing a vector in a matrix doesn't specify a contiguous chunk of data. If this is consistently how it works, regardless of platform/hardware, it's relevant and should NOT be an implementation detail. From wikipedia:

Data layout is critical for correctly passing arrays between programs written in different programming languages. It is also important for performance when traversing an array because modern CPUs process sequential data more efficiently than nonsequential data. This is primarily due to CPU caching. In addition, contiguous access makes it possible to use SIMD instructions that operate on vectors of data.

AlfishSoftware avatar Mar 30 '23 02:03 AlfishSoftware