make Heading pitch roll counter clock wise again 2 #5809

kaktus40 · 2017-09-03T08:20:54Z

following this and that

…rnion

cesium-concierge · 2017-09-03T08:20:55Z

@kaktus40, thanks for the pull request! Maintainers, we have a signed CLA from @kaktus40, so you can review this at any time.

I am a bot who helps you make Cesium awesome! Thanks again.

ggetz

Thanks @kaktus40! I have a few comments.

ggetz · 2017-09-06T13:55:00Z

Apps/Sandcastle/gallery/HeadingPitchRoll.html

@@ -208,7 +208,7 @@
    speedVector = Cesium.Cartesian3.multiplyByScalar(Cesium.Cartesian3.UNIT_X, speed / 10, speedVector);
    position = Cesium.Matrix4.multiplyByPoint(planePrimitive.modelMatrix, speedVector, position);
    pathPosition.addSample(Cesium.JulianDate.now(), position);
-    Cesium.Transforms.headingPitchRollToFixedFrame(position, hpRoll, Cesium.Ellipsoid.WGS84, fixedFrameTransform, planePrimitive.modelMatrix);
+    Cesium.Transforms.directHeadingPitchRollToFixedFrame(position, hpRoll, Cesium.Ellipsoid.WGS84, fixedFrameTransform, planePrimitive.modelMatrix);


For directHeadingPitchRollToFixedFrane, do we need to modify the parameters for these two functions at all like you are doing everywhere else you're using the new functions?

directHeadingPitchRollToFixedFrame takes the same parameters than headingPitchRollToFixedFrame. Internally, it uses Quaternion.fromDirectHeadingPitchRoll instead of Quaternion.fromHeadingPitchRoll. For now in this PR we have two groups of functions that we shouldn't mix. The first group, that should be deprecated, has the next functions:

HeadingPitchRoll.fromQuaternion,

Matrix3.fromHeadingPitchRoll,

Quaternion.fromHeadingPitchRoll,

Transforms.HeadingPitchRollToFixedFrame.

The newcomer group, that should replace the first group, has the next functions:

HeadingPitchRoll.fromDirectQuaternion,

Matrix3.fromDirectHeadingPitchRoll,

Quaternion.fromDirectHeadingPitchRoll,

Transforms.directHeadingPitchRollToFixedFrame.

A function in the first group and its equivalent in the second group takes the same parameters in the same orders and return the same type of result (HeadingPitchRoll, Matrix3, Quaternion or Matrix4 instances respectively). The only difference is the sense of rotation for heading and pitch used internally.

Normally, I checked in my IDE that each instance of the first group functions and if needed, I changed for the equivalent in the second group. Also I didn't replace the functions when the parameters where heading=0, pitch=0 because there will be no change in their behaviour.

ggetz · 2017-09-06T13:59:52Z

Source/Core/HeadingPitchRoll.js

+            throw new DeveloperError('quaternion is required');
+        }
+        //>>includeEnd('debug');
+        deprecationWarning('HeadingPitchRoll.fromDirectQuaternion', 'This HeadingPitchRoll.fromDirectQuaternion works in the classical interpretation used in mathematics. This function will replaced HeadingPitchRoll.fromQuaternion in 1.43.');


You shouldn't include the deprecation warning in the new functions. Instead include this additional information about how it's been calculated in the documentation.

I just updated following your recommandations

ggetz · 2017-09-07T14:33:10Z

Alright, thanks @kaktus40! This looks good to me.

Make sure before we merge this you submit issues to remove the depreciated function in future versions as we specify in the Coding Guide in the Deprecation section

@pjcozzi Does this look good to merge?

…rnion

hpinkos · 2017-09-07T21:19:53Z

CHANGES.md

