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

Consistent winding order for polygon rings #809

Merged
merged 3 commits into from
Jul 6, 2013
Merged

Consistent winding order for polygon rings #809

merged 3 commits into from
Jul 6, 2013

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Jun 22, 2013

In order to support efficient rendering of holes in polygons (see #803), we store rings in consistent winding order: clockwise for exterior rings and counter-clockwise for interior rings.

Different formats call for different winding order in polygon rings. If we are consistent in our internal storage, then we can serialize consistently.

KML and WKT don't specify a winding order, so we write those out in CW/CCW order (for exterior/interior).  GML references ISO 19107 that specifies CCW/CW, so we serialize in that winding order.

Having hand generated all this GML data the first time around, I reserve the right to modify it for the tests.
@tschaub
Copy link
Member Author

tschaub commented Jun 24, 2013

Thanks for any review (@bartvde, you may be interested in this).

@ahocevar
Copy link
Member

Looks great to me. Let's hope that ol.geom.LinearRing.isClockwise() is not too expensive, otherwise we might want to use it only for polygons that do have inner rings, which would add logic to the parsers. Anyway, please merge.

@elemoine
Copy link
Member

I'll look at it.

@@ -28,6 +28,42 @@ goog.inherits(ol.geom.LinearRing, ol.geom.LineString);


/**
* Determine of a vertex array representing a linear ring is in clockwise
Copy link
Member

Choose a reason for hiding this comment

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

s/of/if ?

@elemoine
Copy link
Member

elemoine commented Jul 4, 2013

I just noticed what I think is a typo. Otherwise looks good to me.

@ahocevar
Copy link
Member

ahocevar commented Jul 4, 2013

I'm also looking at this again right now. It seems to me that we could wrap

var coordinates = [];
coordinates.push(...);
var geom = new ol.geom.Polygon([coordinates]);

in a ring start - add - finalize workflow and calculate the edge value while building the ring. I'll send a pull request against @tschaub's clockwise branch to show what I mean.

@tschaub
Copy link
Member Author

tschaub commented Jul 5, 2013

@ahocevar would you be opposed to having this merged and then issuing a separate pull request for the change you're envisioning?

There will be a number of places where the ring coordinates are assembled (when reading in parsers) and not all will be pushing individual vertex coordinates to a ring's coordinate array. I think it would make sense to consider your proposed change separate from this.

It would be great to allow the rendering work to proceed (see #803).

@ahocevar
Copy link
Member

ahocevar commented Jul 5, 2013

Sounds good @tschaub. Please merge (it seems to me I said something like this already above 😄).

tschaub added a commit that referenced this pull request Jul 6, 2013
Consistent winding order for polygon rings.
@tschaub tschaub merged commit 087b4d0 into openlayers:master Jul 6, 2013
@tschaub tschaub deleted the clockwise branch July 6, 2013 00:04
@bartvde
Copy link
Member

bartvde commented Apr 10, 2015

@tschaub can this FIXME be removed or is there still something TODO?

https://github.com/openlayers/ol3/blob/master/src/ol/format/geojsonformat.js#L1

@tschaub
Copy link
Member Author

tschaub commented Apr 13, 2015

@tschaub can this FIXME be removed or is there still something TODO?

https://github.com/openlayers/ol3/blob/master/src/ol/format/geojsonformat.js#L1

See #3544.

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.

4 participants