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

Fly to data on load #2601

Merged
merged 6 commits into from
May 29, 2015
Merged

Fly to data on load #2601

merged 6 commits into from
May 29, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Mar 25, 2015

I don't want this merged any time soon, but please play with this and provide feedback

This is a simple 2 file change that builds on #2600 (once that is merged, this PR will be tiny). It does 2 things.

  1. Added viewerDragDropMixin.flyToOnDrop (on by default) which causes viewer to fly to a data source when it is dropped onto the globe, similar to Google Earth.
  2. The Cesium Viewer reference app flies to data that is specified via url (unless a lookAt parameter is provided).

This functionality actually works really well, but has a few minor drawbacks:

  1. There's a noticeable pause between the drop and the flight if we're waiting for geometry to be processed or models to be loaded. This can probably be easily addressed with a simple loading indicator in the corner (which we need to add in the long run anyway). It doesn't have to hold up this PR though.
  2. Dropping time-dynamic data that contains a single object or small group of objects moving together doesn't work well if you're animating because the thing you fly to is already out of the frame when you get there. I'm not sure there's a good way to fix this but it only happens for this edge case.
  3. It's pretty obvious our flights need improvement and you can end up with some non-typical views when you load something with global data. We may not care for now, but introducing this feature puts the flights more in your face.

Also made Cesium Viewer fly to a data source if loaded via url without a lookAt parameter.
@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 23, 2015

Looks good.

I'm sure it's not this code, but dragging and dropping facilities.kml produces an unexpected default view.

I think this could be merged even before fixing flights.

@mramato
Copy link
Contributor Author

mramato commented May 28, 2015

@emackey, have you had a chance to check this out? If you think it's cool too we should merge it for 1.10, otherwise we can hold off until flights are better.

return flyToOnDrop;
},
set : function(value) {
flyToOnDrop = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we do this, as opposed to having a simple property, or knockout? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like proxy and clearOnDrop are simple get/set as well, so I was most likely just following suit. I'm not sure why they are though, so I'll change them and see what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I had a suspicion the reason was jsDoc and I was right. For some reason assigning the value directly to viewer and documenting it causes it not to show up in the doc. So we'll just leave it the way it is for now.

@emackey
Copy link
Contributor

emackey commented May 29, 2015

I like the behavior of this, and I think we should merge for 1.10. I'll second @pjcozzi's comment about facilities.kml producing wrong results, it would be better for it to end up looking at the whole Earth. Is that specific to this one file, or is there something that can be fixed to back the camera off?

@emackey
Copy link
Contributor

emackey commented May 29, 2015

Maybe the facilities.kml default view will be fixed by #2764?

@emackey
Copy link
Contributor

emackey commented May 29, 2015

Sadly, no, not fixed by #2764.

Also, BUG : This new feature overwrites the saved camera position in the URL added recently in #2706 and #2500.

@mramato
Copy link
Contributor Author

mramato commented May 29, 2015

Maybe the facilities.kml default view will be fixed by #2764?

Unfortunately not. viewer.flyTo uses the underlying camera.viewBoundingSphere, not viewRectangle. Weird views occur for all datasets that spans both sides of the globe. (Google Earth has different weird views as well). There are two issues that need to be addressed to improve this:

  1. Probably special case camera.viewBoundingSphere when the size of the sphere is close to the size of the earth.
  2. BoundingSphere.fromBoundingSpheres is inaccurate and does not actually produce an optimal boundingsphere. In many cases it produces a larger than expected bounding sphere and it's pulled in the direction of most data points. This produces weird, off-center views when zooming. There's an algorithm out there we need to implement to fix this, but no one has had time to do it.

@mramato
Copy link
Contributor Author

mramato commented May 29, 2015

This new feature overwrites the saved camera position in the URL added recently in #2706.

I'm not sure what you mean. I thought that feature continually updates the URL as the view changes. So when it flies to the data, the url reflects the new view.

@emackey
Copy link
Contributor

emackey commented May 29, 2015

Here's a link that in master will show you Wallops Island, not the South pole:

http://localhost:8080/Apps/CesiumViewer/index.html?source=..%2FSampleData%2Fkml%2Ffacilities%2Ffacilities.kml&view=-75.45817949184679%2C37.88841656484918%2C36340.62747313898%2C0.7736803327114128%2C-89.81362728388645%2C0

@emackey
Copy link
Contributor

emackey commented May 29, 2015

Basically, if the source and view parameters are both specified, CesiumViewer shouldn't auto-compute a new view from the source when it first loads.

@mramato
Copy link
Contributor Author

mramato commented May 29, 2015

Yeah, I noticed what you mean, I'm fixing it now.

@mramato
Copy link
Contributor Author

mramato commented May 29, 2015

Should be ready.

@emackey
Copy link
Contributor

emackey commented May 29, 2015

Flights and bounding sphere issues aside, this behavior is better than master. Tests pass. Merging.

emackey added a commit that referenced this pull request May 29, 2015
@emackey emackey merged commit 7e2391f into master May 29, 2015
@emackey emackey deleted the fly-on-load branch May 29, 2015 14:46
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

3 participants