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

Add ability complete flight #8788

Merged
merged 6 commits into from
May 31, 2020
Merged

Add ability complete flight #8788

merged 6 commits into from
May 31, 2020

Conversation

SambaLim
Copy link
Contributor

For #8441

I add ability completeFlight
It immediately finish a flight in progress and just jump to the end.

The following Sandcastle Code can run completeFlight

var viewer = new Cesium.Viewer("cesiumContainer");
var destination = Cesium.Cartesian3.fromDegrees(126.990525, 37.493530, 3000);

viewer.camera.flyTo({
  destination: destination,
    orientation: {
    heading: Cesium.Math.toRadians(0),
    pitch: Cesium.Math.toRadians(-60),
    roll: Cesium.Math.toRadians(0),
  },
  duration: 5,
});

setTimeout(function() {
  viewer.camera.completeFlight();
}, 3000);

@cesium-concierge
Copy link

Thanks for the pull request @SambaLim!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@mramato
Copy link
Contributor

mramato commented Apr 23, 2020

Thanks @SambaLim there is definitely room for improvement in the camera API for stuff like this. I took a quick look at your change and I think the main feedback I have is your code will trigger the "flight cancelled" callback and not the "flight completed" callback (both are passed as parameters to flyTo).

Long term, I would love to make async camera operations return an object with options like this instead of globbing everything onto the Camera itself, but that can be a bigger refactoring for a future PR.

@SambaLim
Copy link
Contributor Author

I think positively about the same change as I mentioned earlier.
Thank you for your comment.
And I fix completeFlight code to callback "flight completed".

The following Sandcastle Code can run fixed completeFlight

var viewer = new Cesium.Viewer("cesiumContainer");
var destination = Cesium.Cartesian3.fromDegrees(126.990525, 37.493530, 3000);

function testComplete() {
  console.log('It Complete!');
}

function testCancel() {
  console.log('It Cancel!');
}

viewer.camera.flyTo({
  destination: destination,
    orientation: {
    heading: Cesium.Math.toRadians(0),
    pitch: Cesium.Math.toRadians(-60),
    roll: Cesium.Math.toRadians(0),
  },
  duration: 5,
  complete: testComplete,
  cancel: testCancel
});

setTimeout(function() {
  viewer.camera.completeFlight();
}, 3000);

*/
Camera.prototype.completeFlight = function () {
if (defined(this._currentFlight)) {
this.cancelFlight();
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still an issue here where calling cancelFlight will cause the current flights cancelled callback to be called. Instead of calling cancelFlight, just add this._currentFlight = undefined; at the end of this if block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was wrong, what I meant was call this._currentFlight.cancelTween(); directly here.

if (defined(this._currentFlight)) {
this.cancelFlight();

newOptions.duration = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set the duration here? It doesn't look like setView takes a duration so wouldn't it be ignored either way?

newOptions.duration = 0;
this.setView(newOptions);

if (defined(newOptions.complete)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be safe, I would use this._currentFlight.complete() instead. Then also set this._currentFlight to undefined, since the flight is done.

@mramato
Copy link
Contributor

mramato commented Apr 28, 2020

Sorry for not getting back to you sooner @SambaLim, I just had to carve out some time to look more into this. I think those are the only comments I had. Thanks again for the pull request!

@SambaLim
Copy link
Contributor Author

Thanks for your delicate reply.
I fix my code with your reply.
It is safer to use _currentFlight rather than newOption

@mramato
Copy link
Contributor

mramato commented May 1, 2020

Thanks @SambaLim, I tested this out and something isn't quite right.

At the end of http://localhost:8080/Apps/Sandcastle/index.html?src=Camera.html, add

Sandcastle.addToolbarButton('Complete Flight', ()=>{
  scene.camera.completeFlight();
});

Then run the example and select Fly to Location with heading, pitch and roll. While the flight is happening, hit the Complete Flight button. The flight finishes, but with a different view than if you just let the flight complete on its own. Looks like we are losing something about the flight.

@SambaLim
Copy link
Contributor Author

SambaLim commented May 1, 2020

Thanks to reply @mramato . I appreciate your kindness.

I fix completeFlight loose heading, pitch, roll.
Because newOptions doesn't have an orientation property.
So I create options and use it.
If you run example and hit Complete Flight, It finish well.

@hpinkos
Copy link
Contributor

hpinkos commented May 29, 2020

bump @mramato

@SambaLim SambaLim requested a review from mramato May 30, 2020 02:06
@mramato
Copy link
Contributor

mramato commented May 31, 2020

Thanks @SambaLim, sorry for not getting back to you sooner. I added a test and tweaked the doc. I'll merge as soon as CI passes and this will go out with tomorrow's release.

@mramato mramato merged commit 929385d into CesiumGS:master May 31, 2020
@SambaLim
Copy link
Contributor Author

SambaLim commented Jun 1, 2020

Thanks @mramato !
I am happy to contribute to Cesium.js. Thank you for coming.

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