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 validity rules for polygons in locations.geojson #476

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

leonardehrenfried
Copy link
Contributor

@leonardehrenfried leonardehrenfried commented Jun 13, 2024

Problem

When you have self-intersecting polygons in locations.geojson, there can be situations where routing engines cannot decide if a point is inside or outside of it:

image

Yesterday I asked in Slack about validity rules of flex polygons and received positive responses about tightening them.

For this reason, I'm opening this PR to gather more feedback and eventually call a vote.

Rather than spelling out all the ways in which polygons can be invalid or just mentioning self-intersecting ones, I opted instead to reference another standard that has been battle hardened and has a very good description of of validity rules for polygons.

image

cc @miles-grant-ibigroup @m-mcqueen @LeoFrachet

@skinkie
Copy link
Contributor

skinkie commented Jun 13, 2024

+1 I don't even think this should be a vote.

@westontrillium
Copy link
Contributor

(Copying my Slack comments here)

I would support adding this kind of restriction. Feeds with these polygons are in effect already “invalid” if they have impossible polygons. This wouldn’t be so much introducing a backwards compatibility-breaking restriction as it would be codifying a requirement that already exists implicitly. It makes sense to make it explicit so that it can be easily enforced since there are already several examples of these kind of errors out in the wild.

I was not previously familiar with the OpenGIS Simple Features Specification, but it seems to account for everything we need (including valid multipolygons). I encourage interested parties to do a more thorough review of that document before casting a vote for due diligence's sake. I'll be particularly looking for any cases where it would impose unwanted restrictions excluding use cases we've actually encountered (unlikely, but again, due diligence 🙂).

@leonardehrenfried
Copy link
Contributor Author

👍

You can start your due diligence in the GeoJSON RFC which refers to this spec in its introduction and says it implements its data model.

It's also the data model for the very popular software packages PostGIS and Java Topology Suite (JTS). It's where names like Linestring, Polygon, MultiPolygon come from.

@LeoFrachet
Copy link
Contributor

Thanks @leonardehrenfried for working on that and coming with some great reference. 🎉

Looks great on my side!

@eliasmbd eliasmbd added GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule Extension: GTFS-Flex Issues and Pull Requests that focus on GTFS-Flex Extension Status: Discussion Issues and Pull Requests that are currently being discussed and reviewed by the community. Change: Clarification Revisions of the current specification to improve understanding. labels Jun 27, 2024
@westontrillium
Copy link
Contributor

Do we need anymore discussion on this proposal? Should a vote be opened?

@leonardehrenfried
Copy link
Contributor Author

A vote can be opened but I'm currently on holiday. @tzujenchanmbd @eliasmbd Could you be so kind and start the formalities of the voting process?

@eliasmbd
Copy link
Collaborator

eliasmbd commented Aug 28, 2024

As per request of @leonardehrenfried, I open the vote for this proposal

The vote requirements are as follow:

  • Vote lasts the minimum period sufficient to cover 14 full calendar days. Vote ends at 23:59:59 UTC.
  • Anyone is allowed to vote yes/no in a form of comment to the pull request, and votes can be changed until the end of the voting period. If a voter changes her vote, it is recommended to do it by updating the original vote comment by striking through the vote and writing the new vote.
  • Votes before the start of the voting period are not considered.
  • The proposal is accepted if there is a unanimous consensus yes with at least 3 votes

The voting period ends on Wednesday, September 11, 2024 at 23:59:59 UTC.

Happy voting!

@westontrillium
Copy link
Contributor

+1 Trillium

@evansiroky
Copy link
Contributor

+1 Caltrans

@gcamp
Copy link
Contributor

gcamp commented Aug 28, 2024

+1 Transit

@eliasmbd eliasmbd added Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md and removed Status: Discussion Issues and Pull Requests that are currently being discussed and reviewed by the community. labels Aug 29, 2024
@leonardehrenfried
Copy link
Contributor Author

@miles-grant-ibigroup @LeoFrachet You indicated approval before the voting started but would you be so kind as to give your unambiguous +1?

@LeoFrachet
Copy link
Contributor

With pleasure!

+1 from Spare.

@miles-grant-ibigroup
Copy link

+1 Arcadis

@PodarisEng
Copy link

+1 Podaris

@eliasmbd
Copy link
Collaborator

The voting period has ended on Wednesday, September 11, 2024 at 23:59:59 UTC.

The result is:
+1 Trillium
+1 Caltrans
+1 Transit
+1 Spare
+1 Arcadis
+1 Podaris

@leonardehrenfried Congratulations the vote has passed with 6 votes for and 0 against.

@tzujenchanmbd tzujenchanmbd merged commit f4b7964 into google:master Sep 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change: Clarification Revisions of the current specification to improve understanding. Extension: GTFS-Flex Issues and Pull Requests that focus on GTFS-Flex Extension GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants