Prevent pitching when zooming into terrain

[avpeery]

Fixes #10197.

Previously, constraining camera altitude adjusted the camera z position to be positioned above the terrain (see line #1676) and then adjusts the pitch with camera orientation (see line #1685). This change removes these lines, and just ensures that the camera positions are a safe distance away from the terrain when the camera height is lower than the terrain height.

Before:

Screen.Recording.2022-10-18.at.12.48.03.PM.mov

After:

Screen.Recording.2022-10-18.at.12.47.08.PM.mov

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
• 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 into terrain without unintended pitching.</changelog>

[SnailBones]

Testing this out, it seems that the change exacerbates clipping through terrain.

Screen.Recording.2022-10-18.at.3.04.42.PM.mov

[avpeery]

Testing this out, it seems that the change exacerbates clipping through terrain.

Thanks for testing this @SnailBones! I'll look into this (didn't come across this previously). Any specific settings you are using?

[SnailBones]

Thanks for testing this @SnailBones! I'll look into this (didn't come across this previously). Any specific settings you are using?

I can reproduce this reliably by going to http://localhost:9966/debug/terrain-debug.html#15.24/35.147619/-106.425675/83.6/85 and panning the map forward. On main, we clip through while dragging but the camera jumps out of the mountain on mouse release, but in this branch the camera stays inside of the mountain.

[avpeery]

@SnailBones @karimnaaji Fixed failing unit test - pitch needed to be reset from the previous unit test. Camera under terrain issue fixed while dragging. One trade off is if you zoom forcefully downwards into terrain it will bounce back a couple times - this is because we need to reposition the camera back before hitting terrain which prevents clipping through terrain.. We can create an issue for smoother close zooming in terrain to address later but it is better than automatically pitching and is otherwise relatively smooth.

[SnailBones]

What's the reason for hard-coding this value? Maybe add context in a comment?

[avpeery]

Added a comment. In maps with maxPitch set to less than 85, fast-zooming past terrain is exasperated since it would adjust the camera position too early. Hard-coding this value to 85 instead of maxPitch reduces this experience in these cases

[karimnaaji]

Since Math.cos(degToRad(85)) < Math.cos(degToRad(0)), should we use the upper bound of the possible values in order to keep a constant minHeigh safe bound? That would mean directly using:

        const minHeight = this._minimumHeightOverTerrain();

Since Math.cos(degToRad(0)) is 1. Would that work or lead to issues in practice?

[avpeery]

The initial experience of using minHeight = this._minimumHeightOverTerrain() is a bit more jarring. if you bounce into the terrain it pushes the camera farther up and away. It also allowed for the camera to go under the terrain. I'll look into why

[SnailBones]

Nit: If this is the correct value, should we consider hard coding the result in order to avoid an unnecessary cos() operation?

Suggested change

	const minHeight = this._minimumHeightOverTerrain() * Math.cos(degToRad(85));
	const cosMaxPitch = 0.08715560745961583
	const minHeight = this._minimumHeightOverTerrain() * cosMaxPitch;

Though, have you experimented with other values to see if you the behavior can be smoother?