Page MenuHome
Differential D9410 Diff 30780 source/blender/blenkernel/intern/armature.c

Changeset View

Standalone View

View Options

source/blender/blenkernel/intern/armature.c

Show First 20 Lines • Show All 2,150 Lines • ▼ Show 20 Lines
* <pre> * <pre>
* ┌ z^2 - x^2, -2 * x * z ┐ * ┌ z^2 - x^2, -2 * x * z ┐
* M* = 1 / (x^2 + z^2) * │ │ * M* = 1 / (x^2 + z^2) * │ │
* └ -2 * x * z, x^2 - z^2 ┘ * └ -2 * x * z, x^2 - z^2 ┘
* </pre> * </pre>
*/ */
void vec_roll_to_mat3_normalized(const float nor[3], const float roll, float r_mat[3][3]) void vec_roll_to_mat3_normalized(const float nor[3], const float roll, float r_mat[3][3])
{ {
#define THETA_THRESHOLD_NEGY 1.0e-9f const float THETA_CRITICAL = 1.0e-9f; /* theta must be above this value */
#define THETA_THRESHOLD_NEGY_CLOSE 1.0e-5f const float THETA_SAFE = 1.0e-5f; /* theta above this value are safe to use */
sybrenUnsubmitted
Done
Inline Actions

Any particular reason this is static? The good thing about #define is that it's all handled by the precompiler.

sybren: Any particular reason this is `static`? The good thing about `#define` is that it's all handled…
gaiaclaryAuthorUnsubmitted
Done
Inline Actions

Any particular reason this is static? The good thing about #define is that it's all handled by the precompiler.

maybe it was a misunderstanding from a comment from sergey. I removed the 'static'. I still prefer to have it a regular variable.

gaiaclary: > Any particular reason this is `static`? The good thing about `#define` is that it's all…
sybrenUnsubmitted
Done
Inline Actions

Comments in C files should be /* */ style.

sybren: Comments in C files should be `/* */` style.
sybrenUnsubmitted
Done
Inline Actions

Comments should be a full sentence, so start with capital and end with period.

sybren: Comments should be a full sentence, so start with capital and end with period.
sybrenUnsubmitted
Done
Inline Actions

I think the change in name to "safe" and "critical" is really nice.

sybren: I think the change in name to "safe" and "critical" is really nice.
float theta; const float x = nor[0];
const float y = nor[1];
const float z = nor[2];
const float theta = 1.0f + y;
const float theta_alt = x * x + z * z;
float rMatrix[3][3], bMatrix[3][3]; float rMatrix[3][3], bMatrix[3][3];
BLI_ASSERT_UNIT_V3(nor); BLI_ASSERT_UNIT_V3(nor);
theta = 1.0f + nor[1]; /* When theta is close to zero (nor is aligned close to negative Y Axis),
sybrenUnsubmitted
Done
Inline Actions

Not for this patch, but this comment is a bit mind-boggling to me.

  1. Why does it matter what old code did when it's no longer around?
  2. Why is this comment saying it uses two different threshold values, when one of the two is actually the same as used before?
sybren: Not for this patch, but this comment is a bit mind-boggling to me. 1. Why does it matter what…
sybrenUnsubmitted
Done
Inline Actions

These don't change value any more, so you can declare them as const:

const float theta = 1.0f + y;
const float theta_alt = x * x + z * z;
sybren: These don't change value any more, so you can declare them as `const`: ``` const float theta =…
sybrenUnsubmitted
Not Done
Inline Actions

Calculating theta_alt can be deferred to the else clause where it's actually used. Might save a few CPU ticks (although this might be optimized by the compiler as well).

sybren: Calculating `theta_alt` can be deferred to the `else` clause where it's actually used. Might…
gaiaclaryAuthorUnsubmitted
Done
Inline Actions

Calculating theta_alt can be deferred to the else clause where it's actually used. Might save a few CPU ticks (although this might be optimized by the compiler as well).

This is true for now, but when the bug fix is applied, then we need theta_alt already in the if clause.

gaiaclary: > Calculating `theta_alt` can be deferred to the `else` clause where it's actually used. Might…
* we have to check we do have non-null X/Z components as well.
sybrenUnsubmitted
Done
Inline Actions

Instead of

(due to float precision errors, we can have nor = (0.0, -0.99999994, 0.0)...).

I think it's nicer to remove the "..." and explain why this is a problem:

Due to float precision errors, nor can be (0.0, -0.99999994, 0.0) which results in theta being almost zero.
This will cause problems when used as divisor.

sybren: Instead of > (due to float precision errors, we can have nor = (0.0, -0.99999994, 0.0)...).
/* With old algo, 1.0e-13f caused T23954 and T31333, 1.0e-6f caused T27675 and T30438, * Also, due to float precision errors, nor can be (0.0, -0.99999994, 0.0) which results
* so using 1.0e-9f as best compromise. * in theta being close to zero. This will cause problems when theta is used as divisor.
*
* New algo is supposed much more precise, since less complex computations are performed,
* but it uses two different threshold values...
*
* Note: When theta is close to zero, we have to check we do have non-null X/Z components as well
* (due to float precision errors, we can have nor = (0.0, 0.99999994, 0.0)...).
*/ */
if (theta > THETA_THRESHOLD_NEGY_CLOSE || ((nor[0] || nor[2]) && theta > THETA_THRESHOLD_NEGY)) { if (theta > THETA_SAFE || ((x || z) && theta > THETA_CRITICAL)) {
/* nor is *not* -Y. /* nor is *not* aligned to negative Y-axis (0,-1,0).
sybrenUnsubmitted
Done
Inline Actions

Instead of -Y I think not (0, -1, 0) is clearer. Or "the -Y vector", whichever you prefer.

sybren: Instead of `-Y` I think `not (0, -1, 0)` is clearer. Or "the -Y vector", whichever you prefer.
* We got these values for free... so be happy with it... ;) * We got these values for free... so be happy with it... ;)
*/ */
bMatrix[0][1] = -nor[0];
bMatrix[1][0] = nor[0]; bMatrix[0][1] = -x;
bMatrix[1][1] = nor[1]; bMatrix[1][0] = x;
bMatrix[1][2] = nor[2]; bMatrix[1][1] = y;
bMatrix[2][1] = -nor[2]; bMatrix[1][2] = z;
if (theta > THETA_THRESHOLD_NEGY_CLOSE) { bMatrix[2][1] = -z;
/* If nor is far enough from -Y, apply the general case. */
bMatrix[0][0] = 1 - nor[0] * nor[0] / theta; if (theta > THETA_SAFE) {
bMatrix[2][2] = 1 - nor[2] * nor[2] / theta; /* nor differs significantly from negative Y axis (0,-1,0): apply the general case. */
bMatrix[2][0] = bMatrix[0][2] = -nor[0] * nor[2] / theta; bMatrix[0][0] = 1 - x * x / theta;
bMatrix[2][2] = 1 - z * z / theta;
bMatrix[2][0] = bMatrix[0][2] = -x * z / theta;
} }
else { else {
/* If nor is too close to -Y, apply the special case. */ /* nor is close to negative Y axis (0,-1,0): apply the special case. */
theta = nor[0] * nor[0] + nor[2] * nor[2]; bMatrix[0][0] = (x + z) * (x - z) / -theta_alt;
bMatrix[0][0] = (nor[0] + nor[2]) * (nor[0] - nor[2]) / -theta;
bMatrix[2][2] = -bMatrix[0][0]; bMatrix[2][2] = -bMatrix[0][0];
bMatrix[2][0] = bMatrix[0][2] = 2.0f * nor[0] * nor[2] / theta; bMatrix[2][0] = bMatrix[0][2] = 2.0f * x * z / theta_alt;
} }
} }
else { else {
/* If nor is -Y, simple symmetry by Z axis. */ /* nor is very close to negative Y axis (0,-1,0): use simple symmetry by Z axis. */
sybrenUnsubmitted
Done
Inline Actions

"very close to" includes "at zero distance", so "nor is very close to -Y" is enough.

sybren: "very close to" includes "at zero distance", so "nor is very close to -Y" is enough.
unit_m3(bMatrix); unit_m3(bMatrix);
bMatrix[0][0] = bMatrix[1][1] = -1.0; bMatrix[0][0] = bMatrix[1][1] = -1.0;
} }
/* Make Roll matrix */ /* Make Roll matrix */
axis_angle_normalized_to_mat3(rMatrix, nor, roll); axis_angle_normalized_to_mat3(rMatrix, nor, roll);
/* Combine and output result */ /* Combine and output result */
mul_m3_m3m3(r_mat, rMatrix, bMatrix); mul_m3_m3m3(r_mat, rMatrix, bMatrix);
#undef THETA_THRESHOLD_NEGY
#undef THETA_THRESHOLD_NEGY_CLOSE
} }
void vec_roll_to_mat3(const float vec[3], const float roll, float r_mat[3][3]) void vec_roll_to_mat3(const float vec[3], const float roll, float r_mat[3][3])
{ {
float nor[3]; float nor[3];
normalize_v3_v3(nor, vec); normalize_v3_v3(nor, vec);
vec_roll_to_mat3_normalized(nor, roll, r_mat); vec_roll_to_mat3_normalized(nor, roll, r_mat);
▲ Show 20 Lines • Show All 701 Lines • Show Last 20 Lines