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

getCenter function for polygons #1586

Closed
wants to merge 1 commit into from
Closed

Conversation

snkashis
Copy link
Member

Putting this into a PR since it's very useful for one of my projects as well.

Implementation taken directly from @Solider-Boy's comments.

References #1196

@ghost ghost assigned mourner Apr 20, 2013
@mourner
Copy link
Member

mourner commented Apr 20, 2013

@danzel @jacobtoye @jfirebaugh @tmcw opinions? Should methods like this be present in the core?

This particular method would be useful for poly.openPopup() to open the popup from representative point instead of one of the points (like it is now). But then we would need to implement corresponding methods for Polyline and other shapes. And it's not quite clear how such a method should behave in case of MultiPolygons/MultiPolylines.

@danzel
Copy link
Member

danzel commented Apr 20, 2013

I'm a bit torn about including this. It would be nice for open popup, but most people who need this would probably want a few more geospatial functions. @snkashis what was your use case?

@snkashis
Copy link
Member Author

I sometimes need to pan the viewport to the center, or drop a label there.

@jacobtoye
Copy link
Member

We coud use this in our application. The only issue I see, is like you say the multiploygons.

@mourner
Copy link
Member

mourner commented Apr 30, 2013

It also seems that it will return the wrong center on really big (world-scale) polygons due to Mercator distortion. Should we calculate the center from projected points? If we do, the center would be correct geometrically, but then getCenter wouldn't work for polygons not yet added to the map.

@tmcw
Copy link
Contributor

tmcw commented Jun 24, 2013

I'm mostly 👎 on including this; this kind of geometric op should be in a separate library that does it well and handles the different kinds of centroids people expect (ref mapnik in which there's both centroids, centers, along-lines, and an 'inner' mode that tries to guarantee the point will be within a surface)

@mourner
Copy link
Member

mourner commented Jun 30, 2013

@tmcw yeah, the only reason I'm still considering this is for a nicer polygon.openPopup(). We would need a complementary algorithm for polylines though. Not sure if this relatively little amount of code worth it or not.

@mourner
Copy link
Member

mourner commented Oct 15, 2013

@tmcw did you change your mind on this, now that you saw a practical use for it in mapbox/geojson.io#90?

@tmcw
Copy link
Contributor

tmcw commented Oct 15, 2013

Don't let my opinion be a blocker here; this might be useful enough per-line-of-code to justify it for everyone else. In the geojson.io situation, it's still kind of a halfway situation because there are many other complex situations, like multipolygons with empty centers, multipoints, etc.

@mourner
Copy link
Member

mourner commented Oct 15, 2013

For multipolygons, I'm thinking of taking the centroid of a part with the largest area as the best centroid. Most real-world multipolys are one large polygon with some small ones, so it works quite well.

Here's my branch with the same implementation as this pull but a bit cleaner: 88ad667

There's another problem of MultiPolygon not having openPopup, but I think I'd better refactor MultiPoly code into one multi-part Path before merging the branch.

@mourner mourner closed this Oct 15, 2013
@mourner
Copy link
Member

mourner commented Oct 15, 2013

BTW, for multipoints, we can use a similar approach — just pick one of the points as centroid. Maybe even the one that's the closest to the center of the multipoint bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants