bpy.types.Bone.AxisRollFromMatrix() returns bad roll value #82930

New Issue

A. H. · 2020-11-23T10:13:25+01:00

A. H. commented

2020-11-23 10:13:25 +01:00

System Information
Operating system: Win 10
Graphics card: Nvidia

Blender Version
Broken: 2.82a+, ..., 2.90
Worked: 2.81a

Short description of error
The roll value returned by bpy.types.Bone.AxisRollFromMatrix() changes between 2.81 and 2.82, and is still broken in 2.90. This means that bones created with this function have a wrong local matrix and become unusable.
The broken roll values are consistently tiny (about e-07).

good - 2.81a:

<Matrix 4x4 (-0.0891, -0.0448,  0.9950,  0.1884)
            ( 0.4410, -0.8975, -0.0009, -4.7614)
            (-0.8931, -0.4387, -0.0998,  4.2873)
            ( 0.0000,  0.0000,  0.0000,  1.0000)>

result_axis: <Vector (-0.0448, -0.8975, -0.4387)>
result_roll: 1.6745758056640625

broken - 2.82+:

<Matrix 4x4 (-0.0891, -0.0448,  0.9950,  0.1884)
            ( 0.4410, -0.8975, -0.0009, -4.7614)
            (-0.8931, -0.4387, -0.0998,  4.2873)
            ( 0.0000,  0.0000,  0.0000,  1.0000)>

result_axis: <Vector (-0.0448, -0.8975, -0.4387)>
result_roll: 8.284717296191957e-06

Exact steps for others to reproduce the error
Run the py script in the attached blend file with the blender versions mentioned above.AxisRollFromMatrix.blend

