[core] Avoid wrapping longitude values of exactly 180 and 360 (#12797)

[ryanhamley]

Fixes #12797

The thinking here is that 180 and 360 are special values that do not need to be wrapped. In the case of #12797 unwrapForShortestPath was causing a bug because -180 was being converted to 180 and confining the viewport to the antimeridian when it should have been possible to scroll horizontally around the world.

TODO:

• add test(s) - not needed for this. tests for generalized bounding box behavior are in progress at [Core] Add tests for bounding boxes that cross the antimeridian #13087
• update CHANGELOG

[ryanhamley]

@ansis I was told you may have some knowledge on this area of the codebase. Does this change look reasonable as a solution or is this likely to have unexpected adverse effects elsewhere?

cc @mollymerp

[julianrex]

/cc @captainbarbosa for potential impact on your recent iOS change.

[ansis]

This is supposed to fix the conversion from java bounds to native bounds, right?

I'm not sure that unwrapForShortestPath is what is wanted here.

Let's say you have these java bounds that cover most of the width of the world:
longitudeWest is -170 and goes into sw.lon
longitudeEast is 170 and goes into ne.lon

I think unwrapForShortestPath changes sw.lon to 190, which after the LatLngBounds::hull(sw, ne) would create the native bounds with sw.lon being 170 and ne.lon being 190, which is the opposite of what we would want?

I haven't run this to confirm I'm not reading this wrong, but why would bounds want to use the shortest path between two locations?

It looks like this was added in https://github.com/mapbox/mapbox-gl-native/pull/11807/files

[LukasPaczos]

@ansis #11807 was added to close #11733 which is a popular scenario where users would like to position a camera based on bounds that cross the antimeridian, so values passed to JNI are that lon east < lon west. We need to unwrap them, otherwise, as you pointed out, the LatLngBounds::hull(sw, ne) would create bounds that wrap a world around and base the new camera position on that, which is not what the users expect.

[ansis]

@LukasPaczos It looks like the example from my previous comment produces unexpected results (thanks @ryanhamley for running this).

mapboxMap.setLatLngBoundsForCameraTarget(LatLngBounds.from(20, 170.0 /*east*/, -20.0, -170.0 /*west*/));

In the resulting map the east side is -170 and the west is 170:

---

My suggestions:

• come up with examples for all the cases and implement these as tests so that one case isn't regressed while fixing another. It seems like cases that need to be included are:
  • a box > 180 degrees wide that crosses the date line
  • a box > 180 degrees wide that doesn't cross the date line
  • a box < 180 degrees wide that crosses the date line
  • a box < 180 degrees wide that crosses the date line
  • a box that allows unrestricted horizontal panning but restricted vertical panning
• Unless there is a good reason for the java bounds type and the c++ bounds type to mean different things, avoid do any of these adjustments when converting between them. Decide what should allow as values and what they mean and just pass these straight through from java.
• The bounds type has east and west values. When setting the map to the bounds the location at the east side of the map should be the east value and the west side should be the west value. These should not be flipped.
• Check how iOS does this. I think the solution should be the same on both platforms.

More open questions (that could already be settled by the core implementation):

• How can use specify bounds that limit vertical panning but not horizontal? west: -180, east: 180? Or any bounds where east - west >= 360?
• Is east < west allowed? (I think so)

---

Relatedly, a while back John made a pretty convincing case for eliminating bounds and using geojson instead of the north/east/south/west format: mapbox/mapbox-gl-js#2112 (comment)

[ryanhamley]

How can use specify bounds that limit vertical panning but not horizontal? west: -180, east: 180? Or any bounds where east - west >= 360?

This is what this PR was originally intended to fix (#12797). According to that bug report,
mapboxMap.setLatLngBoundsForCameraTarget(LatLngBounds.from(40, 180.0, -40.0, -180.0)); is expected to limit vertical panning while not limiting horizontal panning.

[1ec5]

Check how iOS does this. I think the solution should be the same on both platforms.

This documentation describes the expected wrapping behavior on iOS. Note that it differs from MapKit’s behavior, which tries to guess at the developer’s intent while constraining the longitude to ±180°. It’s expected that MGLCoordinateBounds.sw lies to the west of MGLCoordinateBounds.ne, but to my knowledge nothing actually enforces this expectation at the SDK level.

mapbox-gl-native/platform/darwin/src/MGLGeometry_Private.h

Lines 55 to 58 in f4ffcaa

	NS_INLINE mbgl::LatLngBounds MGLLatLngBoundsFromCoordinateBounds(MGLCoordinateBounds coordinateBounds) {
	return mbgl::LatLngBounds::hull(MGLLatLngFromLocationCoordinate2D(coordinateBounds.sw),
	MGLLatLngFromLocationCoordinate2D(coordinateBounds.ne));
	}

Relatedly, a while back John made a pretty convincing case for eliminating bounds and using geojson instead of the north/east/south/west format: mapbox/mapbox-gl-js#2112 (comment)

For what it’s worth, MapKit largely eschews the concept of a bounding box in favor of a “region”, which consists of a center point and span (vertical and horizontal degrees spanned by the region). This center-centric approach is suitable for transitions between Mercator and non-Mercator projections, as when switching between standard and satellite modes in Apple Maps. It also neatly avoids any problems spanning the antimeridian.

[LukasPaczos]

We're planning on removing forced wrapping of longitude values in the upcoming Android semver major release (see #13149) which will enable us to pass values straight to core without any manipulation. This, I believe, will establish parity with the iOS implementation.

[ansis]

Merging this as a temporary fix seems ok!

Long term fix is #13006 (comment) to address #13186.

This will not need to be undone after those changes because I think this change makes sense for the expected uses of the method as well.