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

Add Polygon centroid and center of mass #148

Merged
merged 7 commits into from
Jul 27, 2021

Conversation

nighthawk
Copy link
Contributor

This ports Turf's centerOfMass and centroid to Swift, at least for the individual Polygon case. I've followed the same implementation and test cases as from the JS repo.

Note: This could be extended to support the other geometries that Turf.js supports, though this should then also add support for convex hull calculations.

This addresses part of #145.

@1ec5 1ec5 added improvement Improvement for an existing feature. JS parity Java parity labels Jul 19, 2021
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.

Thank you for porting the JavaScript implementation of these methods – I’m sure a lot of developers will find them quite handy. I haven’t gone line by line comparing the port to the original JavaScript, but let me know if you’d like a second pair of eyes on that.

Sources/Turf/CoreLocation.swift Outdated Show resolved Hide resolved
Sources/Turf/Geometries/Polygon.swift Outdated Show resolved Hide resolved

var signedArea: Double = 0
var sum = LocationCoordinate2D(latitude: 0, longitude: 0)
let zipped = zip(neutralized.prefix(upTo: neutralized.count - 1), neutralized.suffix(from: 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Good use of zip(_:_:)!

Sources/Turf/Geometries/Polygon.swift Show resolved Hide resolved
Sources/Turf/Geometries/Polygon.swift Show resolved Hide resolved
Sources/Turf/Geometries/Polygon.swift Show resolved Hide resolved
/// Calculates the centroid using the mean of all vertices.
/// This lessens the effect of small islands and artifacts when calculating the centroid of a set of polygons.
public func centroid() -> LocationCoordinate2D? {
let coordinates = outerRing.coordinates.dropLast()
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, once similar calculations are implemented for other geometry types, Geometry itself could have centroid and centerOfMass properties that pass through to type-specific implementations like this.

@1ec5
Copy link
Contributor

1ec5 commented Jul 22, 2021

@nighthawk, not to rush you, but feel free to push up a revised version when you’re ready, or let us know if you need help. FYI, we’re planning to release v2.0.0 in the coming weeks, but these changes are backwards-compatible, so we can land them in a subsequent minor version if the timing works out better.

@nighthawk
Copy link
Contributor Author

@1ec5 , no worries. I've pushed a revised version. Please have a look.

A little curiosity that I encountered when implementing center: Doing the manual calculation of the latitude and longitude of the bounding boxes center as commited leads to a different result that doing mid(bbox.southWest, bbox.northEast). For the Lyon example it was off by 5 metres. Is that due to the curvature of the Earth? I went with the manual calculation for consistency with Turf.js.

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.

Thank you for porting the full complement of centroid-related methods. Just to keep things simple for now, let’s roll back the changes related to the antimeridian and track that issue separately. Otherwise, this PR will be good to go. Thanks!

Sources/Turf/Geometries/Polygon.swift Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

func testPolygonCentre() {
// Adopted from https://github.com/Turfjs/turf/blob/3b20c568e5638f680cde39c26b56fbcf034133f2/packages/turf-center/test.js
let coordinate = LocationCoordinate2D(latitude: 45.7536760235992, longitude: 4.841880798339844)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the Lyon example would be more offset than expected when using mid(_:_:) because of a longstanding issue in which LocationCoordinate2D.distance(to:) uses a different approximation of the radius of the Earth than Turf.js does these days: #26.

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.

Ignore my previous review above, this is actually correct. 😅

Sources/Turf/Geometries/Polygon.swift Show resolved Hide resolved
@1ec5 1ec5 merged commit 8815a2b into mapbox:main Jul 27, 2021
@1ec5 1ec5 added this to the v2.0.0 (GA) milestone Jul 27, 2021
@nighthawk nighthawk deleted the feature/polygon-center branch September 5, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for an existing feature. Java parity JS parity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants