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

Explicit orientation field for polygon winding #74

Merged
merged 21 commits into from Apr 22, 2022

Conversation

felixpalmer
Copy link
Collaborator

No description provided.

@felixpalmer
Copy link
Collaborator Author

Following discussion in #46

format-specs/geoparquet.md Outdated Show resolved Hide resolved
@Jesus89
Copy link
Collaborator

Jesus89 commented Apr 21, 2022

Co-authored-by: Even Rouault <even.rouault@spatialys.com>
Copy link
Member

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Added a couple little comments, but it's looking pretty good.

format-specs/geoparquet.md Show resolved Hide resolved
The winding order of polygons follows the [GeoJSON spec](https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6). Polygon rings MUST follow the right-hand rule for orientation (counterclockwise external rings, clockwise internal rings). Traversing vertices of rings in order, the interior of the polygon is on the left.
This attribute indicates the winding order of polygon. Available values are:

- counterclockwise: the winding order of polygons follows the [GeoJSON spec](https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6). Polygon rings MUST follow the right-hand rule for orientation (counterclockwise external rings, clockwise internal rings). Traversing vertices of rings in order, the interior of the polygon is on the left.
Copy link
Member

Choose a reason for hiding this comment

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

Could be nice to explain a bit more what winding order is. If there's a good web page on it then perhaps just a link to it.

Also I feel like we should start with describing the rule, and then mention that GeoJSON does the same, but not open with 'the winding order follows geojson'.

Copy link
Contributor

Choose a reason for hiding this comment

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

then mention that GeoJSON does the same

concur

Copy link
Contributor

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for pulling this together @felixpalmer ! Greatly appreciate your effort to move quickly on this.

I tried to add some clarifying text below.

I think the big decision point at the moment is whether orientation is conditionally required if edges="spherical". At the moment, I'm loathe to rush that in as a hard requirement in the spec; I think most requirements should start as optional and graduate after time and vetting in the broader ecosystem. I think in this case we can leave it optional, encourage writers to set it, and leave it up to readers to demand that it be present (required in practice rather than in specification). That gives more time for others to weigh in that are more familiar with spherical coordinates before making it a hard requirement.

format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
The winding order of polygons follows the [GeoJSON spec](https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6). Polygon rings MUST follow the right-hand rule for orientation (counterclockwise external rings, clockwise internal rings). Traversing vertices of rings in order, the interior of the polygon is on the left.
This attribute indicates the winding order of polygon. Available values are:

- counterclockwise: the winding order of polygons follows the [GeoJSON spec](https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6). Polygon rings MUST follow the right-hand rule for orientation (counterclockwise external rings, clockwise internal rings). Traversing vertices of rings in order, the interior of the polygon is on the left.
Copy link
Contributor

Choose a reason for hiding this comment

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

then mention that GeoJSON does the same

concur

felixpalmer and others added 4 commits April 22, 2022 16:46
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
felixpalmer and others added 4 commits April 22, 2022 17:08
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Copy link
Member

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Ok, I put my latest changes in. Not sure if I represented the spherical case totally right, but hopefully either get a quick review on that or we can revisit in the next release as I think it's a minor point.

alasarr
alasarr previously approved these changes Apr 22, 2022
Copy link
Collaborator

@alasarr alasarr left a comment

Choose a reason for hiding this comment

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

Thanks all the people for moving this!

@alasarr alasarr dismissed their stale review April 22, 2022 16:15

Missing stuff at readme

examples/example.py Outdated Show resolved Hide resolved
@alasarr
Copy link
Collaborator

alasarr commented Apr 22, 2022

I think we shouldn't mix 0.3 release with adding the orientation field as optional

Copy link
Contributor

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

This is looking good to me overall, thanks for the continued effort here!

I think we should be careful about making any recommendation for spherical coordinates when orientation is absent, given the discussion in #46; I think we should just point out is a a good idea to have it and problematic if absent.

format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
felixpalmer and others added 3 commits April 22, 2022 18:52
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
@cholmes cholmes merged commit e3bdb28 into main Apr 22, 2022
@jorisvandenbossche jorisvandenbossche deleted the felix/polygon-orientation branch April 23, 2022 07:40
@@ -32,6 +32,7 @@
"geometry_type": ["Polygon", "MultiPolygon"],
"crs": df.crs.to_wkt(pyproj.enums.WktVersion.WKT2_2019_SIMPLIFIED),
"edges": "planar",
"orientation": "counterclockwise",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong, as the example data is not actually using CCW rings.

The current example file is converted from a Shapefile, which has by definition an opposite winding order.

(ideally we would have multiple examples so we have both cases covered, but until then we should ensure this is correct for the current file)

@jorisvandenbossche
Copy link
Collaborator

I opened #80 with a short-term fix for the example file and some small clean-up (I think there was a left-over sentence from accepting an inline review comment)

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

7 participants