**System Information** Operating system: Win 10 Graphics card: Nvidia **Blender Version** Broken: 2.82a+, ..., 2.90 Worked: 2.81a **Short description of error** The roll value returned by bpy.types.Bone.AxisRollFromMatrix() changes between 2.81 and 2.82, and is still broken in 2.90. This means that bones created with this function have a wrong local matrix and become unusable. The broken roll values are consistently tiny (about e-07). good - 2.81a: ``` <Matrix 4x4 (-0.0891, -0.0448, 0.9950, 0.1884) ( 0.4410, -0.8975, -0.0009, -4.7614) (-0.8931, -0.4387, -0.0998, 4.2873) ( 0.0000, 0.0000, 0.0000, 1.0000)> ``` result_axis: `<Vector (-0.0448, -0.8975, -0.4387)>` result_roll: **1.6745758056640625** broken - 2.82+: ``` <Matrix 4x4 (-0.0891, -0.0448, 0.9950, 0.1884) ( 0.4410, -0.8975, -0.0009, -4.7614) (-0.8931, -0.4387, -0.0998, 4.2873) ( 0.0000, 0.0000, 0.0000, 1.0000)> ``` result_axis: `<Vector (-0.0448, -0.8975, -0.4387)>` result_roll: **8.284717296191957e-06** **Exact steps for others to reproduce the error** Run the py script in the attached blend file with the blender versions mentioned above.[AxisRollFromMatrix.blend](https://archive.blender.org/developer/F9358709/AxisRollFromMatrix.blend)

A. H. commented

2020-11-23 10:13:25 +01:00

Added subscriber: @csnyfan

blender-admin commented

2020-11-25 15:43:16 +01:00

This issue was referenced by 8330e19cb2

This issue was referenced by 8330e19cb2e7faca9884e34a8f46564250c8c29a

Germano Cavalcante commented

2020-11-25 15:57:50 +01:00

Added subscribers: @angavrilov, @mano-wii

Germano Cavalcante commented

2020-11-25 15:57:50 +01:00

I'm not familiar with this area and I do not know what results to expect.
But this change occurred in the commit d1657b40.
So, @angavrilov, could you shed some light here?

I'm not familiar with this area and I do not know what results to expect. But this change occurred in the commit d1657b40. So, @angavrilov, could you shed some light here?

Alexander Gavrilov commented

2020-11-25 16:19:19 +01:00

Well, this matrix is flipped with negative scale (determinant is -1), so technically AxisRollFromMatrix has no solution - there is no possible axis and roll it could return so that matrix == MatrixFromAxisRoll(*AxisRollFromMatrix(matrix)). The old code seemed to work because it was very simple and based on a lot of assumptions, while the new code includes a full conversion to quaternions.

Internally this is a non-issue for the actual armature code because rest pose cannot have scale, and may only potentially cause problems in a degenerate case for B-Bones, which were the cause of that fix in the first place due to previous math having issues in a way less degenerate case.

Personally I would lean to making the function error out if determinant is negative (or just documenting the restriction for the sake of performance), because there isn't a single 'best' way to unflip a matrix, so doing that is best left to the caller.

Well, this matrix is flipped with negative scale (determinant is -1), so technically AxisRollFromMatrix has no solution - there is no possible axis and roll it could return so that `matrix == MatrixFromAxisRoll(*AxisRollFromMatrix(matrix))`. The old code seemed to work because it was very simple and based on a lot of assumptions, while the new code includes a full conversion to quaternions. Internally this is a non-issue for the actual armature code because rest pose cannot have scale, and may only potentially cause problems in a degenerate case for B-Bones, which were the cause of that fix in the first place due to previous math having issues in a way less degenerate case. Personally I would lean to making the function error out if determinant is negative (or just documenting the restriction for the sake of performance), because there isn't a single 'best' way to unflip a matrix, so doing that is best left to the caller.

Germano Cavalcante commented

2020-11-25 16:39:43 +01:00

@angavrilov, would it be a good idea to set an error message in these cases?
Something like:

  if (!mat3_vec_to_roll(mat, r_axis, &tmp_roll)) {
     BKE_report(reports, RPT_ERROR, "Impossible to determine the rool of this matrix. Make sure it is not negative");
  }

@angavrilov, would it be a good idea to set an error message in these cases? Something like: ``` if (!mat3_vec_to_roll(mat, r_axis, &tmp_roll)) { BKE_report(reports, RPT_ERROR, "Impossible to determine the rool of this matrix. Make sure it is not negative"); } ```

A. H. commented

2020-11-25 16:54:52 +01:00

This is good to know - I wasn't aware that the function needed a positive determinant since the same data just worked on the old blender.
Raising an error and documenting it seems like the best solution.

This is good to know - I wasn't aware that the function needed a positive determinant since the same data just worked on the old blender. Raising an error and documenting it seems like the best solution.

Alexander Gavrilov commented

2020-11-25 16:57:25 +01:00

There is no error return - I didn't check, but this probably fails the matrix to quaternion conversion and outputs a zero rotation quaternion. To do a check you would actually have to compute the determinant of the matrix. Also, a sheared matrix isn't great either. The math behind this function basically expects a so called 'special orthogonal matrix', meaning there should be no scaling or shear, so the easiest solution without a performance impact is to adjust the documentation string.

If you really want to do a check it would be something like:

normalize_m3(matrix);
if (determinant_m3_array(matrix) < 0.9999f) { error "matrix is flipped or has shear" }

There is no error return - I didn't check, but this probably fails the matrix to quaternion conversion and outputs a zero rotation quaternion. To do a check you would actually have to compute the determinant of the matrix. Also, a sheared matrix isn't great either. The math behind this function basically expects a so called '*special orthogonal matrix*', meaning there should be no scaling or shear, so the easiest solution without a performance impact is to adjust the documentation string. If you really want to do a check it would be something like: ``` normalize_m3(matrix); if (determinant_m3_array(matrix) < 0.9999f) { error "matrix is flipped or has shear" } ```

Germano Cavalcante commented

2020-11-25 17:05:30 +01:00

Would this be a good description?:
"Convert a rotational matrix to the axis + roll representation. Note that the resulting roll value is unpredictable if the matrix has shear or is negative"

Would this be a good description?: `"Convert a rotational matrix to the axis + roll representation. Note that the resulting roll value is unpredictable if the matrix has shear or is negative"`

Alexander Gavrilov commented

2020-11-25 17:08:33 +01:00

According to wikipedia, "negative matrix" means all matrix elements are negative, so you have to say "has negative determinant" to be correct. In practical terms it means it mirrors/flips the object.