+* Breaking changes
+
+* Deprecated
+  * `HeadingPitchRoll.fromDirectQuaternion` is deprecated and his behaviour will replace `HeadingPitchRoll.fromQuaternion` behaviour in 1.43. [#5666](https://github.com/AnalyticalGraphicsInc/cesium/issues/5666)


Do we want to deprecate this 6 months out? We normally do 1-3 releases. I think 3 releases would be appropriate for this case unless someone said otherwise.

Well, who decides?

@kaktus40 usually the developer and whoever reviews decides.

I agree with @hpinkos that 3 releases would be fine. Please submit a GitHub issue for the deprecation.

For more on deprecation, see https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#deprecation-and-breaking-changes

pjcozzi · 2017-09-08T13:05:35Z

Source/Core/Quaternion.js

@@ -179,6 +181,7 @@ define([
     * Computes a rotation from the given heading, pitch and roll angles. Heading is the rotation about the
     * negative z axis. Pitch is the rotation about the negative y axis. Roll is the rotation about
     * the positive x axis.
+     * @deprecated since V1.38


Does the "since V1.38" show up in the doc? We usually just put @deprecated.

I tested and it shows up in the doc. I found this syntax here

pjcozzi · 2017-09-08T13:06:34Z

Specs/Core/HeadingPitchRollSpec.js

+    it('conversion from direct quaternion', function() {
+        var testingTab = [
+            [0, 0, 0],
+            [90 * deg2rad, 0, 0],


Maybe this file was already like this, but why not use the conversion functions in Cesium.Math?

In the old PR, I used this variable, which is also a shortcut to CesiumMath.RADIANS_PER_DEGREE, for its concision.

pjcozzi · 2017-09-08T13:07:48Z

It's a shame that we have to deprecate the most natural name for these functions, what is the meaning of "direct?" Is that the best possible name? Are there any other options that would preserve the current name, e.g., with the addition of a parameter?

kaktus40 · 2017-09-08T13:37:03Z

Well for the name of direct, it's a hint about direct orthonormed repere. I didn't find an elegant solution to maintain the current name because after deprecation, the additional parameter will be removed. For the user who decide to update his code, he'll need to change a second time.
That was my initial idea to add a parameter also if you prefer this solution, I can make it.

pjcozzi · 2017-09-08T14:27:40Z

direct orthonormed repere

Sorry I'm dense here, but what does this mean? "direct orthonormal representation?"

Can you point me to a use of this term with "direct" in some literature?

kaktus40 · 2017-09-08T15:01:01Z

Well you are right, that's a bad translation... So would you like that I rename the functions with a prefix like trigonometricWise or any suggestion? Or should I use the same functions with a new parameter (a boolean). With the first solution the user will just need to rename the static functions when the change will be needed. The second solutions seems more difficult to update.

…rnion

kaktus40 · 2017-09-10T03:17:35Z

I change the deprecated version from 1.43 to 1.40

ggetz · 2017-09-11T13:27:27Z

Thanks @kaktus40. I think adding and optional boolean parameter might keep the naming more clear.

Could you also make your updates to CHANGES.md shorter with maybe one or two bullet points describing the changes?

kaktus40 · 2017-09-11T13:59:38Z

I'll need some suggestions to sum up my writings in the CHANGES.md.

ggetz · 2017-09-11T14:09:15Z

If we're going with passing the parameter, I think this will be enough:

The default behavior of the functions HeadingPitchRoll.fromQuaternion, Matrix3.fromHeadingPitchRoll, Quaternion.fromHeadingPitchRoll, Transforms.headingPitchRollToFixedFrame, and Transforms.headingPitchRollQuaternion has been deprecated. An optional boolean flag can be supplied to these functions that, if true, uses the classical orientation of heading and pitch calculated counter-clockwise. The flag will be removed and the new behavior made default in 1.40.
#5666

kaktus40 · 2017-09-11T14:16:12Z

That's good for me. So @pjcozzi is it ok to add a boolean parameter or an other solution?

pjcozzi · 2017-09-11T21:44:28Z

@kaktus40 thanks for the updates. I'll take a quick look this week.

kaktus40 · 2017-09-12T15:23:34Z

I push the update where I add a boolean parameter into original functions.

pjcozzi · 2017-09-15T21:39:47Z

@kaktus40 thanks again for the updates!

Given all the undefined arguments this can require adding, and then a user's code needs to be changed again later, I suggest we break the standard deprecation policy, and just make the breaking change right away. To lessen the impact, we could also put a deprecationWarning in the changed functions for one or two releases that explains the breaking change and how to update.

Let's sit on this idea for a few days to see if anyone has any objections.

kaktus40 · 2017-09-22T12:08:19Z

Hello, so what should I do?

ggetz · 2017-09-22T14:39:45Z

@kaktus40 I think @pjcozzi 's plan here to remove the if(classical === true) blocks entirely and just break the functions now, including the developer warnings of course, so people can update to the new correct functions immediately.

Thanks for your patience, these changes will have a pretty big impact, so we want to make sure we taking the best path forward!

pjcozzi · 2017-09-22T21:11:10Z

Given no objections to #5809 (comment), I think it is OK to make this change now.

@kaktus40 do you want to do it or do you want one of the maintainers to do it?

kaktus40 · 2017-09-22T21:40:49Z

Sorry I can't work on it the next week. Nom objection if a maintainer works on it.

pjcozzi · 2017-09-26T15:30:23Z

@ggetz do you have time to do this before the next release?

ggetz · 2017-09-27T15:35:25Z

@pjcozzi Updated in #5848

pjcozzi · 2017-09-28T22:47:11Z

Thanks @kaktus40 @ggetz! Will close this and merge #5848.

kaktus40 added 5 commits March 4, 2017 08:40

make heading pitch roll counter clock wise again

b12bf9d

Merge remote-tracking branch 'upstream/master' into CorrectHPRtoQuate…

fced5d3

…rnion

Merge remote-tracking branch 'upstream/master' into CorrectHPRtoQuate…

66e6da1

…rnion

Merge remote-tracking branch 'upstream/master' into CorrectHPRtoQuate…

a02d32c

…rnion

Added warning and create separate functions

a8234af

Correct of unused variable

9135155

ggetz requested changes Sep 6, 2017

View reviewed changes

remove warnings from new functions

f1c1337

kaktus40 added 3 commits September 7, 2017 22:09

Merge remote-tracking branch 'upstream/master' into CorrectHPRtoQuate…

0223afc

…rnion

add deprecated tag

6281116

deprecated tag misplaced

02f4046

hpinkos reviewed Sep 7, 2017

View reviewed changes

pjcozzi reviewed Sep 8, 2017

View reviewed changes

pjcozzi mentioned this pull request Sep 8, 2017

Update deprecated HeadingPitchRoll related functions #5822

Closed

kaktus40 added 2 commits September 10, 2017 04:51

Merge remote-tracking branch 'upstream/master' into CorrectHPRtoQuate…

7468635

…rnion

Change deprecated version to 1.40

db16f8f

updates to add a boolean parameter

fc9cb93

Merge branch 'master' into CorrectHPRtoQuaternion

16cd536

ggetz mentioned this pull request Sep 27, 2017

Calculate HPR counter clock wise #5848

Merged

pjcozzi closed this Sep 28, 2017

kaktus40 deleted the CorrectHPRtoQuaternion branch October 2, 2017 12:36

ggetz mentioned this pull request Oct 2, 2017

Sandcastle examples have deprecation warnings #5868

Closed

bagnell mentioned this pull request Oct 2, 2017

Counter-clockwise heading, pitch and roll #5873

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make Heading pitch roll counter clock wise again 2 #5809

make Heading pitch roll counter clock wise again 2 #5809

kaktus40 commented Sep 3, 2017

cesium-concierge commented Sep 3, 2017

ggetz left a comment

ggetz Sep 6, 2017

kaktus40 Sep 6, 2017

ggetz Sep 6, 2017

kaktus40 Sep 6, 2017

ggetz commented Sep 7, 2017 •

edited

Loading

hpinkos Sep 7, 2017 •

edited

Loading

kaktus40 Sep 8, 2017

pjcozzi Sep 8, 2017

pjcozzi Sep 8, 2017

kaktus40 Sep 10, 2017

pjcozzi Sep 8, 2017

kaktus40 Sep 10, 2017

pjcozzi commented Sep 8, 2017

kaktus40 commented Sep 8, 2017

pjcozzi commented Sep 8, 2017

kaktus40 commented Sep 8, 2017 via email

kaktus40 commented Sep 10, 2017 via email

ggetz commented Sep 11, 2017

kaktus40 commented Sep 11, 2017

ggetz commented Sep 11, 2017

kaktus40 commented Sep 11, 2017

pjcozzi commented Sep 11, 2017

kaktus40 commented Sep 12, 2017

pjcozzi commented Sep 15, 2017

kaktus40 commented Sep 22, 2017

ggetz commented Sep 22, 2017

pjcozzi commented Sep 22, 2017

kaktus40 commented Sep 22, 2017 via email

pjcozzi commented Sep 26, 2017

ggetz commented Sep 27, 2017

pjcozzi commented Sep 28, 2017

make Heading pitch roll counter clock wise again 2 #5809

make Heading pitch roll counter clock wise again 2 #5809

Conversation

kaktus40 commented Sep 3, 2017

cesium-concierge commented Sep 3, 2017

ggetz 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

ggetz commented Sep 7, 2017 • edited Loading

hpinkos Sep 7, 2017 • edited Loading

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 Sep 8, 2017

kaktus40 commented Sep 8, 2017

pjcozzi commented Sep 8, 2017

kaktus40 commented Sep 8, 2017 via email

kaktus40 commented Sep 10, 2017 via email

ggetz commented Sep 11, 2017

kaktus40 commented Sep 11, 2017

ggetz commented Sep 11, 2017

kaktus40 commented Sep 11, 2017

pjcozzi commented Sep 11, 2017

kaktus40 commented Sep 12, 2017

pjcozzi commented Sep 15, 2017

kaktus40 commented Sep 22, 2017

ggetz commented Sep 22, 2017

pjcozzi commented Sep 22, 2017

kaktus40 commented Sep 22, 2017 via email

pjcozzi commented Sep 26, 2017

ggetz commented Sep 27, 2017

pjcozzi commented Sep 28, 2017

ggetz commented Sep 7, 2017 •

edited

Loading

hpinkos Sep 7, 2017 •

edited

Loading