get_translation() and set_translation() of Matrix44 not working correctly

[Candoran2]

@niftools/pyffi-reviewers

Issue Overview

Because the Matrix44 class is imported/exported transposed to how it is displayed in NifSkope, the get_translation() and set_translation() functions of that class do not work as expected.

Version Information

Pyffi Version Info

2.2.4.dev3, can confirm it is also happening with 2.2.4.dev4

Platform information

Windows 10 64-bit, using pyffi through the Blender Nif plugin.

Context

The transform of a bhkConvexTransformShape is transposed to how it is displayed in NifSkope. This is even right after the pyffi import (i.e. at the end of the load_nif function) or right before pyffi export (i.e. just before the data.write(stream)). This means that the get_translation() and set_translation() functions, which set and get from the bottom row (correct with how it is displayed in NifSkope), do not operate on the translation, but on the useless fourth column once back in NifSkope.

Steps to Reproduce

• Load a model with a bhkConvexTransformShape that has a non-zero translation using Pyffi
• (optional) Display the bhkConvexTransformShape's transform
• Modify the translation using set_translation() and get_translation() (e.g. multiply the translation by 2)
• (optional) Display the bhkConvexTransformShape's transform
• Save it back as a nif

Expected Result

The expected result is that get_translation() and set_translation() do the intended modification.

Actual Result

There is no modification on the exported translation, instead the fourth column is affected.

Possible Fix

Either change get_translation() and set_translation() to reflect the fact that the Matrix44 is transposed upon import/export, or do not transpose the Matrix44.

Screenshot

Logs and Files

File I tested with:
https://cdn.discordapp.com/attachments/702566403605266435/706515307014914138/irondagger.nif

Console Output

Relevant output found in screenshots above.

Similar Known Issues

None that I could find.

Additional Information

[neomonkeus]

Is this a generic problem or specific to only that use case.
I wouldn't like to update it and then break everything else in the process that might rely on common Matric functionality.

[Candoran2]

Upon testing, a Matrix33 (rotation on a NiTriShape) also seems to be transposed compared to how it is displayed in NifSkope. I wouldn't be unreasonable to assume that other matrices are, too (not that there are many more for which this matters).

However, I wouldn't necessarily call it being transposed a problem, it's just a discrepancy between NifSkope and pyffi. They both work out the same, mathematically, in the NifSkope case it's (row vector)*matrix and if it's transposed (pyffi) it (should be) matrix*(column vector). However, this difference isn't accounted for in the get_translation and set_translation functions, and they act as though the translation is in the bottom row (which it isn't). Being consistent with that would, however, also lead to changes with the vector*matrix calculations (which need to be matrix*vector and are currently only vector*matrix).

I should note that the get_mass_center_inertia function of the bhkTransformShape class does seem to be aware that the translation lies in the fourth column, as it takes m_14, m_24 and m_34 rather than using the get_translation() function of the transform.

This is, of course, all presuming I haven't missed some extra step that somehow happens when using the blender nif plugin after/before file data.read(stream) or data.write(stream), respectively, or that I haven't missed something in the way the matrices are printed.

[Candoran2]

I've looked into it further. Testing was done with rotation on a NiTriShape for Matrix33 (rotation) and bhkConvexTransform for Matrix44.

I think I kind of know what the deal is now. As I said before, both Matrix33 and Matrix44 are transposed to how they are in NifSkope. However, Matrix33 and Matrix44 (or at least NiTriShape rotation matrix and bhkConvexTransform transform matrix) are not directly convertible to one another. For the same Euler rotation values, the upper left corner of the Matrix44 is transposed compared to the rotation matrix:
Matrix44:

Rotation Matrix:

For ease of reference, column vector multiplication and row vector multiplication is used here to denote Matrix*vector and vector*Matrix, respectively. It indicates which of the two transpose options the matrix is in, and therefore also where in a 4x4 matrix the translation is: for column vector multiplication, the translation is in the rightmost column, and for row vector multiplication the translation is in the bottom row of the matrix.

To give an overview of what that means:

1. NifSkope Matrix33: correct results with column vector multiplication
2. NifSkope Matrix44: correct results with row vector multiplication
3. pyffi Matrix33: correct results with row vector multiplication
4. pyffi Matrix44: correct results with column vector multiplication.

However, there is also the function get_transform() in pyffi, used by a number of classes. This builds a Matrix44, but based on the rotation matrix (as it's usually separate entries for rotation, scale and translation). Because it's based on the rotation matrix, it has the same transpose option as one. This means there's a 5th case:

5. pyffi Matrix44 created through get_transform(): correct results with row vector multiplication.

As far as I could tell, most if not all of the pyffi code works from the rotation(-based) matrix, i.e. row vector multiplication. This means that the Matrix44 that is created by loading one in from a file is actually the odd one out. As far as I could tell from nif.xml, Matrix44 are only present in a file in bhkTransformShapes, bhkConvexTransformShapes and NiEnvMappedTriShape (where it's unknown what it does). The following might be courses of action (not all necessarily feasible/desirable):

1. Change nothing. Get_translation() and set_translation() are non-functional for Matrix44's created through loading one in from a file, as well as the rotation matrix returned being transposed to the normal format of a rotation matrix. However, all the code will still work for Matrix44s created through functions like get_transform(). Maybe make a note in the documentation.
2. Transpose Matrix44s (again) when they are read/written from a file, so that Matrix44s from a bhkTransformShape work correctly with row vector multiplications, just like every other matrix.
3. Create separate functions that ought to be used when a Matrix44 is created by loading one in from a file. (e.g. something named get_translation_bhk()) which do all the normal things, but transposed.
4. Create a distinction between the two types of Matrix44s, i.e. Matrix44s as they are in the file, and Matrix44s for functions like get_transform() etc.
5. Enforce the way Matrix44s are loaded from a file, and have all conversion functions do a transpose when going from a Matrix33 to Matrix44 or the other way around. Would probably break too many things, as well as being very clunky.