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 section on winding order #59

Merged
merged 3 commits into from Apr 19, 2022

Conversation

felixpalmer
Copy link
Collaborator

No description provided.

@alasarr
Copy link
Collaborator

alasarr commented Apr 11, 2022

Related to #46

@alasarr alasarr requested a review from cholmes April 11, 2022 15:48
@cholmes
Copy link
Member

cholmes commented Apr 11, 2022

I'm out of my depth here, don't think I've ever dug into winding order. Hoping @tschaub or @hobu sounds in on #46 from the geojson perspective.

1 similar comment
@cholmes
Copy link
Member

cholmes commented Apr 11, 2022

I'm out of my depth here, don't think I've ever dug into winding order. Hoping @tschaub or @hobu sounds in on #46 from the geojson perspective.

@Jesus89
Copy link
Collaborator

Jesus89 commented Apr 13, 2022

From this #46 (comment), this covers the planar edges case, but not the spherical edges case. I would make this explicit in the spec. Regarding the spherical case, we can leave it undefined or with an explicit definition too.

@alasarr
Copy link
Collaborator

alasarr commented Apr 13, 2022

From this #46 (comment), this covers the planar edges case, but not the spherical edges case. I would make this explicit in the spec. Regarding the spherical case, we can leave it undefined or with an explicit definition too.

I think @felixpalmer proposes to force this winding order for both cases

@cholmes
Copy link
Member

cholmes commented Apr 13, 2022

I think @felixpalmer proposes to force this winding order for both cases

Yes, but I think we also need to specify the orientation rule. It probably makes sense to follow MS SQL, and Google BigQuery and 'interpret the side to the left of the line as the content of the ring'.

@jorisvandenbossche
Copy link
Collaborator

I commented on the issue for the more general discussion (#46).

But specific comment on the text in this PR: can we avoid using the term "right hand rule"? I know the GeoJSON spec uses this, but that term is very confusing, as other sources use "left hand" to specify the same .. (eg the cited bigquery and MS SQL sources)

@alasarr
Copy link
Collaborator

alasarr commented Apr 16, 2022

Thanks @jorisvandenbossche and @cholmes. @felixpalmer I think makes sense to specify that to be more explicit.

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

tschaub commented Apr 16, 2022

Late to the party, but I agree that this is good to specify. I think mentioning the "right-hand rule" is ok (as some people may know it), but that it also makes sense to add some more detail (something like "the interior of the polygon is to the left of the boundary as you traverse vertices in order").

Inevitably, consumers of the data will do orientation checks before rendering, topology operations, etc. (as the data gets passed through structures that don't keep track of or require a specific orientation), but I still think it improves things to have the format specify how polygon rings are to be interpreted.

@cholmes cholmes added this to the 0.2 milestone Apr 18, 2022
alasarr and others added 2 commits April 18, 2022 22:33
Co-authored-by: Tim Schaub <tschaub@users.noreply.github.com>
@alasarr
Copy link
Collaborator

alasarr commented Apr 18, 2022

@tschaub this is one of the latest PRs of v02.0. I've added your commit suggestion. Could you add your stamp here?

@felixpalmer
Copy link
Collaborator Author

I agree on the clarification here, it makes it more accessible to wider reader base

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