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

Improve Camera Flights #2825

Merged
merged 38 commits into from
Jun 30, 2015
Merged

Improve Camera Flights #2825

merged 38 commits into from
Jun 30, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jun 22, 2015

TODO

  • Fix/update tests
  • Update CHANGES.md
  • Deprecate unused splines and submit issues for them
  • Update Sandcastle example for common use cases, e.g., global view, city view, etc.

CC #1060

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

I updated the TODO list in this issue:

Deprecate unused splines and submit issues for them

@@ -2299,6 +2315,8 @@ define([
* @param {Matrix4} [options.endTransform] Transform matrix representing the reference frame the camera will be in when the flight is completed.
* @param {Boolean} [options.convert=true] When <code>true</code>, the destination is converted to the correct coordinate system for each scene mode. When <code>false</code>, the destination is expected
* to be in the correct coordinate system.
* @param {Number} [options.altitude] The maximum altitude at the peak of the flight.
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout Cesium, we call this height. We should do the same here unless we have a compelling reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maximumHeight.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

Also added this to the TODO:

Update Sandcastle example for common use cases, e.g., global view, city view, etc.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

The flight from, for example, Exton to LA feels a bit too "square" if you know what I mean.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

The flight from, for example, Exton to LA feels a bit too "square" if you know what I mean.

When it is a bit slower, I think this is fine.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

The Cesium API and Geocoder/Home flights seem to have different durations. Why not have the same default for both?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

To freeze when morphing:

I'm pretty sure this is because the morph started before the flight finished.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2015

The CV flights tend to dip early then fly low to the ground. Instead, if we dip late then zoom closer to directly in, I bet it will look better and load fewer tiles since it is not requesting all the high res tiles when moving quickly.

For example:

@mramato
Copy link
Contributor

mramato commented Jun 25, 2015

The home flight is now WAY to long. Can we change it so the unified defaults use the old home flight value.

After playing with it some more, the default flight time in general is way to long. I would recommend we make it no longer than 1 second. I think this also has to do with the fact that flight length (in meters traveled) seems to be significantly less without the bouncing.

@mramato
Copy link
Contributor

mramato commented Jun 25, 2015

Change the last line of the below example to viewer.flyTo(viewer.entities); In this branch the end view is incorrect.

1.10: http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Polygon.html&label=Geometries
branch: http://localhost:8080/Apps/Sandcastle/index.html?src=Polygon.html&label=Geometries

@mramato
Copy link
Contributor

mramato commented Jun 25, 2015

I think this fixes #2073 as well. @bagnell can you confirm.

var duration = options.duration;
if (!defined(duration)) {
duration = Math.ceil(Cartesian3.distance(camera.position, destination) / 1000000.0) + 3.0;
duration = Math.min(duration, 10.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to be the cause of the extremely slow fights that I'm seeing (10 seconds). I would suggest changing these two lines to the below, which I feel works much better in all of the test cases I've tried.

            duration = Math.ceil(Cartesian3.distance(camera.position, destination) / 1000000.0) + 1.0;
            duration = Math.min(duration, 2.0);

@mramato
Copy link
Contributor

mramato commented Jun 25, 2015

That's all I've got for now, this is an awesome change.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 26, 2015

@bagnell can you reply here: https://groups.google.com/forum/#!topic/cesium-dev/90utjnigscg

I imagine this is just use LINEAR?

* Deprecated
* `CatmullRomSpline` has been deprecated and will be removed in Cesium 1.12.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should rethink this: https://groups.google.com/forum/#!topic/cesium-dev/MMIdPKFNR2s

Deprecating HermiteSpline is still OK though.

Choose a reason for hiding this comment

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

Might want to keep both and add others. Depends on what direction is taken for tools in Viewer. Also, GML uses b-spline and cubic spline which I believe are for roads.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with not deprecating these and reevaluating in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GatorScott I added cubic b-splines and bezier splines a while back for model animation. They're sitting in the splines branch if we ever need them.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 28, 2015

@bagnell anything left here? Let's aim to merge this on Monday for 1.11.

@mramato
Copy link
Contributor

mramato commented Jun 29, 2015

#2468 still seems to be a problem.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 29, 2015

@pjcozzi I removed the deprecation from CatmullRomSpline and HermiteSpline. This is ready.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 29, 2015

@mramato #2468 is fixed again. I broke it when adding a fix for flying to the bounding sphere of the polygon Sandcastle demo.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 29, 2015

It seems that #2073 is fixed but I haven't been able to reproduce it in master either.

Conflicts:
	CHANGES.md
@mramato
Copy link
Contributor

mramato commented Jun 29, 2015

A big 👍 from me. @pjcozzi feel free to merge if you're happy.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 29, 2015

Awesome, will review and merge first thing tomorrow.

pjcozzi added a commit that referenced this pull request Jun 30, 2015
@pjcozzi pjcozzi merged commit a48142a into master Jun 30, 2015
@pjcozzi pjcozzi deleted the flights branch June 30, 2015 12:16
@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 30, 2015

@bagnell can you still reply to Gaurav on the forum?

@mramato mramato mentioned this pull request Sep 28, 2015
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.

None yet

5 participants