Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios, macos] Document setting visible coordinates that cross antimeridian #9804

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

fabian-guerra
Copy link
Contributor

@fabian-guerra fabian-guerra commented Aug 18, 2017

Adds documentation to clarify how to use coordinate bounds that cross the anti-meridian.

@fabian-guerra fabian-guerra added this to the ios-v3.6.2 milestone Aug 18, 2017
@fabian-guerra fabian-guerra self-assigned this Aug 18, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

I think it would be best if we fix this issue in mbgl so that the Android and macOS SDKs also benefit from the fix. mbgl already allows features and shape annotations to straddle the antimeridian by specifying vertices with longitudes beyond ±180°. Can you investigate how that works and adapt Transform to do likewise?


mbgl::CameraOptions cameraOptions = _mbglMap->cameraForLatLngs(latLngs, padding);

if (boundariesCrossDateTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code base calls it the “antimeridian”, not the “date time”.


@return A coordinate at given fraction. */
// Ported from http://www.movable-type.co.uk/scripts/latlong.html
NS_INLINE CLLocationCoordinate2D MGLCLCoordinateAtFraction(CLLocationCoordinate2D origin, CLLocationCoordinate2D destination, double fraction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of code to inline. Move the body of the function to MGLGeometry.mm.

@1ec5
Copy link
Contributor

1ec5 commented Aug 18, 2017

Can you investigate how that works and adapt Transform to do likewise?

Actually, it looks like the method affected by this PR uses -cameraForCameraOptions:, so I’d look into whether mbgl::Map::cameraForLatLngs() needs to know how to straddle the antimeridian.

@friedbunny
Copy link
Contributor

This seems like good candidate for testing; perhaps in MGLGeometryTests.

@fabian-guerra
Copy link
Contributor Author

@1ec5 I changed the implementation to follow this https://github.com/mapbox/mapbox-gl-native/blob/release-ios-v3.6.0-android-v5.1.0/platform/darwin/src/MGLPolyline.h#L36 that led me to avoid adding new functions.

@fabian-guerra fabian-guerra changed the title [ios] Fix visible coordinates when boundaries cross international datelline [ios, macos] Fix visible coordinates when boundaries cross international datelline Sep 6, 2017
@@ -2667,6 +2667,11 @@ - (void)_setVisibleCoordinates:(const CLLocationCoordinate2D *)coordinates count
{
latLngs.push_back({coordinates[i].latitude, coordinates[i].longitude});
}

if (count == 4 && coordinates[1].longitude > 0 && coordinates[3].longitude < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This public method is intended to work for an arbitrary number of coordinates.

@@ -2707,6 +2712,19 @@ - (void)_setVisibleCoordinates:(const CLLocationCoordinate2D *)coordinates count
[self didChangeValueForKey:@"visibleCoordinateBounds"];
}

- (std::vector<mbgl::LatLng>)normalizeCoordinatesForAntimeridianSW:(CLLocationCoordinate2D)sw ne:(CLLocationCoordinate2D)ne
{
CLLocationDegrees longitude = (180 + (180 - fabs(sw.longitude))) * -1;
Copy link
Contributor

@1ec5 1ec5 Sep 8, 2017

Choose a reason for hiding this comment

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

If I’m not mistaken, the mbgl::LatLngBounds class can handle spanning the antimeridian – that’s how the existing shape annotation functionality is able to handle the antimeridian gracefully. See the mbgl::LatLngBounds::extend() and mbgl::LatLng::unwrapForShortestPath() methods in particular.

@fabian-guerra
Copy link
Contributor Author

@1ec5 Giving a second thought I'm thinking the best solution for this is keep the consistency across our API by adding the proper documentation to setVisibleCoordinateBounds just like https://github.com/mapbox/mapbox-gl-native/blob/release-ios-v3.6.0-android-v5.1.0/platform/darwin/src/MGLPolyline.h#L36 in Polyline we are not responsible for extending the coordinates. Thoughts?

/cc @boundsj

@1ec5
Copy link
Contributor

1ec5 commented Sep 12, 2017

I’m not sure I follow. That documentation states that MGLPolyline correctly handles straddling the antimeridian on the map as long as you specify some coordinates beyond ±180° longitude. #9731 is about -[MGLMapView setVisibleCoordinateBounds:] fails to straddle the antimeridian and instead shows “the other side” of the world.

@fabian-guerra
Copy link
Contributor Author

What I mean is if devs also set setVisibleCoordinateBounds beyond ±180º it will work.

@1ec5
Copy link
Contributor

1ec5 commented Sep 12, 2017

Oh, I see what you mean. If setting a coordinate bounds that goes beyond ±180° already works, then that’s great. The remaining issue is whether the developer should get the same behavior by setting a “backwards” coordinate bounds in which sw.longitude > ne.longitude. As long as the ±180° approach works, it isn’t as important to address the remaining issue, at least not while #4664 is still open.

@boundsj boundsj removed this from the ios-v3.6.3 milestone Sep 18, 2017
@fabian-guerra fabian-guerra changed the base branch from release-ios-v3.6.0-android-v5.1.0 to release-agua October 10, 2017 20:53
@fabian-guerra
Copy link
Contributor Author

I added the documentation comments to clarify how to use bounds that cross the anti-meridian. We already support extending longitudes beyond ±180°.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This is a good change to make, but I don’t think we can say that it fixes #9731 or #4664 per se. Rather, those issues would be addressed like this: the setter would add 360 to ne.longitude if sw.longitude > ne.longitude.

@@ -668,12 +668,24 @@ MGL_EXPORT IB_DESIGNABLE
Changing the value of this property updates the receiver immediately. If you
want to animate the change, call `-setVisibleCoordinateBounds:animated:`
instead.

To make the visible bounds go across the antimeridian or international date line,
specify some longitudes less than −180 degrees or greater than 180 degrees.
Copy link
Contributor

Choose a reason for hiding this comment

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

This documentation is for a getter, not a setter, so it should talk about what it means to see a longitude beyond ±180°:

If a longitude is less than −180 degrees or greater than 180 degrees, the visible bounds straddles the antimeridian or international date line.

@fabian-guerra
Copy link
Contributor Author

@1ec5 you are right. This PR does not fix #9731 or #4664 per se. Attempts only to clarify how to handle this use case.

@fabian-guerra fabian-guerra merged commit ff62fa9 into release-agua Dec 21, 2017
@fabian-guerra fabian-guerra deleted the fabian-visible-coordinates-9731 branch December 21, 2017 20:04
@1ec5 1ec5 changed the title [ios, macos] Fix visible coordinates when boundaries cross international datelline [ios, macos] Document setting visible coordinates that cross international datelline Feb 20, 2018
@1ec5 1ec5 changed the title [ios, macos] Document setting visible coordinates that cross international datelline [ios, macos] Document setting visible coordinates that cross antimeridian Feb 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants