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

Fix infinite loop in GEOSClipByRect (#865) #110

Closed
wants to merge 3 commits into from

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Jul 28, 2018

Infinite loop was caused by floating point error in the intersection between a near-vertical or near-horizontal line segment and an edge of the rectangle. This PR corrects for the special case and adds a guard to throw an exception instead of entering an infinite loop should a similar situation arise.

@strk
Copy link
Member

strk commented Jul 30, 2018

A promise of GEOSClipByRect is that it will prefer giving a not-so-perfect result than throw an exception, are we breaking that promise with this change ?

@Algunenano
Copy link
Contributor

I've tested the Postgis code that used to loop infinitely against this branch and it's fixed; no exceptions thrown to Postgis code.

There are some differences in the tests (GEOSClipByRect seems less precise), but I understand that's the intended behaviour.

@dbaston
Copy link
Member Author

dbaston commented Jul 30, 2018

A promise of GEOSClipByRect is that it will prefer giving a not-so-perfect result than throw an exception

Yes, but the alternative of an infinite loop is clearly worse. I can modify the PR so that instead of throwing an exception, the INSIDE/OUTSIDE bits are ignored, which would also avoid the infinite loop. I'm not sure what would happen downstream of this point, though.

@strk strk closed this in de4927d Jul 31, 2018
strk pushed a commit that referenced this pull request Jul 31, 2018
strk pushed a commit that referenced this pull request Jul 31, 2018
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

3 participants