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

Loop validate() fails to adequately detect duplicate vertices #108

Open
missinglink opened this issue Sep 20, 2024 · 8 comments · May be fixed by #135
Open

Loop validate() fails to adequately detect duplicate vertices #108

missinglink opened this issue Sep 20, 2024 · 8 comments · May be fixed by #135

Comments

@missinglink
Copy link
Contributor

The current code only checks for duplicate adjacent vertices:

geo/s2/loop.go

Lines 251 to 260 in 6adc566

// Loops are not allowed to have any duplicate vertices or edge crossings.
// We split this check into two parts. First we check that no edge is
// degenerate (identical endpoints). Then we check that there are no
// intersections between non-adjacent edges (including at vertices). The
// second check needs the ShapeIndex, so it does not fall within the scope
// of this method.
for i, v := range l.vertices {
if v == l.Vertex(i+1) {
return fmt.Errorf("edge %d is degenerate (duplicate vertex)", i)
}

While the comments say:

Loops are not allowed to have any duplicate vertices (whether adjacent or not)

geo/s2/loop.go

Lines 34 to 35 in 6adc566

// Loops are not allowed to have any duplicate vertices (whether adjacent or
// not). Non-adjacent edges are not allowed to intersect, and furthermore edges

@rusneustroevkz
Copy link

@missinglink Hello
Did you solve this problem?

@missinglink
Copy link
Contributor Author

missinglink commented Nov 1, 2024

It's simple enough to remove duplicate vertices (although slightly slow as it's O(n)).

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 O(n).

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 Validate() returns nil.

@missinglink
Copy link
Contributor Author

missinglink commented Nov 1, 2024

@missinglink
Copy link
Contributor Author

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)

https://github.com/google/s2geometry/blob/ca1f3416f1b9907b6806f1be228842c9518d96df/src/s2/s2loop.cc#L190-L193

@missinglink
Copy link
Contributor Author

I could port s2shapeutil::FindSelfIntersection to Go but I doubt it would be merged, so not worth the effort:

https://github.com/google/s2geometry/blob/ca1f3416f1b9907b6806f1be228842c9518d96df/src/s2/s2shapeutil_visit_crossing_edge_pairs.cc#L454

@panmari
Copy link
Collaborator

panmari commented Mar 31, 2025

There is indeed a an open TODO to implement findSelfIntersection at

// if findSelfIntersection(p.index) {
. @missinglink your PR would be greatly appreciated!

@missinglink
Copy link
Contributor Author

@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?

@panmari
Copy link
Collaborator

panmari commented Mar 31, 2025

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.

@missinglink missinglink linked a pull request Apr 1, 2025 that will close this issue
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 a pull request may close this issue.

3 participants