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

Classify polygon rings to distinguish inner polygon rings and multipolygons #45

Merged
merged 1 commit into from
May 18, 2016

Conversation

jfirebaugh
Copy link
Contributor

This is a non-semver-major reimplementation of #33 that applies ring classification only in toGeoJSON. The structure of the return value of loadGeometry() is unchanged.

This is needed for mapbox/mapbox-gl-js#2586.

However, I can't get a GeoJSON Polygon with inner ring to round trip through a mapnik created vector tile. As the failing test here shows, it comes out with ring order indicating a multipolygon. cc @flippmoke @springmeyer @mourner.

@mourner
Copy link
Member

mourner commented May 18, 2016

What's the reason to keep it non-semver-major now that we have earcut in master + VT2? We could merge #33 instead and then eliminate duplicated classifyRings code from gl-js.

@jfirebaugh jfirebaugh force-pushed the backward-compatible-classify-rings branch from 2545ae5 to c16b5c0 Compare May 18, 2016 18:40
@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented May 18, 2016

mapbox-gl-js depends now on vt-pbf, which would need a coordinated semver major release. Plus there are other things I'd want to consider for a semver major release (#34, #35), and I don't want to take them on right now. I'm already several levels down in a yak shave.

@jfirebaugh jfirebaugh merged commit c16b5c0 into master May 18, 2016
@jfirebaugh jfirebaugh deleted the backward-compatible-classify-rings branch May 18, 2016 18:51
@mourner
Copy link
Member

mourner commented May 18, 2016

@jfirebaugh yeah, sounds good.

@springmeyer
Copy link
Contributor

However, I can't get a GeoJSON Polygon with inner ring to round trip through a mapnik created vector tile. As the failing test here shows, it comes out with ring order indicating a multipolygon. cc @flippmoke @springmeyer @mourner.

Per chat confirmed this is not longer an issue.

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

3 participants