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

Fix invalid LngLat with no currently loaded DEM tiles #11339

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Dec 11, 2021

Launch Checklist

Closes #10610

When terrain is enabled and the map is quickly zoomed, map.elevation.visibleDemTiles occasionally returns an empty array while awaiting DEM tile loading. This previously caused getBounds to return null values, As explained here by @ted-piotrowski:

If elevation.visibleDemTiles is empty array and elevation.exaggeration() > 1 then minmax.min * elevation.exaggeration() will equal Infinity, possibly producing a NaN down the line:

This PR adds a fix by defaulting to the 2D getBounds behavior if no terrain is available.

This leaves undefined behavior in the application, where getBounds returns different results depending on whether terrain is loaded or not. It's also likely that if 1+ but not all DEM tiles are loaded, the bounds returned could be insufficient to completely cover all terrain, though this did not nor will they now throw an error.

I suspect that these cases are subtle enough to be only a minor issue. If we decide to address this in the future, we should consider the performance cost of delaying getBoundscalls until terrain is loaded.

  • 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>Fixed 'getBounds' sometimes returning invalid LngLat when zooming on a map with terrain.</changelog>

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit but looks good to me otherwise.

src/geo/transform.js Outdated Show resolved Hide resolved
@@ -1301,6 +1302,7 @@ class Transform {
_getBounds3D(): LngLatBounds {
assert(this.elevation);
const elevation = ((this.elevation: any): Elevation);
if (!elevation.visibleDemTiles.length) { return this._getBounds(0, 0); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

@SnailBones SnailBones merged commit b1cfebf into main Dec 14, 2021
@SnailBones SnailBones deleted the aidan/getbounds-error branch December 14, 2021 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getBounds() is returning Invalid LngLat object when terrain is enabled and map is at high zooms or large tilt.
2 participants