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

Align GLSL matrix multiply and divide with OSL and C++ #2089

Open
HardCoreCodin opened this issue Oct 29, 2024 · 2 comments
Open

Align GLSL matrix multiply and divide with OSL and C++ #2089

HardCoreCodin opened this issue Oct 29, 2024 · 2 comments

Comments

@HardCoreCodin
Copy link

Shading languages use different matrix conventions: OSL is row major, GLSL is column major
This determines multiplication order: M1 * M2 in OSL is M2 * M1 in GLSL
MaterialX back-end implementations generate code with the same ordering, consistent with the node inputs.
If it is correct for OSL, it would be incorrect for GLSL (and vice versa)
This makes the same node (and hence any graph that uses it) non-portable across back-ends.

MatMul_Blender_Vs_MGE_REVERSED MatMul_Blender_Vs_MGE MatMul_Blender_Vs_MGE_M1 MatMul_Blender_Vs_MGE_M2

MaterialX specification mentions row major, but only with reference to matrix literal construction component ordering.
However, both OSL and GLSL use the same ordering anyway: ( X.x, X.y, X.z , Y.x, Y.y, Y.z , Z.x, Z.y, Z.z )
So this is not really saying anything meaningful.
The specification should state what the implied matrix convention is in MaterialX - explicitly.
All implementations should then be made to account for that convention.

If it is row major, then the ordering for the multiply node types that take a 3x3 or 4x4 matrix, should be reversed in GLSL.

MatrialX_glsl_impl_incorrect_multiply_divide_matrix

There should also be a special handling for divide node types, as just flipping the order will not suffice:
Matrix division is defined as the first matrix provided multiplied by the inverse of the second.
The correct matrix need to be inverted in column-major implementations.

MGE_matrix_A_divided_by_B_flipped_order MGE_matrix_A_multiply_by_inverse_of_B_flipped_order
@dbsmythe
Copy link
Contributor

The MaterialX Specification only states that the constructor values for a matrix are "9 or 16 floats separated by commas, specified in row-major order". It doesn't (and I believe shouldn't) state whether the resulting matrix is row-major or column-major ordering in memory; instead, each shading language should use whatever its native representation for a matrix is. So anything that constructs a matrix (either direct values or the <creatematrix> node) should put the 9 or 16 values in the correct places for an implementation, e.g. the 13th, 14th and 15th values in a matrix contructor tuple should go in the 1st/2nd/3rd spots in the bottom row of the matrix, however that's actually laid out in memory. Similarly for anything that reads or extracts specific values from within a matrix: we should provide nodes to do that in a language-independent way (e.g. there is a proposed <extractrowvector> node, there should also be an <extractcolumnvector> node).

@jstone-lucasfilm jstone-lucasfilm changed the title Matrix Multiply and Divide are inconsistent across back-ends due to vague matrix convention specification Align GLSL matrix multiply and divide with C++ and OSL Oct 31, 2024
@jstone-lucasfilm
Copy link
Member

Good catch, @HardCoreCodin, and we've proposed to address this by updating the GLSL implementations of matrix multiply and divide to align with the OSL behavior.

The OSL behavior of the matrix product additionally aligns with our own reference implementation in MaterialX C++, which is defined here:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXCore/Types.h#L511

@jstone-lucasfilm jstone-lucasfilm changed the title Align GLSL matrix multiply and divide with C++ and OSL Align GLSL matrix multiply and divide with OSL and C++ Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants