adding a method to retrieve pitch, roll and heading from a quaternion #4047

kaktus40 · 2016-06-25T16:54:14Z

No description provided.

pjcozzi · 2016-06-25T18:54:16Z

Source/Core/HeadingPitchRoll.js

+
+    return HeadingPitchRoll;
+});
+


Minor, but can you remove the extra blank lines?

pjcozzi · 2016-06-25T18:57:23Z

Can you add the following functions and unit tests for them?

HeadingPitchRoll.clone
HeadingPitchRoll.equals
HeadingPitchRoll.equalsEpsilon
HeadingPitchRoll.prototype.clone
HeadingPitchRoll.prototype.equals
HeadingPitchRoll.prototype.equalsEpsilon
HeadingPitchRoll.prototype.toString

For simple examples, see Cartesian2.

pjcozzi · 2016-06-25T19:00:06Z

Source/Core/HeadingPitchRoll.js

+    './defaultValue',
+    './defined',
+    './DeveloperError',
+    './freezeObject',


Remove freezeObject and defineProperties; they are not used.

pjcozzi · 2016-06-25T19:08:19Z

It would be cool to update the 3D Models Sandcastle example (here and here) to show how to rotate the aircraft model using heading, pitch, and roll.

pjcozzi · 2016-06-25T19:08:40Z

@bagnell can you take a quick look at the math?

pjcozzi · 2016-06-25T19:11:08Z

@kaktus40 thanks again for this pull request; I think a lot of users will benefit from this new class, especially when they want to rotate a model in an intuitive way.

Most of the comments were just minor style things; you can check out the Cesium Coding Guide for more details.

kaktus40 · 2016-06-25T23:33:53Z

For the sandCastle examples, I think I'll need this pull and my previous pull. For example in this line, I could use aircraftHeadingPitchRollQuaternion instead of headingPitchRollQuaternion.

bagnell · 2016-06-27T15:11:38Z

@pjcozzi Looks OK to me.

pjcozzi · 2016-06-28T13:08:27Z

For the sandCastle examples, I think I'll need this pull and my previous pull. For example in this line, I could use aircraftHeadingPitchRollQuaternion instead of headingPitchRollQuaternion.

I just merged #4048 so you should be able to merge in master and then make this example.

Also, I just realized that it would be better for the new Transforms functions to take a HeadingPitchRoll instance instead of separate heading, pitch, and roll parameters. Can you please make that change in this pull request?

pjcozzi · 2016-06-29T21:09:04Z

@kaktus40 sorry for the short notice, but the Cesium 1.23 release is on Friday. Do you think you could make this change in the next day? Otherwise, I have to mark the new Transform functions as private to avoid a later breaking change. Thanks!

Also, I just realized that it would be better for the new Transforms functions to take a HeadingPitchRoll instance instead of separate heading, pitch, and roll parameters. Can you please make that change in this pull request?

kaktus40 · 2016-06-30T05:15:20Z

Sorry I can't work on it until next week. In paralells, I search a little use case if you have any idea?

Le 29/06/2016 23:09, Patrick Cozzi a écrit :

@kaktus40 https://github.com/kaktus40 sorry for the short notice, but the Cesium 1.23 release is on Friday. Do you think you could make this change in the next day? Otherwise, I have to mark the new Transform functions as private to avoid a later breaking change. Thanks!

Also, I just realized that it would be better for the new Transforms functions to take a HeadingPitchRoll instance instead of separate heading, pitch, and roll parameters. Can you please make that change in this pull request?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/4047#issuecomment-229489523, or mute the threadhttps://github.com/notifications/unsubscribe/AF0OfB5QdLA7vGD6Y8QDcPv47MwnE2MMks5qQt75gaJpZM4I-Yld.

…itchRoll

kaktus40 · 2016-07-09T14:14:27Z

I added a sandCastle example in development gallery. I need a little help for this example. It uses an keypressed event listener on html document but when the example is launched, it doesn't have the focus also the event listener is useless. In order to work, you have to get the focus on the cesium container (fullscreen, focus on a checkbox used in the example or make a focus on the geocoder). Any ideas?

kaktus40 · 2016-07-09T14:29:43Z

That's strange, with jasmine in my navigator, I didn't have errors.

kaktus40 · 2016-07-09T14:33:16Z

I have some error with makeZipFile command

[14:11:49] Error: Command failed: /bin/sh -c npm run requirejs -- --eyJ3cmFwIjp0cnVlLCJ1c2VTdHJpY3QiOnRydWUsIm9wdGltaXplIjoibm9uZSIsIm9wdGltaXplQ3NzIjoic3RhbmRhcmQiLCJwcmFnbWFzIjp7ImRlYnVnIjp0cnVlfSwiYmFzZVVybCI6IlNvdXJjZSIsInNraXBNb2R1bGVJbnNlcnRpb24iOnRydWUsIm5hbWUiOiIuLi9ub2RlX21vZHVsZXMvYWxtb25kL2FsbW9uZCIsImluY2x1ZGUiOiJtYWluIiwib3V0IjoiQnVpbGQvY29tYmluZU91dHB1dC9ub25lL0Nlc2l1bS5qcyJ9 --silent

