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

Halt camera zoom #4967

Merged
merged 1 commit into from
Feb 9, 2017
Merged

Halt camera zoom #4967

merged 1 commit into from
Feb 9, 2017

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Feb 8, 2017

This is the dumbest possible fix for #4855, much hackier than @kring's abandoned fix, but hear me out.

Basically, the camera "target" is decided when the zoom begins, and is not recalculated every frame of the zoom (for UX reasons). As such it is possible to get the camera into a situation where it is zooming close to and then past a small entity, such as the Aircraft model in the 3D models sandcastle demo.

@duvifn deserves the credit for finding the spot where the problem begins. He isolates a place where a dot product flips over, this indicates that the camera has already zoomed past its own target in the previous frame of zoom, and is about to flip to the other side of the planet (due to Math.acos being applied to the flipped dot product).

When this "error condition" is detected, the camera is already just barely past its target, and a decision needs to be made: Other than flipping to the opposite side of the Earth, what can we do?

  • Move the camera backwards, to be in front of the target again? No, that leads to bounce.
  • Call Math.abs and pretend that the target is still in front of us? No, that leads to Earthquake Cam.
  • Stop. Just stop the camera from zooming, until the user selects a new zoom target by releasing the right mouse or pinch action and starting a new one.

This PR takes the last option above, so the camera halts one frame after passing the target, and waits for the user to start a new zoom action.

@emackey
Copy link
Contributor Author

emackey commented Feb 8, 2017

It's worth noting that @duvifn's sample code will respond very poorly to this fix. This sample consistently places a white dot 300 meters in front of the mouse pointer while you are trying to zoom. As a result, you are almost always zooming past the dot with any zoom you attempt. This fix will cause the camera to keep locking up as you try to do this, which is better than flipping across the globe, but not good. Even so I claim this is a poor example, the app should not be placing a dot 300 meters in front of the mouse pointer and expecting zoom to work correctly, you'd be forever limiting your zoom range to the nearest 300 meters.

@bagnell
Copy link
Contributor

bagnell commented Feb 9, 2017

The code looks good to me. The problem was fixed for me while testing. Merging.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 9, 2017

@emackey please update CHANGES.md in master.

@duvifn
Copy link
Contributor

duvifn commented Feb 9, 2017

Thanks @emackey! This works great and it solves the problem for me.

Just one comment (I'm not sure it worth fixing):

It's a common use case to zoom with a mouse wheel while the mouse cursor is fixed in same position.
If the zoom target is an entity, the camera gets locked (for zoom in and zoom out) when it pass through it unless a mouse movement is done.

This might be an annoying problem in machines with pickPosition problems.

You can check this with the examples in this comment.

One possible hacky solution that worked for me is to reset the object._zoomMouseStart just before the return in your fix so the next call to handleZoom will update object._zoomWorldPosition again.

Thanks again.

@kring
Copy link
Member

kring commented Feb 10, 2017

This is an improvement on my dodgy system, but not great. The camera doesn't fly off to nowhere anymore, but zooming by right-click dragging is nearly impossible because it just halts constantly. I think it's safe to say there's something wrong with depth picking on this system, though I haven't been able to figure out what.

Just a thought, but why not tweak the behavior when detecting the error condition? Instead of halting, just initiate another pick and continue zooming from there. Basically, do what my abandoned PR did, but only when the error condition actually occurs.

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.

5 participants