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.

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

Heading-Pitch-Roll #2381

Merged
merged 50 commits into from
Jan 14, 2015
Merged

Heading-Pitch-Roll #2381

merged 50 commits into from
Jan 14, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jan 7, 2015

  • Added Quaternion.fromHeadingPitchRoll to create a rotation from heading, pitch, and roll angles.
  • Added Transforms.headingPitchRollToFixedFrame to create a local frame from a position and heading/pitch/roll angles.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 7, 2015

@t100n would this work for you for youbeQ?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 7, 2015

I'm not sure that the typical user is going to find these functions and know what to do with them. Can we:

  • Update some Sandcastle examples? At the least, we should add "Point camera using heading, pitch, and roll" and perhaps a model example or another primitive and set its model matrix.
  • Add this to a prominent part of the Camera tutorial. I would put this (and, as time allows, other simple camera tasks) at the very top before "Let’s create our own event handlers..." How many people need to create their own handlers vs. do basic camera control?
  • Update Section 2 of @t100n's youbeQ blog post to say Starting with Cesium 1.6, this can be done with Quaternion.fromHeadingPitchRoll like [code example] - Dan.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 7, 2015

Code and tests are good, otherwise.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 7, 2015

@bagnell while your in the camera tutorial, does CameraController exist anymore?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 7, 2015

Also, I know this is outside the scope of this pull request, but could you also add an example like this to the camera Sandcastle example:

var ray = viewer.camera.getPickRay(movement.endPosition);
var position = viewer.scene.globe.pick(ray, viewer.scene);
if (Cesium.defined(position)) {
    // convert position to cartographic and display altitude
}

Conflicts:
	CHANGES.md
Conflicts:
	CHANGES.md
@mramato
Copy link
Contributor

mramato commented Jan 8, 2015

So I did a local merge of this into implicit-properties and it ended up being a little more work than I expected to create the below graphic:

image

The code

var viewer = new Cesium.Viewer('cesiumContainer');
for (var i = 0; i <= 90; i += 15) {
    var position = Cesium.Cartesian3.fromDegrees(-104.0, 45.0, 200000.0 * (i / 15) + 1);
    var transform = Cesium.Transforms.headingPitchRollToFixedFrame(position, 0, Cesium.Math.toRadians(i), 0);
    var rotation = Cesium.Matrix4.getRotation(transform, new Cesium.Matrix3());
    var orientation = Cesium.Quaternion.fromRotationMatrix(rotation);

    viewer.entities.add({
        position : position,
        orientation : orientation,
        label : {
            text: i + ' degrees',
            horizontalOrigin : Cesium.HorizontalOrigin.LEFT,
            eyeOffset: new Cesium.Cartesian3(90000.0, 0.0, -90000.0)
        },
        ellipsoid : {
            radii : new Cesium.Cartesian3(30000.0, 60000.0, 90000.0),
            outline : true
        }
    });
}

Just like Cartesian.fromDegrees returns you a Cartesian3 in earth-fixed; shouldn't Quaternion.fromHeadingPitchRoll do the same thing? I know this would require it take a position, but it would greatly simplify the typical use case above and I think it would stave off a lot of confusion.

    var position = Cesium.Cartesian3.fromDegrees(-104.0, 45.0, 200000.0 * (i / 15) + 1);
    var orientation = Cesium.Quaternion.fromHeadingPitchRoll(position, 0, Cesium.Math.toRadians(i), 0);

If we wanted to keep Cesium.Quaternion.fromHeadingPitchRoll we could also add a Cesium.Quaternion.fromHeadingPitchRollFixed instead, but I think the latter would be the more widely used version.

It's possible I'm missing something. What do you guys this?

@bagnell
Copy link
Contributor Author

bagnell commented Jan 13, 2015

@pjcozzi This is ready. I'll update the blogs posts and send a message to the forum.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 13, 2015

When we merge this, can you please start a new forum thread with a code example that says this will be in Cesium 1.6 on February 2?

Also add a link to that thread from this older one: https://groups.google.com/forum/#!topic/cesium-dev/7MOyzJ9_6so

* centered at the provided origin to the provided ellipsoid's fixed reference frame.
*
* @param {Cartesian3} origin The center point of the local reference frame.
* @param {Number} heading The heading angle in radians.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and throughout the doc in this PR, we need to describe what heading, pitch, and roll are, e.g., relative to north, geodetic normal, etc. If you want to go all out, draw a picture.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 13, 2015

Have you tested all the corner cases in 2D, Columbus view, and the morph?

For example, in the new camera Sandcastle example, I switched to 2D, then did the heading/pitch/roll zoom, which put me in the wrong place. Then I tried to morph to CV, and:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 13, 2015

Also, for the forum updates, chime in here too: https://groups.google.com/forum/#!topic/cesium-dev/YY0ly3rne_w

And anywhere else you find.

@bagnell
Copy link
Contributor Author

bagnell commented Jan 14, 2015

@pjcozzi The crash when switching from 2D to CV is in master too. Go the Viewer on the website, zoom in to Exton, middle mouse click to rotate the view until the heading is about west, switch to CV.

I'm still looking into the crash, but I think this is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 14, 2015

Looks good. Submitting a separate issue for the exception. Even though it is in master, it is now prominent so we should fix it for 1.6.

pjcozzi added a commit that referenced this pull request Jan 14, 2015
@pjcozzi pjcozzi merged commit 247f746 into master Jan 14, 2015
@pjcozzi pjcozzi deleted the hpr branch January 14, 2015 21:41
@pjcozzi pjcozzi mentioned this pull request Jan 14, 2015
@mramato mramato mentioned this pull request Jan 28, 2015
70 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants