Fixed frame converter #4896

kaktus40 · 2017-01-20T23:43:32Z

In continuity of #4498 and in response for #3256 , I implemented an ability to express a rotation around a local frame different of ENU.

pjcozzi · 2017-01-21T14:44:04Z

Thanks for this big contribution, @kaktus40!

@bagnell could you please review this?

@xtassin @speigg your input here is also very much appreciated.

pjcozzi · 2017-01-21T14:47:20Z

@kaktus40 could you also please update CHANGES.md and resize the new images in the Apps/Sandcastle/gallery/ directory to be the same as the other Sandcastle thumbnails?

kaktus40 · 2017-01-22T09:26:40Z

@pjcozzi :it's done

kaktus40 · 2017-01-24T14:12:17Z

Hello, after playing with LocalToFixedFrame gallery example, I fear there is another misconception in Cesium. Indeed, heading value is the rotation angle around the third axis (for example in East North Up local reference frame, it's the rotation angle around "Up"). As the reference frame is direct, a positive value of heading means a rotation from first axis (respectively east) to second axis (respectively north) in the plane generated by first and second axis, see here. In LocalToFixedFrame gallery example when you turn heading positively, we should expect that the model turns from east to north in ENU representation but it's the opposite. After checking the code I found this: I don't understand why it's -heading that is used and not simply heading. It's the same remark for pitch here

kaktus40 · 2017-02-05T07:18:31Z

Hello, any news for this PR?

hpinkos · 2017-02-06T13:43:34Z

Sorry we haven't had a chance to look at this yet @kaktus40. We might have a chance to at the bug bash this week

bagnell

Looks good. Thanks @kaktus40!

Does anyone else want to review this?

bagnell · 2017-02-08T19:33:28Z

CHANGES.md

@@ -5,6 +5,12 @@ Change Log

 * Deprecated
    * The properties `url` and `key` will be removed from `GeocoderViewModel` in 1.31. These properties will be available on geocoder services that support them, like `BingMapsGeocoderService`.
+    * The function `createBinormalAndBitangent` of `GeometryPipeline` will be removed in 1.31. Use the function `createTangentAndBitangent` instead. [#4856](https://github.com/AnalyticalGraphicsInc/cesium/pull/4856)


You have a couple duplicates here.

bagnell · 2017-02-08T19:37:00Z

Source/Core/Quaternion.js

@@ -184,17 +188,29 @@ define([
     * @param {Quaternion} [result] The object onto which to store the result.
     * @returns {Quaternion} The modified result parameter or a new Quaternion instance if none was provided.
     */
-    Quaternion.fromHeadingPitchRoll = function(heading, pitch, roll, result) {
+    Quaternion.fromHeadingPitchRoll = function(headingOrHeadingPitchRoll, pitchOrResult, roll, result) {


Use an options parameter here.

When you replace the usage throughout the code, create a scratch object literal to avoid many allocations and garbage collection.

Ah I see the deprecation below. Can you add the deprecation to CHANGES.md? Also, move the deprecation warning into the if-block so the warning only occurs when called with heading, pitch and roll arguments.

I don't see how to use options parameter in Quaternion.fromHeadingPitchRoll as the first parameter will be a HeadingPitchRoll object?
You suggest that rollQuaternion, pitchQuaternion and headingQuaternion become scratch variables? Ok for me but just for information that was the previous design.

This looks good. I didn't realize when I made the first comment that it was deprecating those parameters.

bagnell · 2017-02-08T19:56:47Z

Source/Core/Transforms.js

-            result[15] = 1.0;
-            return result;
+        if (!defined(result)) {
+          result = new Matrix4();


Whitespace. Each indent is 4 spaces.

bagnell · 2017-02-08T19:57:09Z

Source/Core/Transforms.js

+        throw new DeveloperError('firstAxis and secondAxis must be east, north, up, west, south or down.');
+      }
+      var thirdAxis = vectorProductLocalFrame[firstAxis][secondAxis];
+      var calculateCartesian = {


These can be file scope scratches.

bagnell · 2017-02-08T19:57:52Z

Source/Core/Transforms.js

+       * @param {Matrix4} [result] The object onto which to store the result.
+       * @returns {Matrix4} The modified result parameter or a new Matrix4 instance if none was provided.
+       */
+      var resultat= function(origin, ellipsoid, result) {


Would it be better to generate these on load? If not, cache the functions.

Sorry I don't see what you mean :-(

This function will be generated each time localFrameToFixedFrameGenerator is called. So every call like: Cesium.Transforms.localFrameToFixedFrameGenerator('north','west') will generate a new function but it doesn't change. Create a file scope object like localFrameToFixedFrameCache with unique names based on the axis. First you check if the function is in the cache, if it is, return it; if not, generate it and place it in the cache before returning it.

bagnell · 2017-02-08T20:24:30Z

Source/Core/Transforms.js

-        var pitch = headingPitchRoll.pitch;
-        var roll = headingPitchRoll.roll;
+    Transforms.headingPitchRollToFixedFrame = function(origin, headingPitchRoll, ellipsoid, fixedFrameTransformOrResult, result) {
+        Check.typeOf.object( 'HeadingPitchRoll', headingPitchRoll);


This should go inside the pragmas to be removed from release builds.

bagnell · 2017-02-08T20:26:52Z

Source/Core/Transforms.js


        // checks for required parameters happen in the called functions
-        var hprQuaternion = Quaternion.fromHeadingPitchRoll(heading, pitch, roll, scratchHPRQuaternion);
+        deprecationWarning('Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, result)', 'The method was deprecated in Cesium 1.31 and will be removed in version 1.32. ' +


Same comment.

bagnell · 2017-02-08T20:27:55Z

Source/Core/Transforms.js

        // checks for required parameters happen in the called functions
-        Check.typeOf.object('headingPitchRoll', headingPitchRoll);
-        var transform = Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, scratchENUMatrix4);
+        Check.typeOf.object( 'HeadingPitchRoll', headingPitchRoll);


Same comments.

bagnell · 2017-02-08T20:29:30Z

Source/Scene/Camera.js

@@ -1160,14 +1164,16 @@ define([
        var pitch = defaultValue(orientation.pitch, -CesiumMath.PI_OVER_TWO);
        var roll = defaultValue(orientation.roll, 0.0);

+        var hpr = new HeadingPitchRoll(heading, pitch, roll);


Create a scratch variable.

bagnell · 2017-02-08T20:30:35Z

Specs/Core/QuaternionSpec.js

@@ -652,7 +660,7 @@ defineSuite([
            Quaternion.fromRotationMatrix(undefined);
        }).toThrowDeveloperError();
    });
-
+/*


Did you mean to leave these commented out?

As fromHeadingPitchRoll use now a headingPitchRoll class, the commented tests have no interest.

pjcozzi · 2017-02-08T21:30:31Z

Does anyone else want to review this?

Just @bagnell's review is OK with me.

@kaktus40 if you are able to update this tomorrow or Friday morning, it will likely get merged quickly since we are having a bug bash and focused on merging PRs.

Thanks again for this!

kaktus40 · 2017-02-08T23:18:07Z

Ok I corrected the code in compliance with the review code

bagnell · 2017-02-09T15:23:50Z

Source/Core/Quaternion.js

-        Check.typeOf.number('pitch', pitch);
-        Check.typeOf.number('roll', roll);
+        if (headingOrHeadingPitchRoll instanceof HeadingPitchRoll) {
+          Check.typeOf.object('headingPitchRoll',headingOrHeadingPitchRoll );


There are still whitespace issues. Four spaces should be used for each indent. Please update throughout.

kaktus40 · 2017-02-09T23:58:34Z

I made the corrections

bagnell · 2017-02-10T17:00:05Z

Thanks @kaktus40!

kaktus40 added 6 commits January 18, 2017 08:55

First implementation of fixedFrameConverter

0c3aa34

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

5deaa1d

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

ee72b62

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

a32fc35

adding ability to change the local frame for orientations

d1d9206

update the examples

603ef86

kaktus40 added 2 commits January 22, 2017 08:10

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

8972e73

update CHANGES.md and resize images in gallery

85dedd0

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

0c6dd0f

kaktus40 added 6 commits January 30, 2017 10:15

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

acb5273

update CHANGES.md

b22cf3e

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

1e66422

correct checks

ee49f0b

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

f75c5b7

correct changes

8f4edca

pjcozzi added the priority - high label Feb 6, 2017

bagnell reviewed Feb 8, 2017

View reviewed changes

kaktus40 added 3 commits February 8, 2017 22:34

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

eb80917

update changes.md

c6df6cc

update in regards of the code review

17f8a3f

bagnell reviewed Feb 9, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fixedFrameConverter

996fb36

made a cache for localFrameToFixedFrameGenerator

96cbf0f

bagnell added 2 commits February 10, 2017 11:14

Merge branch 'master' into fixedFrameConverter

f713212

Fix whitespace and remove duplicate entries from CHANGES.md.

1ca538a

bagnell merged commit 03bc20f into CesiumGS:master Feb 10, 2017

kaktus40 deleted the fixedFrameConverter branch February 12, 2017 14:20

shunter mentioned this pull request May 1, 2017

Quaternion.fromHeadingPitchRoll documentation is incorrect #5264

Closed

kaktus40 mentioned this pull request Sep 3, 2017

Quaternion.fromHeadingPitchRoll uses wrong axis? #3256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed frame converter #4896

Fixed frame converter #4896

kaktus40 commented Jan 20, 2017

pjcozzi commented Jan 21, 2017

pjcozzi commented Jan 21, 2017

kaktus40 commented Jan 22, 2017

kaktus40 commented Jan 24, 2017

kaktus40 commented Feb 5, 2017

hpinkos commented Feb 6, 2017

bagnell left a comment

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

kaktus40 Feb 8, 2017

bagnell Feb 9, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

kaktus40 Feb 8, 2017

bagnell Feb 9, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

bagnell Feb 8, 2017

kaktus40 Feb 8, 2017

pjcozzi commented Feb 8, 2017

kaktus40 commented Feb 8, 2017

bagnell Feb 9, 2017

kaktus40 commented Feb 9, 2017

bagnell commented Feb 10, 2017

Fixed frame converter #4896

Fixed frame converter #4896

Conversation

kaktus40 commented Jan 20, 2017

pjcozzi commented Jan 21, 2017

pjcozzi commented Jan 21, 2017

kaktus40 commented Jan 22, 2017

kaktus40 commented Jan 24, 2017

kaktus40 commented Feb 5, 2017

hpinkos commented Feb 6, 2017

bagnell left a 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

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

Choose a reason for hiding this comment

pjcozzi commented Feb 8, 2017

kaktus40 commented Feb 8, 2017

Choose a reason for hiding this comment

kaktus40 commented Feb 9, 2017

bagnell commented Feb 10, 2017