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

Fix camera view/flight issues when colliding with terrain #4105

Merged
merged 7 commits into from
Aug 2, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jul 11, 2016

NOTE: This is not ready for code review. I'm opening it to get feedback for if this is actually what we want to do. If we decide to go with it, I'll clean up the code and add tests.

Disable camera input before a flight and do not re-enable until the terrain has finished loading for the area.

Fixes #4075.

@mramato
Copy link
Contributor

mramato commented Jul 11, 2016

This seems to be completely broken. When I use the sample code from #4075, the camera rotates, but does not move.

@bagnell
Copy link
Contributor Author

bagnell commented Jul 13, 2016

I forgot to mention that there is a problem with the code sample. You need to clone the camera position, not store the reference or the position will stay the same.

@mramato
Copy link
Contributor

mramato commented Jul 14, 2016

OK, thanks, I fixed the example so no one makes the same mistake again.

@mramato
Copy link
Contributor

mramato commented Jul 14, 2016

This is definitely better in that it recreates the view properly, the only (major) downside is that since it waits for all tiles to load, the camera can appear "stuck" for several seconds (5 in my case) while tiles are rendering. Perhaps this is unavoidable given our current loading architecture? Are we actually waiting for all tiles, or just all tile sunder the camera position?

@pjcozzi since you reported this problem, I would recommend you test this out. The problem is definitely gone, but flying from the home view to a ground horizon view (which is the worse case scenario) definitely has a long delay.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

@pjcozzi since you reported this problem, I would recommend you test this out. The problem is definitely gone, but flying from the home view to a ground horizon view (which is the worse case scenario) definitely has a long delay.

Sounds bad, even with Network throttling, I did not notice this. After I zoomed to the horizon view, I was able to pan even while the tiles (maybe just imagery?!?!?) load in.

@mramato
Copy link
Contributor

mramato commented Jul 14, 2016

Sounds bad, even with Network throttling, I did not notice this. After I zoomed to the horizon view, I was able to pan even while the tiles (maybe just imagery?!?!?) load in.

Did you have terrain on? From my laptop right now I have major lag. I can demo for you tomorrow.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

Yes, with terrain.

@bagnell do you have a similar lag?

@bagnell
Copy link
Contributor Author

bagnell commented Jul 14, 2016

It takes about 3-4 seconds before I can move the camera again when flying from the home view.

Are we actually waiting for all tiles, or just all tile sunder the camera position?

Its waiting for all tiles. The other way to do it would be to repeatedly try to pick the terrain under the camera. As soon as some terrain is picked, we can unlock the camera. The down side is that the camera could really be positioned well below the terrain so it may never get picked.

@bagnell
Copy link
Contributor Author

bagnell commented Jul 14, 2016

I changed it to only disable collision detection until the terrain is loaded not input like we talked about offline.

@pjcozzi @mramato Let me know what you think. I think its worse.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

Up to @mramato if we want to take this; otherwise, we can wait a bit for a real fix that prefetches terrain at the flight's target.

@mramato
Copy link
Contributor

mramato commented Jul 15, 2016

I think the latest behavior is a good compromise until we refactor terrain and imagery loading.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 17, 2016

I agree, this will get it done!

@bagnell

@bagnell
Copy link
Contributor Author

bagnell commented Aug 1, 2016

@pjcozzi @mramato This is ready for another review.

Camera.prototype._adjustHeightForTerrain = function() {
var scene = this._scene;

// Should these be moved to the camera?
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this comment and submit an issue for this if you think it is worth it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 1, 2016

@mramato can you please test and merge when happy?

@mramato
Copy link
Contributor

mramato commented Aug 2, 2016

Looks good to me, once @pjcozzi's one comment is addressed, I'll merge.

@bagnell
Copy link
Contributor Author

bagnell commented Aug 2, 2016

@mramato This is ready.

@mramato
Copy link
Contributor

mramato commented Aug 2, 2016

Thanks!

@mramato mramato merged commit 950eee4 into master Aug 2, 2016
@mramato mramato deleted the camera-terrain branch August 2, 2016 19:15
@pjcozzi pjcozzi mentioned this pull request Aug 2, 2016
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants