Confusing matrix row/col convention in shader language documentation

godotengine / godot-docs

Godot Engine official documentation

Other

3.78k stars 3.07k forks source link

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 :

the meaning of the indexing convention the matrix’ components (scalar or sub-vector) are accessed with
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

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.

godotengine / godot-docs