Do not update camera zoom when DEM data isn't available yet

[karimnaaji]

Make sure to not update camera zoom when the terrain DEM data isn't available at center point, which is the reference to calculate the camera zoom over terrain. This fixes the following issue that was reported by customer:

Edited-gljs.mov

More description of the issue and a reproducible case is available here https://github.com/mapbox/gl-js-team/issues/341

Launch Checklist

• briefly describe the changes in this PR
• 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>Fix a potential issue with camera going under the terrain when the DEM data isn't available at center point.</changelog>

[SnailBones]

I'm still recreate the linked issue in this PR by zooming before terrain loads. This makes me think that the issue might be in the camera transformation algorithm itself.

[karimnaaji]

Thanks for testing and the catch @SnailBones ! I'll close that tentative fix for now, until we find a more appropriate one as it's still possible to trigger that issue when zooming in before the terrain is loaded (resulting in the camera being below ground).

[SnailBones]

Great work @karimnaaji!

A few nits: thoughts on changing the variable names to make the code more clear?

For instance, cameraZoom could perhaps be cameraHeight since zoom suggests a relationship to map zoom.

And perhaps _updateCameraOnTerrain could be called _resetCameraIfNoTerrain.

The unit tests look great. Have you looked into render tests for this? Would it be possible to simulate the case that was causing this bug by placing the camera at high pitch and zoom and loading terrain after a wait?

[karimnaaji]

For instance, cameraZoom could perhaps be cameraHeight since zoom suggests a relationship to map zoom.

That's a good point, cameraZoom is a bit of a misleading name. It is the zoom based on MSL (Mean Sea Level), which is also the default zoom when terrain is disabled. When terrain is enabled, the default zoom (transform._zoom) is the zoom at the center point, which accounts for the altitude at this given point. I've renamed it to be seaLevelZoom to better reflect the reference point.

And perhaps _updateCameraOnTerrain could be called _resetCameraIfNoTerrain.

Hmm 🤔 I've kept _updateCameraOnTerrain as it is more generic about what this function does. I found _resetCameraIfNoTerrain less clear, as it may only reflect one branch of that function.

The unit tests look great. Have you looked into render tests for this? Would it be possible to simulate the case that was causing this bug by placing the camera at high pitch and zoom and loading terrain after a wait?

Good call, although I think the unit test to be enough in that particular instance. The original issue is network-dependent and more difficult to recreate in a render test the way it is done in the unit test without introducing flakiness. Render tests are a better fit for visual testing or to be on-par with gl-native rather than a fine-grained data order/network testing.

[SnailBones]

I like the change to _seaLevelZoom, LGTM!