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

Use better example in Transforms doc #7041

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Use better example in Transforms doc #7041

merged 3 commits into from
Sep 19, 2018

Conversation

OmarShehata
Copy link
Contributor

The current example for Transforms.computeIcrfToFixedMatrix is the only one on the page that doesn't have a comment describing what it does.

It seems like it prevents the camera from tilting. I think we should at least describe that if that's the intended use case. Although since you can just disable tilting with a flag on the camera, I propose we use this simpler example instead:

scene.postUpdate.addEventListener(function(scene, time) {
  // By default, the camera automatically rotates with the globe.
  // This example fixes the camera to its current position.
  var icrfToFixed = Cesium.Transforms.computeIcrfToFixedMatrix(time);
  if (Cesium.defined(icrfToFixed)) {
    var offset = Cesium.Cartesian3.clone(camera.position);
    var transform = Cesium.Matrix4.fromRotationTranslation(icrfToFixed);
    camera.lookAtTransform(transform, offset);
  }
});

I don't think there's another way to do that so it's nice to have that example in the doc.

I also wanted to use viewer.scene instead of scene so that it works if you copy paste it without having to define scene and camera but it would make it feel inconsistent unless we change all examples to do that.

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Sep 14, 2018

@OmarShehata ICRF (Inertical Coordinate Reference Frame) is a standard reference frame used in many space based applications. It basically means "Make the camera behave as if it were stationary and the earth (or any origin point) were rotating under it instead of being locked into an earth based position" The analog to Inertial is the Fixed frame, which means the camera is locked into an earth based position (or any origin point) as the earth moves, the camera moves with it.

I think the reason there is no comment is because "view in ICRF" is self explanatory to anyone interested in an inertial view. If you want, you can always add a link to some explanation of ICRF though I'm not sure Wikipedia is the best link to go to.

As for why tilt doesn't work in the example, that sounds like a bug in the example, not the intended behavior.

@OmarShehata
Copy link
Contributor Author

Thanks for the context and explanation @mramato ! Definitely wasn't aware of that.

In that case, I think the code example doesn't do what it's intended to. Here's a Sandcastle where you can see the camera is still locked into an earth based view.

Here's my example which does make the camera behave as if it is stationary. Although neither allows the camera to tilt as far as I can tell.

@mramato
Copy link
Contributor

mramato commented Sep 17, 2018

Your example matches what's in the Camera Sandcastle and it definitely the correct one to use.

Both have the tilt issue because they are calling lookAtTransform with the result of fromRotationTranslation without taking into account the current camera rotation. You would need to either copy the relevant camera settings into the transform before applying it, or get the relevant properties from the camera before applying the transform and then set them back after the fact. You're welcome to make this change but if it takes more than a few minutes I wouldn't worry about it unless someone was recently asking about it on the forum.

@OmarShehata
Copy link
Contributor Author

Couldn't get the complete transform applied correctly, but this should at least be a more correct example than before.

I updated the comments in the example to just say "View in ICRF". I think it's still worth adding a comment since the doc on that function say it transforms a point or a vector, and so this example is showing you how to use that to set the view, (as opposed to just transforming a point).

@OmarShehata
Copy link
Contributor Author

@mramato do you think this looks good to merge?

@mramato mramato merged commit d9bb9c2 into CesiumGS:master Sep 19, 2018
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.

3 participants