Alternate zoom with terrain

[avpeery]

Fixes #10197

Previously, constraining camera altitude sometimes avoided going under terrain by changing the pitch along with the camera orientation (see line #1685). However, this has resulted in a not ideal user experience when zooming into terrain and the camera is pitched when that is not the desired interaction.

See example of accidental pitching currently in main:

Screen.Recording.2022-12-11.at.11.26.12.PM.mov

In addition, interactions in main still allowed for clipping through terrain in cases of dragging and rotation.

More current non-user-friendly gesture handling in main:

Untitled.mov

This change fixes most of these cases.

After:

Screen.Recording.2022-12-11.at.11.16.21.PM.mov

Screen.Recording.2022-12-11.at.11.16.58.PM.mov

First this change fixes unintended pitching that sometimes happens when zooming into terrain. Pitch was changed from this line of code. The call to orientationFromFrame adjusts the pitch and bearing based on the adjusted camera position - we do not want this functionality to alter pitch or bearing.

Second this fix resolves most cases of clipping under terrain. By forcing gestures such as zooming, panning forward to stop when at an unsafe distance from terrain or by pushing the camera height up when dragging in order to prevent dragging under terrain. The choice was made to limit gestures in gesture handling and in the keyboard handling. In order to stop a gesture from continuing towards it destined path, we need to zero out its delta value, otherwise the gesture will continue towards its path which could result in clipping under terrain.

However, since clipped terrain does occur in main, we can opt to remove the change here that constrains keyboard handling. Constraining keyboard handling does need to exist inside the keyboard handler as this code path executes immediately, and does not follow the same code path as other gesture handlers in the handler_manager.js file. The keyboard handler does already take in the map and runs other map methods within it, so I do feel it is low risk to introduce map.transform._isCameraContrained in the keyboard handler to prevent zooming and panning forward at an unsafe distance from terrain. I believe stopping forward panning and zooming with keyboard is preferred over altering the camera height during these movements. Unlike dragging, it does not feel natural to adjust the camera height with keyboard movements. The keyboard movements do not depend on force of motion and therefore it is not a smooth experience to push the camera height up during keyboard movements.

In addition, stopping zooming during wheel events and double click events is preferred over adjusting the camera height. A user zooms to get closer to a specific location. If we take the same code path to adjust the camera height while zooming, the user will be pushed over the mountain, and sometimes the inertia picks up over the other side of mountain. This creates a jarring unintended consequence to zooming if the user is trying to zoom into the side of the mountain.

When dragging or when "under" terrain (when turning on and off terrain) or when rotating into the side of a mountain, a code path is triggered that pushes the camera above the terrain to avoid clipped terrain. This code path gets executed when camera height is less than 0 (this indicates camera is under terrain) or while dragging. After testing other calculations to adjust the z position (such as setting the z position to the z position acceptable by the minimum height) are not as exact as the current behavior of the original calculation that adjusts the camera position z value (introduced by internal pr #44). I've adjusted this calculation to only affect the z value, but otherwise it remains the same.

In the future, it would be ideal to overhaul the current gesture handling. This is a good fix for now that greatly improves a frustrating user experience when using terrain.

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• manually test the debug page
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Allow zooming towards terrain at a safe distance without pitching the camera</changelog>

[avpeery]

@SnailBones I updated the keyboard movement (i.e. panning forward and zooming) to be consistent with scroll zooming. The check for cameraHeight less than zero is for when camera is actually under the terrain (which would happen in the case of switching on and off terrain) since the logic there forcefully pushes the camera above it... in cases of zooming though we just want to stop the camera from getting too close (and not forcefully adjust the camera position elsewhere). Thanks for bringing keyboard movements up!

[SnailBones]

Also not introduced in this PR, but I'm curious as to why we adjust the camera along the vector of camera to center? Why not simply raise the camera`s z value?

[SnailBones]

I'm concerned that including this logic in the keyboard handler introduces excess complexity and fragility. Could we have the same behavior on all easeTo and flyTo events (or better yet, all map movements?), allowing us to keep interactions and terrain handling separate?

[avpeery]

Thanks for the feedback - I'll look into easeTo in general (I don't think flyTo is used in any of these movements). There does need to be some map movements that are allowed, so the user can zoom back or pan back when getting too close to terrain (otherwise they will get stuck)

[SnailBones]

There does need to be some map movements that are allowed, so the user can zoom back or pan back when getting too close to terrain (otherwise they will get stuck)

Would it be possible to decide if a movement is allowed or not allowed from with transform.js?

Do you think that we could change the behavior of updateElevation _constrainCamera() so that they don't need any arguments and camera constraint is based on (invalid) position alone? Then instead of blocking specific interactions, invalid ones would essentially be a noop.

Or is it important for user experience that we allow zooming to a higher zoom level while constraining panning to a lower one? (The latter may well be the case, both mouse interactions seem to work well in this PR)

Besides code readability and maintainability, another reason why I think separating interaction and camera adjustment code would be preferable is that it's hard to predict how users will interact with camera position. For instance withreact-map-gl map position is overridden at every frame (the reason is to sync position with kepler.gl, but this applies to developers using the extension whether they use kepler.gl or no). So if clipping prevention depends on the specific type of interaction (or even the specific event called), we might see unpredictable behavior.

[SnailBones]

The check for cameraHeight less than zero is for when camera is actually under the terrain (which would happen in the case of switching on and off terrain) since the logic there forcefully pushes the camera above it... in cases of zooming though we just want to stop the camera from getting too close (and not forcefully adjust the camera position elsewhere).

@avpeery Thanks for the explanation! If I'm understanding correctly: dragging and terrain clips place the camera a certain distance back from terrain, while zooming just prevents further zooms when in contact with terrain. Other camera motions (keyboard panning, flyTo(), etc) are handled as is zooming. Should we consider instead handling them like mouse panning (i.e. keep the minimum distance)? Two issues that I think might be solved by this:

• I'm still noticing that keyboard panning (arrow keys) is stopped by hills instead of being zoomed out like mouse panning. I don't think it's necessarily a problem but it's worth considering the alternative.

• Also noticing a strange behavior with keyboard rotation (shift + arrow keys) into a mountain, where the adjusted position seems quite far away and behind the starting position. I presume this is happening because it's triggering a terrain clip (though I'm not sure why the distance is so large, it might be worth walking through the logic in the case of non-drag clips)

