-
Notifications
You must be signed in to change notification settings - Fork 186
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
Loop validate()
fails to adequately detect duplicate vertices
#108
Comments
@missinglink Hello |
It's simple enough to remove duplicate vertices (although slightly slow as it's The issue with removing non-adjacent vertices is that it could invalidate the geometry, such as the case of an hourglass polygon. What I settled on is to remove adjacent duplicates like it's currently doing and raising an error for all other cases, this still requires a duplicate check operation which is also I suspect the authors preferred to assume that the geometry was validated outside the library, but included this adjacent neighbor check specifically to gracefully handle cross compatibility with specifications where the first and last point of a loop are duplicated by convention. [edit] sorry, reading the code again from desktop, the library doesn't attempt to fix geometries, it just checks them for validity. So I guess this is still something which needs to be fixed as it's possible to construct an invalid Loop where |
The bug isn't present in the C++ codebase because it has an OR condition which hasn't yet been implemented in Go: FindValidationErrorNoIndex(error) || s2shapeutil::FindSelfIntersection(index_, error) |
I could port |
There is indeed a an open TODO to implement Line 466 in 0b6e08c
|
@panmari I don't see many PRs getting merged here, I wouldn't want to commit the time unless it's actually going to be reviewed and merged. I see you are a collaborator on the repo, do you have merge access? |
Indeed, there was a period of less active maintenance, but I'm happy to say that the repository is now actively being maintained again, see recently merged PRs. Yes, I do have merge access and we are committed to reviewing and merging PRs in a timely manner. We are actively working through the backlog right now. We would definitely value your contributions and encourage you to submit your PRs. |
The current code only checks for duplicate adjacent vertices:
geo/s2/loop.go
Lines 251 to 260 in 6adc566
While the comments say:
geo/s2/loop.go
Lines 34 to 35 in 6adc566
The text was updated successfully, but these errors were encountered: