fixing rotation mat3 from euler bug

[rahwang]

Fixes #5195. We noticed that using fromHeadingPitchRoll created subtle distortions.

I double, double checked @zimengyang 's fix (see here: http://en.wikipedia.org/wiki/Conversion_between_quaternions_and_Euler_angles)

We should probably update our tests too... I'll get on that.

[pjcozzi]

Thanks for the fix.

Please update CHANGES.md and the unit tests. Please also run the unit tests locally to confirm in addition to the tests ran by CI.

[mramato]

Tests written this way are generally not useful because it just simply copies the same math done in Matrix3.fromHeadingPitchRoll. Most of the time there's a bug, it's because the bad math exists in both cases.

For cases like this, we prefer to use hardcoded known good values (usually created via a trusted source like STK Components, Matlab, or Wolfram Alpha. We then put a comment above the hardcoded values indicating where they are from.

[rahwang]

Ah, that makes sense. Annoyingly, I couldn't find any decent euler to rot matrix conversion tools online to reference. Does a test like this, comparing to other conversion functions in the class, work?

[mramato]

Unfortunately I don't think this approach is good either. It's still circular in that we're testing our own code against our own code. There may even be a chance that under the hood one of these are written in terms of the other (though I didn't check).

This test is also poor because it uses PI/2 and other "nice" values that can result in simple cases working fine but complicated cases being wrong. (I think some of the terms of the matrix end up being 0 for example).

Math unit tests like this should always contain hard numbers that have been validated to be correct either manually or via other trusted sources. I'll try and whip up a test case using STK Components.

[mramato]

@rahwang give this a try:

it('fromHeadingPitchRoll computed correctly', function() {
    // Expected generated via STK Components
    var expected = new Matrix3(
        0.754406506735489, 0.418940943945763, 0.505330889696038,
        0.133022221559489, 0.656295369162553, -0.742685314912828,
        -0.642787609686539, 0.627506871597133, 0.439385041770705);

    var headingPitchRoll = new HeadingPitchRoll(-CesiumMath.toRadians(10), -CesiumMath.toRadians(40), CesiumMath.toRadians(55));
    var result = new Matrix3();
    var returnedResult = Matrix3.fromHeadingPitchRoll(headingPitchRoll, result);
    expect(result).toBe(returnedResult);
    expect(returnedResult).toEqualEpsilon(expected, CesiumMath.EPSILON15);
});

[rahwang]

Gotcha. That makes sense. Thanks! Let me know if you don't have time to get to it for a while, I can always compute an ugly case manually. Manual matrix multiplication is painful, but will keep me strong. :)

[rahwang]

Thanks @mramato ! This test is way better, and everything checks out.

[mramato]

No problem, thanks again for fixing this.