[karimnaaji]

Are there more situations where we need to run this path? I'm not sure whether this should be tied to the drag gesture and instead run always. While testing the PR, I've noticed that when the camera has some inertia due to a click-drag-release gesture, it results in the camera getting too close to the terrain, then when picking it back up with a drag gesture the constraint gets applied again resulting in an immediate camera jump.

[avpeery]

If we run it always we experience a weird affect of jumping while panning/zooming, but it felt more intuitive with dragging and allows for the user to keep moving forward in the terrain by dragging if they zoom too close. I'll check out the mouse codepath that @SnailBones suggested. Thanks for the feedback and testing it out @karimnaaji!

[avpeery]

Thanks for the thorough review @SnailBones - Yes I agree it feels a bit risky to place code directly in keyboard. In order to stop zooming though I do need to intercept the gestures to stop the action. I might look into establishing an internal method that can be used to stop gestures in place on the map?

• I'm still noticing that keyboard panning (arrow keys) is stopped by hills instead of being zoomed out like mouse panning. I don't think it's necessarily a problem but it's worth considering the alternative.

Do you mean panning by drag panning using the mouse? This triggers isDragging to be true, unfortunately testing this logic out for keyboard movements doesn't look natural like it it does for drag pan.

• Also noticing a strange behavior with keyboard rotation (shift + arrow keys) into a mountain, where the adjusted position seems quite far away and behind the starting position. I presume this is happening because it's triggering a terrain clip (though I'm not sure why the distance is so large, it might be worth walking through the logic in the case of non-drag clips)

Yes it is from the calculation, so there's a jump when rotating (if you are getting close to being under terrain) - would it be better to stop rotation like zooming? Otherwise we do have to adjust the camera position to allow rotating otherwise we risk terrain clip.

[SnailBones]

Approving as this is very much an improvement to the current behavior!

I'm still noticing two issues that we might want to open tickets to address in the future.

When mouse pitching or rotating into terrain, I1m seeing unexpected camera jumping and a change in zoom level. Maybe the ideal behavior would be a more subtle camera adjustment? Though this is still an improvement over clipping!

Screen.Recording.2022-12-12.at.3.07.39.PM.mov

I'm also noticing that in some cases, keyboard panning is halted even when it seems like it should allow movement. Again, still an improvement over the changing pitch behavior in main.

Screen.Recording.2022-12-12.at.3.10.15.PM.mov

[avpeery]

Thanks for the review @SnailBones

I'm also noticing that in some cases, keyboard panning is halted even when it seems like it should allow movement. Again, still an improvement over the changing pitch behavior in main.

I can remove the constraint on keyboard forward panning. It doesn't risk clipping. Here is is without constraining for forward panning

Screen.Recording.2022-12-13.at.8.38.37.AM.mov

[SnailBones]

I'm also noticing that in some cases, keyboard panning is halted even when it seems like it should allow movement. Again, still an improvement over the changing pitch behavior in main.

I can remove the constraint on keyboard forward panning. It doesn't risk clipping. Here is is without constraining for forward panning

Do you know if that makes a difference in situations where the camera is grazing a peak, and is blocked by terrain that's not visible onscreen (like in the video I share above)?

This issue doesn't occur on mouse/drag panning due to the behavior you've implemented with the camera zooming out and rising above the obstacle terrain. My intuition is that extending this behavior to keyboard panning would solve this issue (and perhaps ease map navigation by keyboard) But given you tested this and found it looked less natural, I'm happy to merge this as is, and the possibility of getting stuck above a peak is a suitably small edge case that we can address it in the future.

[avpeery]

@SnailBones It looks like grazing peak does not fall into the camera jump code path with dragging for keyboarding panning. I have tested changing the camera position (as in dragging) in keyboard movements and it does not feel intuitive. However, I don't believe keyboard panning needs to be accounted for here. Maybe the behavior for keyboard panning has changed in main since I first opened this PR, as keyboard movements (aside from rotating - which does use the camera change code path to avoid clipping) seem acceptable without being constrained.

Screen.Recording.2022-12-13.at.11.29.43.AM.mov

[karimnaaji]

Thanks for the very detailed explanation and detailed reasoning of the choices made here, this LGTM as all of the concerns mentioned in previous reviews were addressed and the behavior works great. Left one minor suggestion, otherwise good to merge for next release.