pjcozzi · 2016-07-09T18:26:38Z

Source/Core/HeadingPitchRoll.js

+     * @param {Number} [pitch=0.0] The pitch component in radians.
+     * @param {Number} [roll=0.0] The roll component in radians.
+     */
+    var HeadingPitchRoll = function(heading, pitch, roll) {


Use the more concise function HeadingPitchRoll syntax here. We use to use var to workaround a Jasmine bug that has been fixed.

…itchRoll

kaktus40 · 2016-08-23T03:13:10Z

Hello, sorry for my heaviness but is there any news about a new review of this pull?

hpinkos · 2016-08-23T13:26:20Z

Thank you for your patience @kaktus40. We want to get better at reviewing pull requests in a more timely manner.

@pjcozzi @bagnell what's the status here? This is functionality that is requested with some frequency on the forum so it would be nice to get merged in.

@kaktus40 in the meantime, please merge in master when you get a chance. thanks!

kaktus40 · 2016-08-24T18:48:36Z

Hello @hpinkos , what do you mean by merging in master? Do you mean in my repository?

hpinkos · 2016-08-24T19:17:08Z

Yes, you need to sync the master branch in your fork with our master branch. Then merge your master into your branch. Right now there are conflicts so we can't merge your branch until you're synced up.

Here are some instructions for syncing a fork: https://help.github.com/articles/syncing-a-fork/

Thanks!

…itchRoll

kaktus40 · 2016-08-24T20:40:31Z

Ok it's done. Conflicts only in the CHANGES.md.

hpinkos · 2016-09-13T19:19:43Z

@kaktus40 sorry we haven't gotten back to you yet, we have some people out on vacation and we've been pretty busy around here lately. We have a bug bash coming up in October, and I've labeled this PR for the bug bash so we will definitely be able to review it then, if not sooner. Thanks!

kaktus40 · 2016-09-13T20:10:17Z

Ok no problem.
I'm working on a PR for calculating polygon area. I worked with algorithm edited here
Have you any suggestion for this PR? For example is it a good place for the function (into polygonGraphics)?

hpinkos · 2016-09-13T20:29:09Z

Something like that would be a stand-alone function. I don't think area is something we would want tied into the entities layer.
I'm honestly not sure if polygon area is something we would merge into the main code base. Cesium is primarily a rendering engine so we might not want to add analysis tools in the core Cesium library.
However, I'm sure people would appreciate it if you wanted to share it on the forum. It's definitely a frequently asked question

kaktus40 · 2016-09-13T20:42:16Z

Ok I'll cancel this pull.

bagnell · 2016-10-19T14:52:08Z

This looks good to me. The development example doesn't work in Chrome for me, but it does work in Firefox. It looks like mostly whitespace issues and an update to CHANGES.md remain.

@pjcozzi Do you want to do another review?

pjcozzi · 2016-10-19T14:58:49Z

@kaktus40 I'm going to finish the review for this now.

For polygon area, do you need to roll a new algorithm or could you use http://turfjs.org/?

pjcozzi · 2016-10-19T15:54:29Z

@kaktus40 the tests pass and the Sandcastle example looks great, but the controls did not work for me in Chrome or Firefox (running Mac) even after setting focus to the canvas or running fullscreen. Do they work for you?

Also, would you be able to update CHANGES.md (just move your changes to the 1.27 section) and merge in master?

…itchRoll

kaktus40 · 2016-10-19T21:13:51Z

I didn't use turf for calculating the area of a polygon. I used the algorithms defined here. I wrote the most precise and algorithm and the approximate algorithm.
I test the example on firefox, it's ok. On chromium, it's KO. I don't know how to capture the focus in the window in order trigger the keyboard event.
Anyway. I updated the CHANGES.md.

pjcozzi · 2016-10-19T21:17:27Z

I test the example on firefox, it's ok. On chromium, it's KO. I don't know how to capture the focus in the window in order trigger the keyboard event.

Sorry, I'm not following. Do the keyboard options work for you? If not, perhaps you could replace them with buttons like other Sandcastle examples or look at the Camera Tutorial example that uses key input.

Anyway. I updated the CHANGES.md.

Thanks. Can you please also merge master from this repo into your branch so the final merge is smooth?

…itchRoll

kaktus40 · 2016-10-20T04:33:35Z

I changed the key event in the example and it's now ok for me in chromium. I just merge changes.md

pjcozzi · 2016-10-20T14:26:07Z

Thanks again @kaktus40 for this nice contribution and working with us on the review.

I'm going to do a few tweaks/fixes to the Sandcastle example and will open a separate PR in a few minutes.

pjcozzi · 2016-10-20T14:26:44Z

@hpinkos can you please update any forum threads if folks were asking for this?

adding a method to retrieve pitch, roll and heading from a quaternion

22d3875

pjcozzi reviewed Jun 25, 2016
View reviewed changes

Source/Core/HeadingPitchRoll.js

return HeadingPitchRoll;

});

