Core: Fix Basis::get_euler incorrectly simplifying rotations in some cases.

[Cwazywierdo]

Fixes #49839

A large testing threshold would sometimes cause Basis::get_euler to incorrectly assume a rotation could simplified. This would cause certain rotations with a range of +-0.26° to be effectively truncated, depending on the rotation order and axis. Using the MRP from #49839 for testing, I determined that 0.00000025 was the largest threshold where the truncation was visually unnoticeable (Range of +-0.04°).

Master	PR
Threshold: 0.00001 (CMP_EPSILON)	Threshold: 0.00000025
Error Range: +-0.26°	Error Range: +-0.04°

[TokageItLab]

BTW, this is not related to this PR, but I remember that Quaternion's is_normalized() has a similar accuracy problem, so this might be a good opportunity to rethink the epsilon values.

[lawnjelly]

My first thoughts are, why is this epsilon needed at all? (Don't get me wrong, it could be required for detecting gimbal lock, but would be nice to confirm.)

If it is for asin (which errors outside -1 to 1), the Math::asin() is a safe version which won't error. I do however notice that for YXZ at line 540 it doesn't seem to be using the Math versions, that should be corrected, as we should prefer the Math versions especially for asin / acos because they are safe.

I don't know if atan2 has any strange out of range behaviour that the epsilons helped with, but from memory atan2 is fairly robust (unless x and y are zero).

Here's a gemini generated version for comparison:

// Function to extract Euler angles (ZYX order - Yaw, Pitch, Roll) from a rotation matrix (basis)
Eigen::Vector3d getEulerAnglesZYX(const Eigen::Matrix3d& rotationMatrix) {
    double yaw, pitch, roll;

    // Check for singularity (gimbal lock) -  pitch close to +/- 90 degrees
    double sinPitch = -rotationMatrix(2, 0); //sin(pitch)
    if (std::abs(sinPitch) >= 1.0) {
        // Gimbal lock:  pitch = +/- 90 degrees.  Roll becomes undefined/arbitrary.
        pitch = std::copysign(M_PI / 2.0, sinPitch); // pitch = +/- pi/2
        yaw = std::atan2(-rotationMatrix(0, 2), rotationMatrix(1, 2)); // yaw becomes rotation around Z
        roll = 0.0;  // Roll is undefined, set to 0 or any other value.
        // Or handle differently, e.g., return a special "gimbal lock" indicator.
    } else {
        pitch = std::asin(sinPitch);
        yaw = std::atan2(rotationMatrix(2, 1), rotationMatrix(2, 2));
        roll = std::atan2(rotationMatrix(1, 0), rotationMatrix(0, 0));
    }

    return Eigen::Vector3d(yaw, pitch, roll); // Yaw, Pitch, Roll
}

That said, this PR seems an improvement on the current behaviour and there's no reason we can't evaluate whether epsilons are required in a later PR. 👍

[aaronfranke]

With this code, in single-precision, this means that the value is allowed to be up to 3 ULPs away in order to pass the check and run the simpler code (the expression 1.0f - epsilon is 4 ULPs away from 1.0f). This is a fairly narrow margin of error if we expect to be working with values calculated from other things.

However, in this particular case, the point of these checks is to handle the common cases where Euler angle rotation happens in only one direction / around one axis with better accuracy. In those cases, we expect the values to be exact, because they shouldn't be altered (if you rotate in the XY plane around Z, then Z shouldn't be changed). This purpose is actually already evident by the == 0 checks under the // is this a pure Y rotation? comments.

I wrote similar code last year for my 4D module for decomposing a Basis4D into 4D Euler angles (6 numbers), though it's simpler in that it only handles one Euler order. Suffice to say, the purpose of this code is pretty clear to me.

Anyway, this means that a more strict check should be absolutely fine with the expected use case. Even exact equality should still do the trick (as @lawnjelly mentions above). As the OP shows, the current check is too broad since it produces visual errors. Besides, the general code path still works even for the special cases. Therefore I think this is a good change, I don't think there are any downsides.

The only notes I have is that I would add to the comment to mention that 1.0 minus this value is 4 ULPs away from 1.0 in single precision, to give a 3 ULPs margin (maybe this can be worded more concisely / simply), and that @lawnjelly's comment above about potentially not needing an epsilon here at all is very valid too.

[Cwazywierdo]

While I was trying to determine the ideal epsilon value, I actually did consider getting rid of it entirely. When I looked at the PR that introduced it though (#39483), the author claimed that it solved a nondescript "floating point issue." Without any other information on the actual issue, I figured it would be better to keep it and avoid a possible regression. I will change this if it's considered unnecessary.

I will definitely go ahead and change those function calls to the Math versions though.

[lawnjelly]

Worth tagging @AndreaCatania and @RevoluPowered , I don't know if they are still contributing but they might remember why the epsilon was introduced.

[Repiteo]

We'll send it as-is for the time being. If we want global consistency in the future, there's always CMP_EPSILON2; though that might be better suited for a PR focused on the math constants as a whole

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