-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Camera underground controls #8811
Conversation
Thanks for the pull request @lilleyse!
Reviewers, don't forget to make sure that:
|
0c74a4c
to
65a49de
Compare
a952848
to
a54a7ba
Compare
I pretty much have the underground navigation where I want it now. Tomorrow I'll do some final testing and then bump @IanLilleyT for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level: much better than before but prone to unexpected movements depending on where you click and how fast you move. This has afflicted the camera code for a while so I wouldn't say it's a lot worse than above-ground behavior.
Left click (strafe) feels pretty good.
Tilt (middle-click) feels a bit wonky at times, seeming to rotate in a circle around nothing. And spamming the cursor while tilting will eventually get the camera into a completely different position, losing the original pivot point (this could be more of a general camera controller bug).
Right click (zoom) sometimes moves in an unexpected direction, as if it's both strafing and zooming, and will sometimes fix itself mid-zoom (haven't found a consistent repro, just click different spots on the screen and zoom in and out quickly and it's bound to happen eventually).
Moving the camera deliberately near stuff that's close to the surface works pretty well. The crazier the mouse motion gets, the more likely one of the above bugs happens and throws the camera somewhere unexpected.
Not a big fan of the interior surface distance because it gets the camera stuck pretty often, especially when trying to move away from it back towards to surface. I've tried "disabling" is by setting it to -6000000 and it feels ok, though it makes it easier to lose your place.
I agree a camera code rewrite is tempting, but this works pretty well in the meantime. A future system should be informed about objects that have collision volumes and should do waaaay more raycasts to determine the camera's proximity to objects in the scene for smarter avoidance. Kind of like a distance field that the camera moves through.
* @default -20000.0 | ||
* @private | ||
*/ | ||
this._undergroundSurfaceHeight = -20000.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this causes more confusion than it's worth. When I try zooming to the center of the earth, it feels like the camera is going nowhere for a long time, possibly made worse by being at a glancing angle to the undergroundSurfaceHeight. If someone needs an underground distance limiter, it might make more sense for them to place a large underground ellipsoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 8b92dab. Seems ok with the sandcastles I've tried and makes the code simpler.
/** | ||
* The height of an invisible surface that the camera uses for underground navigation. | ||
* For best results the height should be set just below the height of underground entities in the scene. | ||
* Note that the camera can still zoom below this height if {@link minimumZoomDistance} is greater than 0.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really get why minimumZoomDistance
is relevant for whether you can go underground or not. I figured it would be ignored completely if enableCollisionDetection === false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If minimumZoomDistance
is 0.0 then it will approach a surface but never zoom through it. The controller treats collision detection and zoom distance as separate concepts, e.g. you can rotate the camera through the globe regardless of minimum zoom distance.
var translucent = scene.globeTranslucencyState.translucent; | ||
var cullBackFaces = | ||
!controller._cameraUnderground || | ||
(globe.depthTestAgainstTerrain && !translucent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is depthTestAgainstTerrain
needed here if cullBackFaces
is only used for CPU picking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not needed. At one point this fixed a bug but I'm not noticing issues with depthTestAgainstTerrain
removed.
|
||
var windowPosition; | ||
|
||
if (cameraUnderground) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use startPosition
all the time instead of only when underground.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it. I'm not sure what will break for above ground navigation. At the same time this could improve camera navigation on the whole.
This could be a good thing to try post June 1st.
9701364
to
45dfb2d
Compare
The only time I see this is when there's inertia from a pan still happening. |
At this point a lot of the remaining problems are caused by multiple actions happening simultaneously, usually because of inertia. Fixed one pretty serious bug last night where zooming past the pan ellipsoid causes the camera to pan extremely far away. This happens in master too and could explain unexpected camera jumps. The next set of things to look at are strafing + zooming and strafing+ tilting combinations like #8811 (comment). These also happen in master but become more noticeable when underground. |
40e93dc
to
b103ebf
Compare
@IanLilleyT ready for another look |
aa64532
to
6c8d1c1
Compare
The recent changes made the underground camera controls more stable and even fixed a couple problems with existing above ground code, so this is good to go, thanks @lilleyse ! |
36ee69b
to
5fda45a
Compare
Opening this PR for early testing. There's still some work to be done, especially for panning. While I had the urge to rewrite
ScreenSpaceCameraController
I took the opposite approach and just modified camera behavior when underground. Above ground controls should be the mostly same, for better or for worse.At a high level the changes include:
enableCollisionDetection
is false andminimumZoomDistance
is greater than 0.0. I'm occasionally noticing large jumps which I still need to fix.ScreenSpaceCameraController
calledundergroundSurfaceHeight
which acts as an invisible ellipsoid that's used to improve camera navigation when underground. For best results the height should be set just below the height of underground entities in the scene.Local sandcastle for testing
@fredj @gberaudo