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

Wrapping longitudes returned by getBounds() #10895

Closed
wants to merge 2 commits into from

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Jul 27, 2021

When the map is at very low zoom levels or clossing the 180th meridian, map.getBounds() returns coordinates with longitudes greater than 180 or less than -180. This fixes that by applying lng_lat.wrap() to the return values.

Depending on use cases, this could be a breaking change. If the user uses the returned boundaries to draw shapes on the map. I doubt that this is a common use case, but I'm not sure what other side effects this change might have.

Also, when the map is zoomed out slightly more than the world he boundaries might return something like NorthEast: [90, -170], SouthWest: [-90, 170]. Here while in fact the map is covering the whole world, the returned boundaries could just as well be interpreted as just a narrow slice around the 180th meridian. I'm not sure what side effects this might have.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

@rreusser
Copy link
Contributor

This makes sense. Which do you think is the desired behavior though? My only issue is that it's basically a loss of information so that if you receive unwrapped values, you can compute the wrapped value, but if you receive wrapped values, you can't go back the other way.

Or maybe you could. By detecting the case where min > max or max < min, you can presumably infer that the wrapping has happened and undo it, but it seems less robust.

Is there a linked issue or a particular use case this facilitates?

@rreusser
Copy link
Contributor

rreusser commented Jul 27, 2021

Loosely related, perhaps: #10447

@SnailBones
Copy link
Contributor Author

SnailBones commented Jul 27, 2021

Thanks for sharing your thoughts @rreusser !

I'm not sure what's the desired behavior and I'm not familiar with how users use getBounds(). It makes intuitive sense to me that a data structure with explicit directions to the corners can and should return standardized values.

Is there a linked issue or a particular use case this facilitates?

#10261

@rreusser
Copy link
Contributor

Thanks for the link, @SnailBones. I added a comment on the issue to the effect that I thought maybe NaN would be the result, but I only see unwrapped values which it's not clear to me are "invalid".

@rreusser
Copy link
Contributor

rreusser commented Jul 27, 2021

@astojilj has made a good point about testing and that maybe what we really want here is the following:

  1. status quo behavior: return unwrapped values potentially outside of [-180, 180] x [-90, 90]
  2. ensure (1) is covered by a unit test
  3. double-check map.getBounds does not account for horizon visibility and low zoom #10261 so see if there's a case in which it returns NaN, which is definitely invalid and should be fixed, if possible.

@ryanhamley
Copy link
Contributor

Here while in fact the map is covering the whole world, the returned boundaries could just as well be interpreted as just a narrow slice around the 180th meridian

I think this might be the primary concern against this. If you want a line or a polygon that spans the antimeridian, unwrapped bounds are useful for showing that it should go across the antimeridian and not the long way around the other side of the globe.

@SnailBones
Copy link
Contributor Author

I think this might be the primary concern against this. If you want a line or a polygon that spans the antimeridian, unwrapped bounds are useful for showing that it should go across the antimeridian and not the long way around the other side of the globe.

Exactly, in fact unwrapped coordinates are necessary for drawing a line or polygon across the antimeridian (as made clear in #3250).

On the other hand, I think the case that drawing lines or polygons based on the map bounds may be a rare to nonexistent use case in real-world applications. Do we have reasons for why someone would want to do this? Or are there other use cases of getBounds() that similarly produce unexpected results with wrapped longitudes?

I'd be interested in hearing @arindam1993's thoughts on this.

@SnailBones
Copy link
Contributor Author

SnailBones commented Jul 27, 2021

Seems that the consensus is that the behavior is intended, so I'll close this P.R. if there's no objections.

EDIT: On second thought, I'll revert the change to src and modify the new tests to ensure that unwrapped bounds are returned (the opposite of what they're testing for now.)

@SnailBones SnailBones closed this Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API bug 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

map.getBounds does not account for horizon visibility and low zoom
3 participants