Tidy up HeadingPitchRoll API #4498

twpayne · 2016-10-21T18:15:18Z

This PR uses the new HeadingPitchRoll objects introduced in #4047 more widely in the API. It also removes the aircraftHeadingPitchRoll* functions introduced in the same PR.

@pjcozzi, could you take a quick look at this to check that it's on the right track? If so, similar work needs to be done for headingPitchRollToQuaternion and aircraftHeadingPitchRollQuaternion.

…e, as documented.

…edFrame.

twpayne · 2016-10-21T18:15:49Z

This PR also addresses #4468.

pjcozzi · 2016-10-21T18:48:44Z

Apps/Sandcastle/gallery/development/Shadows.html

@@ -585,7 +585,8 @@
 }

 function createModel(url, origin) {
-    var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(origin, 0.0, 0.0, 0.0);
+    var hpr = new HeadingPitchRoll(0.0, 0.0, 0.0);


Since 0.0, 0.0, 0.0 is the default, can you change this throughout to just new HeadingPitchRoll ();

pjcozzi · 2016-10-21T18:49:27Z

Source/Core/Transforms.js

@@ -456,10 +458,12 @@ define([
     * direction where a positive angle is increasing eastward. Pitch is the rotation from the local east-north plane. Positive pitch angles
     * are above the plane. Negative pitch angles are below the plane. Roll is the first rotation applied about the local east axis.
     *
+     * You should pass a HeadingPitchRoll object.  Passing separate heading, pitch, and roll values is deprecated.


Remove this.

pjcozzi · 2016-10-21T18:49:39Z

Source/Core/Transforms.js

-     * @param {Number} heading The heading angle in radians.
-     * @param {Number} pitch The pitch angle in radians.
-     * @param {Number} roll The roll angle in radians.
+     * @param {HeadingPitchRoll|Number} hprOrHeading A HeadingPitchRoll or the heading angle in radians.


Document as just HeadingPitchRoll.

pjcozzi · 2016-10-21T18:50:56Z

Source/Core/Transforms.js

+            pitch = hprOrHeading.pitch;
+            roll = hprOrHeading.roll;
+        } else {
+            deprecationWarning('headingPitchRollToFixedFrame', 'headingPitchRollToFixedFrame with separate heading, pitch, and roll arguments was deprecated in 1.27.  It will be removed in 1.30.  Use a HeadingPitchRoll object.');


Submit an issue to remove this deprecated feature.

pjcozzi · 2016-10-21T18:51:58Z

Source/Core/Transforms.js

-    Transforms.headingPitchRollToFixedFrame = function(origin, heading, pitch, roll, ellipsoid, result) {
+    Transforms.headingPitchRollToFixedFrame = function(origin, hprOrHeading, pitch, roll, ellipsoid, result) {
+        var heading;
+        if (typeof hprOrHeading === 'object') {


Rename hprOrHeading to headingPitchRoll.

pjcozzi · 2016-10-21T18:52:08Z

Thanks @twpayne. Just those comments.

pjcozzi · 2016-10-21T19:14:21Z

Source/Core/Transforms.js

-     * @param {Number} pitch The pitch angle in radians.
-     * @param {Number} roll The roll angle in radians.
+     * @param {HeadingPitchRoll} headingPitchRoll A HeadingPitchRoll or the heading angle in radians.
+     * @param {Number?} pitch The pitch angle in radians if a HeadingPitchRoll object was not passed.


Sorry if I wasn't clear, let's also remove pitch and roll. Also generate the doc to doublecheck that it looks OK; it should be fine since I believe the tool will only look at these comments, not the function signature.

Done.

Confirmed that the generated documentation is as desired:

Cesium.Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, result) → Matrix4

pjcozzi · 2016-10-21T19:23:46Z

Can you fix the jsHint warnings?

Specs/Core/TransformsSpec.js
  line 364  col 13  'hpr' is defined but never used.
  line 895  col 13  'hpr' is already defined.

  ⚠  2 warnings

pjcozzi · 2016-10-21T19:26:54Z

Apps/Sandcastle/gallery/development/3D Models Node Explorer.html

 var origin = Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, height);
-var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(origin, heading, pitch, roll);
+var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(origin, new HeadingPitchRoll());


In all the Sandcastle examples, this should be Cesium.HeadingPitchRoll.

I caught this too :) Done.

…rnion.

pjcozzi · 2016-10-21T20:04:18Z

Looks good, thanks again @twpayne.

twpayne · 2016-10-21T20:06:59Z

Thanks! I created #4506 to track the removal in 1.30.

kaktus40 · 2016-10-23T06:19:32Z

Hello, I don't understand why did you remove aircraftHeadingPitchRoll* ? It answered to this case:
I think some people like me adress cesium for aeronautical uses and so the state of an aircraft is represented into a reference frame different of the classical East/North/Up. aircraftHeadingPitchRoll* is a sugar function that help this use case because you just need to address the heading, pitch, roll expressed in the classical aeronautical reference frame (North/West/Up) in order to work without more code lines!
What it annoys me the most is that my PR take a long time to be accepted and during this time I explained why I need these functions and without a discussion, there are removed in the next PR. It's raging :-(

twpayne · 2016-10-26T19:28:45Z

As the author of this PR, but not a representative of Cesium, the reasons for the clean-up were:

The two aircraftHeadingPitchRoll* functions were almost copy-and-pastes of the existing headingPitchRoll* functions, so it struck me that the API could be simplified by combining them.
Indeed, aircraftHeadingPitchRollQuaternion was a duplicate of headingPitchRollQuaternion.
aircraftPitchHeadingRollToFixedFrame was a near-duplicate of headingPitchRollToFixedFrame with the exception that the reference frame in the implementation was changed from east-north up to north-west up. The comment, however, was inconsistent with the implementation, again suggesting copy-and-paste.
Inconsistent use of units and reference frames leads to subtle and occasionally catastrophic bugs, such as what happened to the Mars Climate Orbiter.
For this reason, it made sense to me that Cesium should use a consistent reference frame in its API.
Cesium has a wide range of users across many domains. Although north-west up frames might be common in aviation, the API would quickly become large and confusing if each domain added functions to support its own particular conventions to the API.
The underlying change - a change of reference frame for two functions - is easily implemented in the client code that calls Cesium (e.g. by duplicating the functions and making the necessary changes).

kaktus40 · 2016-10-27T13:55:55Z

Hello, thanks for your reply.

For your three first points, I agree with you, it's almost a copy and paste of proven code. I just adjust the code to use the newly northWestUpToFixedFrame function.
For your fourth point, I am 100% with you. Now it's difficult to confound headingPitchRoll* with aircraftHeadingPitchRoll (suggested by Cesium team during the review of the code).
For the next two points, I think that Cesium API has taken a little shortcut that twists the way to work with: a matrix, a quaternion or heading/pitch/roll is just a mean to express a rotation (between two states, two frames...). In Cesium, it seems that the East/North/Up reference is a norm see here. I understand the choice which is like academic reference frame (x to the right, y to the up and z coming towards us). Now, I saw some helping requests in the forum to understand the way to work with the frames and how to make their awaited rotations. Obviously the responses are:
1. generate a local ENU,
2. whether make some rotations to attend your goal or convert your rotations in order to work with local ENU.
  I think that Cesium should help the client/user/developer with a common API: most people use heading compared with north and not with east. Moreover developers aren't necessarily expert in math and don't understand how to change referential frames (see forum questions...). Maybe the function should be named commonHeadingPitchroll* :-).
  Seriously, I personally think that headingPitchRollToFixedFrame should take as parameter the fixed frame to work with. By default, it should be ENU fixed frame.
  for example :

    Transforms.headingPitchRollToFixedFrame = function(origin, headingPitchRoll, pitch, roll, ellipsoid, result,fixedFrameConverter) {
        var heading;
        if (typeof headingPitchRoll === 'object') {
            // Shift arguments using assignments to encourage JIT optimization.
            ellipsoid = pitch;
            result = roll;
            heading = headingPitchRoll.heading;
            pitch = headingPitchRoll.pitch;
            roll = headingPitchRoll.roll;
        } else {
            deprecationWarning('headingPitchRollToFixedFrame', 'headingPitchRollToFixedFrame with separate heading, pitch, and roll arguments was deprecated in 1.27.  It will be removed in 1.30.  Use a HeadingPitchRoll object.');
            heading = headingPitchRoll;
        }
        // checks for required parameters happen in the called functions
        fixedFrameConverter=defaultValue(fixedFrameConverter,Transforms.eastNorthUpToFixedFrame)
        var hprQuaternion = Quaternion.fromHeadingPitchRoll(heading, pitch, roll, scratchHPRQuaternion);
        var hprMatrix = Matrix4.fromTranslationQuaternionRotationScale(Cartesian3.ZERO, hprQuaternion, scratchScale, scratchHPRMatrix4);
        result = fixedFrameConverter(origin, ellipsoid, result);
        return Matrix4.multiply(result, hprMatrix, result);
    };

Thus, there is no big changes. What do you think?

pjcozzi · 2016-10-30T15:53:04Z

@kaktus40 your suggestion has potential, but we would have to make a new function and put the fixedFrameConverter (probably renamed to fixedFrameTransform) before the result...or see if we could achieve this with more argument shifting for backwards compatibility until the deprecated parameters are removed.

1.27 is on Tuesday so it would be tight to try to make this change by then, but please consider opening a PR when you have time.

twpayne added 4 commits October 21, 2016 13:36

Allow headingPitchRollToFixedFrame to take a HeadingPitchRoll object.

8b72bf8

Fix aircraftHeadingPitchRollToFixedFrame to use an east-north up fram…

35bb5cc

…e, as documented.

Make aircraftHeadingPitchRollToFixedFrame alias headingPitchRollToFix…

c58317c

…edFrame.

Remove aircraftHeadingPitchRollToFixedFrame.

b9b0669

twpayne added 2 commits October 21, 2016 14:45

Use HeadingPitchRoll in specs.

f20531d

Use HeadingPitchRoll in Sandcastle apps.

718e41a

pjcozzi reviewed Oct 21, 2016

View reviewed changes

twpayne added 3 commits October 21, 2016 14:59

Use default HeadingPitchRoll values.

5eec6a0

Update headingPitchRollToFixedFrame documentation.

9124171

Rename function argument.

3c799c5

pjcozzi reviewed Oct 21, 2016

View reviewed changes

hpinkos added the bug bash label Oct 21, 2016

twpayne added 9 commits October 21, 2016 15:50

Use more HeadingPitchRoll objects in tests.

09bfcb6

Remove deprecated parameters from documentation.

9544f6a

Allow headingPitchRollQuaternion to take a HeadingPitchRoll argument.

7bff93f

Use HeadingPitchRoll in Sandcastle apps.

d022981

Add missing Cesium namespace.

51c011b

Replace aircraftHeadingPitchRollQuaternion with headingPitchRollQuate…

e0319eb

…rnion.

Remove aircraftHeadingPitchRollQuaternion.

f20b28a

Add change note to CHANGES.md.

72d40d4

Remove unused variable.

33a8b41

pjcozzi merged commit 7e7fd34 into CesiumGS:master Oct 21, 2016

twpayne mentioned this pull request Oct 21, 2016

Remove separate heading, pitch, roll value support in 1.30 #4506

Closed

twpayne deleted the heading-pitch-roll branch October 21, 2016 20:07

twpayne mentioned this pull request Oct 26, 2016

about PR 4498 #4527

Closed

kaktus40 mentioned this pull request Jan 2, 2017

Quaternion.fromHeadingPitchRoll uses wrong axis? #3256

Closed

kaktus40 mentioned this pull request Jan 20, 2017

Fixed frame converter #4896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy up HeadingPitchRoll API #4498

Tidy up HeadingPitchRoll API #4498

twpayne commented Oct 21, 2016

twpayne commented Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi commented Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi commented Oct 21, 2016

pjcozzi Oct 21, 2016

twpayne Oct 21, 2016

pjcozzi commented Oct 21, 2016

twpayne commented Oct 21, 2016

kaktus40 commented Oct 23, 2016

twpayne commented Oct 26, 2016

kaktus40 commented Oct 27, 2016

pjcozzi commented Oct 30, 2016

Tidy up HeadingPitchRoll API #4498

Tidy up HeadingPitchRoll API #4498

Conversation

twpayne commented Oct 21, 2016

twpayne commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 21, 2016

twpayne commented Oct 21, 2016

kaktus40 commented Oct 23, 2016

twpayne commented Oct 26, 2016

kaktus40 commented Oct 27, 2016

pjcozzi commented Oct 30, 2016