Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing rotation mat3 from euler bug #5202

Merged
merged 5 commits into from
Apr 20, 2017
Merged

fixing rotation mat3 from euler bug #5202

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c4ac5d
fixing rotation mat3 from euler bug
rahwang Apr 12, 2017
2524505
adding fromHeadingPitchRoll test
rahwang Apr 19, 2017
044f1c9
Merge branch 'master' into headingPitchRoll-bug-fix
rahwang Apr 19, 2017
f49f5fd
changing test
rahwang Apr 19, 2017
d9e64c8
updating test with hard coded vals
rahwang Apr 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Change Log

* Added `disableDepthTestDistance` to billboards, points and labels. This sets the distance to the camera where the depth test will be disabled. Setting it to zero (the default) will alwasy enable the depth test. Setting it to `Number.POSITVE_INFINITY` will never enabled the depth test. Also added `scene.minimumDisableDepthTestDistance` to change the default value from zero. [#5166](https://github.com/AnalyticalGraphicsInc/cesium/pull/5166)
* Fixed issue with displaying `MapboxImageryProvider` default token error message [#5191](https://github.com/AnalyticalGraphicsInc/cesium/pull/5191)
* Fixed bug in conversion formula in Matrix3.fromHeadingPitchRoll [5195](https://github.com/AnalyticalGraphicsInc/cesium/issues/5195)
* Added a `depthFailMaterial` property to line entities, which is the material used to render the line when it fails the depth test. [#5160](https://github.com/AnalyticalGraphicsInc/cesium/pull/5160)
* Upgrade FXAA to version 3.11. [#5200](https://github.com/AnalyticalGraphicsInc/cesium/pull/5200)
* `Scene.pickPosition` now caches results per frame to increase performance [#5117](https://github.com/AnalyticalGraphicsInc/cesium/issues/5117)
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/Matrix3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ define([

var m10 = cosTheta * sinPsi;
var m11 = cosPhi * cosPsi + sinPhi * sinTheta * sinPsi;
var m12 = -sinTheta * cosPhi + cosPhi * sinTheta * sinPsi;
var m12 = -sinPhi * cosPsi + cosPhi * sinTheta * sinPsi;

var m20 = -sinTheta;
var m21 = sinPhi * cosTheta;
Expand Down
18 changes: 18 additions & 0 deletions Specs/Core/Matrix3Spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,24 @@ defineSuite([
expect(returnedResult).toEqualEpsilon(expected, CesiumMath.EPSILON15);
});

it('fromHeadingPitchRoll computed correctly', function() {
var sPi = Math.sin(CesiumMath.PI);
var cPi = Math.cos(CesiumMath.PI);
var sPiOver2 = Math.sin(CesiumMath.PI_OVER_TWO);
var cPiOver2 = Math.cos(CesiumMath.PI_OVER_TWO);
var s3PiOver2 = Math.sin(CesiumMath.PI_OVER_TWO * 3);
var c3PiOver2 = Math.cos(CesiumMath.PI_OVER_TWO * 3);

var headingPitchRoll = new HeadingPitchRoll(-CesiumMath.PI_OVER_TWO, -CesiumMath.PI, CesiumMath.PI_OVER_TWO * 3);
var expected = new Matrix3(cPi * cPiOver2, -c3PiOver2 * sPiOver2 + s3PiOver2 * sPi * cPiOver2, s3PiOver2 * sPiOver2 + c3PiOver2 * sPi * cPiOver2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rahwang rahwang Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

cPi * sPiOver2, c3PiOver2 * cPiOver2 + s3PiOver2 * sPi * sPiOver2, -s3PiOver2 * cPiOver2 + c3PiOver2 * sPi * sPiOver2,
-sPi, s3PiOver2 * cPi, c3PiOver2 * cPi);
var result = new Matrix3();
var returnedResult = Matrix3.fromHeadingPitchRoll(headingPitchRoll, result);
expect(result).toBe(returnedResult);
expect(returnedResult).toEqualEpsilon(expected, CesiumMath.EPSILON15);
});

it('fromScale works without a result parameter', function() {
var expected = new Matrix3(
7.0, 0.0, 0.0,
Expand Down