[ios,macos] multipolygon coordinate

[fabian-guerra]

Fixes #7070

[1ec5]

Thanks for this change – it’s a welcome improvement. In addition to the feedback below, -[MGLPointCollection coordinate] could also benefit from Polylabel.

Also, would you do the honors and update the iOS and macOS changelogs?

[1ec5]

In general, we should initialize simple structs like this using factory methods. This is especially important for geometry structs where it’s easy to confuse latitude and longitude or x and y. Create an MGLLocationCoordinate2DFromPoint() method similar to MGLPointFromLocationCoordinate2D() and call it here.

[1ec5]

This doesn’t account for holes in the polygon, which can affect the shape’s centroid. (We try to avoid labeling a polygon inside one of its holes.)

This class already has a -geometryObject method that returns a mbgl::Geometry<double>, but it actually creates an mbgl::MultiPolygon<double>. Consider factoring out a -multiPolygon method, similar to how -[MGLPolygon geometryObject] calls -[MGLPolygon polygon].

[1ec5]

A multipolygon can represent multiple disjoint polygons, but this code gloms them together into a single polygon geometry. I believe that would be invalid according to the GeoJSON specification.

I belatedly realized that polylabel() only accepts an mbgl::geometry::polygon, not an mbgl::geometry::multipolygon. As I pointed out in #7070 (comment), returning the centroid of an exotic shape like a multipolygon would be a low priority compared to some of the other functionality implemented here. For now, let’s just calculate the centroid of the first polygon in the shape and return that.

[1ec5]

A polygon isn’t a multilinestring.

[1ec5]

This appears to be the cause of the build failures on iOS and macOS:

▸ Compiling MGLPolyline.mm

❌  /Users/vagrant/git/platform/darwin/src/MGLPolyline.mm:60:21: no matching member function for call to 'push_back'

    multiLineString.push_back([self lineString]);
    ~~~~~~~~~~~~~~~~^~~~~~~~~



❌  /Users/vagrant/git/platform/darwin/src/MGLPolyline.mm:138:25: no matching member function for call to 'push_back'

        multiLineString.push_back([polyline lineString]);

[1ec5]

Polylabel probably isn’t the right tool for this job. In JavaScript, we’d use turf-line-distance to determine the length of the polyline, then use turf-along to get the midway point. It might be interesting to port this functionality to C or C++ for use in the SDKs, but that’s enough work that we should probably tackle it in a separate PR.

[1ec5]

Avoid excessive abbreviation: lineCoordinatesCount would be clearer.

[1ec5]

Typo: “outter”.

[1ec5]

Split this into two calls to XCTAssertEqual(), which provides a more informative message than XCTAssert() when it fails.

[1ec5]

FYI, it's perfectly acceptable to return the result of MGLLocationCoordinate2DFromPoint() directly without creating a local variable for it. If you need to verify the return value here, Xcode's debugger displays a "Return Value" entry in the Variables view when stepping out of a function.

[1ec5]

This code is incorrectly treating a polyline as if it's a polygon. Pole of inaccessibility isn't the right tool to use for calculating the midpoint of a polyline. Instead, you would need to manually determine the length of the polyline and find the point half of that distance from the start. We've already ported this functionality to Swift as part of Geometry.swift. Feel free to take care of this in a separate PR.

[1ec5]

Specifically, turf-along was ported to Swift as coordinate(at:fromStartOf:). (Note that Geometry.swift relies on this overloaded operator, which can be difficult to spot sometimes.)

[fabian-guerra]

@1ec5 could you please see the imp for this. I changed it according to your comment.

[1ec5]

Typo: longitude. But it looks like this method returns the length of the polyline, not a longitude, right? If so, let’s call this method -length and make its return type CLLocationDistance (which is in meters).

[1ec5]

Wouldn’t this be the same as making the for statement’s conditional i <= count?

[fabian-guerra]

Actually I can restrict to count - 1 since we don't need to check the last element.

[1ec5]

Turf uses 6,373,000 meters, but mbgl defines EARTH_RADIUS_M as 6,378,137 meters in constants.hpp. I’d be inclined to reuse the constant that mbgl provides.

[1ec5]