Copy link

Contributor

pjcozzi Jun 25, 2016

Choose a reason for hiding this comment

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

Minor, but can you remove the extra blank lines?

pjcozzi reviewed Jun 25, 2016
View reviewed changes

update files following recommendations.

f1684c3

pjcozzi mentioned this pull request Jun 30, 2016

Mark new Transforms functions as private #4070

Merged

kaktus40 added 4 commits July 1, 2016 06:43

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

7d9c475

…itchRoll

Merge branch 'master' into adding_toHeadingPitchRoll

00a32b0

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

2c7be6c

…itchRoll

added a new transformation and a sandCastle example

4aa7eee

pjcozzi reviewed Jul 9, 2016
View reviewed changes

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

dba1f75

…itchRoll

kaktus40 mentioned this pull request Aug 5, 2016

Quaternion.fromHeadingPitchRoll uses wrong axis? #3256

Closed

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

e2fa9f0

…itchRoll

hpinkos added the bug bash label Sep 12, 2016

kaktus40 added 2 commits October 19, 2016 22:37

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

74114c1

…itchRoll

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

26854a1

…itchRoll

kaktus40 added 2 commits October 20, 2016 06:15

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

39bab2c

…itchRoll

update example

edcc1d8

pjcozzi merged commit c0dc341 into CesiumGS:master Oct 20, 2016

This was referenced Oct 20, 2016

Change Transforms.headingPitchRollToFixedFrame to use new HeadingPitchRoll #4468

Closed

Tweak heading pitch roll development Sandcastle example #4471

Merged

kaktus40 deleted the adding_toHeadingPitchRoll branch October 20, 2016 19:10

twpayne mentioned this pull request Oct 21, 2016

Tidy up HeadingPitchRoll API #4498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding a method to retrieve pitch, roll and heading from a quaternion #4047

adding a method to retrieve pitch, roll and heading from a quaternion #4047

kaktus40 commented Jun 25, 2016

pjcozzi Jun 25, 2016

pjcozzi commented Jun 25, 2016

pjcozzi Jun 25, 2016

pjcozzi commented Jun 25, 2016

pjcozzi commented Jun 25, 2016

pjcozzi commented Jun 25, 2016

kaktus40 commented Jun 25, 2016

bagnell commented Jun 27, 2016

pjcozzi commented Jun 28, 2016

pjcozzi commented Jun 29, 2016

kaktus40 commented Jun 30, 2016

kaktus40 commented Jul 9, 2016

kaktus40 commented Jul 9, 2016

kaktus40 commented Jul 9, 2016

pjcozzi Jul 9, 2016

kaktus40 commented Aug 23, 2016

hpinkos commented Aug 23, 2016

kaktus40 commented Aug 24, 2016

hpinkos commented Aug 24, 2016

kaktus40 commented Aug 24, 2016

hpinkos commented Sep 13, 2016

kaktus40 commented Sep 13, 2016

hpinkos commented Sep 13, 2016

kaktus40 commented Sep 13, 2016

bagnell commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

kaktus40 commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

kaktus40 commented Oct 20, 2016

pjcozzi commented Oct 20, 2016

pjcozzi commented Oct 20, 2016

adding a method to retrieve pitch, roll and heading from a quaternion #4047

adding a method to retrieve pitch, roll and heading from a quaternion #4047

Conversation

kaktus40 commented Jun 25, 2016

pjcozzi Jun 25, 2016

Choose a reason for hiding this comment

pjcozzi commented Jun 25, 2016

pjcozzi Jun 25, 2016

Choose a reason for hiding this comment

pjcozzi commented Jun 25, 2016

pjcozzi commented Jun 25, 2016

pjcozzi commented Jun 25, 2016

kaktus40 commented Jun 25, 2016

bagnell commented Jun 27, 2016

pjcozzi commented Jun 28, 2016

pjcozzi commented Jun 29, 2016

kaktus40 commented Jun 30, 2016

kaktus40 commented Jul 9, 2016

kaktus40 commented Jul 9, 2016

kaktus40 commented Jul 9, 2016

pjcozzi Jul 9, 2016

Choose a reason for hiding this comment

kaktus40 commented Aug 23, 2016

hpinkos commented Aug 23, 2016

kaktus40 commented Aug 24, 2016

hpinkos commented Aug 24, 2016

kaktus40 commented Aug 24, 2016

hpinkos commented Sep 13, 2016

kaktus40 commented Sep 13, 2016

hpinkos commented Sep 13, 2016

kaktus40 commented Sep 13, 2016

bagnell commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

kaktus40 commented Oct 19, 2016

pjcozzi commented Oct 19, 2016

kaktus40 commented Oct 20, 2016

pjcozzi commented Oct 20, 2016

pjcozzi commented Oct 20, 2016