Let’s move all this new code to MGLGeometry_Private.h. MGLRadianCoordinate2D is only useful for an internal calculation, but it isn’t used in any public API. I think its presence would confuse developers who are looking for normal coordinates to use with various public APIs.

[1ec5]

-direction:to: returns a CLLocationDirection, not an MGLRadianDirection.

[fabian-guerra]

@1ec5 I moved the radian functions to MGLGeometry_Private.h and fixed minor issues, could you please take a look again.

[1ec5]

Nit: this follow-on line should be indented (also with b in MGLRadianCoordinatesDirection() below).

[1ec5]

2π/6,378,137 is many orders of magnitude smaller than the value that MGLMetersPerRadian was originanlly set to. Can you explain why we need to divide the overshoot by 2π and multiply it by the Earth’s radius? The equivalent code in Geometry.swift doesn’t seem to do that.

[fabian-guerra]

This was a paste mistake, also Gometry.swift implements it this way.

[1ec5]

coordinate(at:facing:) divides distance by metersPerRadian. That’s very different than dividing overshoot by 2π and multiplying the result by mbgl::util::EARTH_RADIUS_M.

Regardless, this line is a call to MGLRadianCoordinateAtDistanceFacingDirection(), not the implementation of MGLRadianCoordinateAtDistanceFacingDirection(). Shouldn’t it match this line instead?

[fabian-guerra]

coordinate(at:facing:) divides distance by metersPerRadian. That’s very different than dividing overshoot by 2π and multiplying the result by mbgl::util::EARTH_RADIUS_M.

Right that's why I said it was a paste mistake, it should be this way:

MGLRadianCoordinate2D otherCoordinate = MGLRadianCoordinateAtDistanceFacingDirection(from,
                                                                                                    overshoot/MGLMetersPerRadian,
                                                                                                     MGLRadiansFromDegrees(direction));

I mistakenly replaced a constant I had before called:
const double MGLMetersPerRadian = 6373000.0;

This is the underlaying impl I didn't consider I needed another function since this is the only place we are using this code.

/// Returns a coordinate a certain Haversine distance away in the given direction.
    func coordinate(at distance: CLLocationDistance, facing direction: CLLocationDirection) -> CLLocationCoordinate2D {
        let radianCoordinate = RadianCoordinate2D(self).coordinate(at: distance / metersPerRadian, facing: direction.toRadians())
        return CLLocationCoordinate2D(radianCoordinate)
    }

[1ec5]

It was probably preferable to use mbgl::util::EARTH_RADIUS_M instead of MGLMetersPerRadian, as I mentioned in #8713 (comment). The two values are within the same order of magnitude, which is a good sign. 😉

[fabian-guerra]

You are right. Changed.

[1ec5]

Do you think this is an adequate test of the added functionality? Do we need more unit tests specifically about the coordinate properties that this PR adds? Perhaps we could port some tests from Turf or from GeometryTests.swift.

[fabian-guerra]

Do you think this is an adequate test of the added functionality?

Yes... if you think I'm missing test cases I will port the tests you mention above.

[fabian-guerra]

Looking at the tests are less related to poi but I added two test scenarios to testPolyline that cover a single and multiple segments.

[1ec5]

The third parameter to this macro is a float with the same magnitude as the maximum delta, not the number of decimal places. So here we’re only testing that the center between 35.040390 and 35.040390 rounded to the ones place is 35. One degree of latitude or longitude is quite a big difference; we should assert with much higher accuracy.

[1ec5]

We should vary both the latitude and the longitude to ensure that the logic works in both directions.

[fabian-guerra]

This test case is for a straight line. I addressed testing a non linear polyline in a test case below.

[1ec5]

[1ec5]

Also note (perhaps in the “Annotations” section) that the coordinate property on MGLPolyline and MGLPolygon now returns a sensible value.

[1ec5]

We’ve already branched for iOS SDK v3.6.0: either retarget and rebase this PR on the release-ios-v3.6.0-android-v5.1.0 branch or add it to this project to be cherry-picked into that branch.

[1ec5]

Also note (perhaps in the “Annotations” section) that the coordinate property on MGLPolyline and MGLPolygon now returns a sensible value.

→ #9088