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

Prevent integer overflow during segment touch ratio calculation #354

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mcquay239
Copy link

I use boost/geometry clipping operations after snap rounding (to prevent robustness issues). So I have no intersecting segments in my areals, segments are intersecting with their endpoints. Integers are large (they can be about 2**30), but I use 64-bit integer coordinate type. So sides (side_info) are calculating perfectly, but when segment ratio calculation leads to integer overflow.

But we don't actually need to calculate these ratios, they are trivial.

@barendgehrels
Copy link
Collaborator

Thanks @mcquay239 !
The compilation fixes can certainly be applied and will help some regressions in the regression matrix.
The other fix can most probably be applied too - (have to spend more time on it to confirm, which I now don't have) but to be sure, did you run the unit tests?

@mcquay239
Copy link
Author

I did and I cought a regression in multiline/multiline intersection.
Unfortunately I noticed it after the pull request, because there already
were several test failures. I beleive it's something easy-to-fix and I'll
try to debug it.

вт, 26 июля 2016 г., 23:11 Barend Gehrels notifications@github.com:

Thanks @mcquay239 https://github.com/mcquay239 !
The compilation fixes can certainly be applied and will help some
regressions in the regression matrix.
The other fix can most probably be applied too - (have to spend more time
on it to confirm, which I now don't have) but to be sure, did you run the
unit tests?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#354 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAxnDP8t82irmxL2CiM1vPxRheDiy4eIks5qZmnpgaJpZM4JVb1u
.

@barendgehrels
Copy link
Collaborator

OK - thanks again. If necessary we can also split this PR

@mcquay239
Copy link
Author

I've just fixed tests. The only notice is that a linestring, consisting of an isolated point (two equal points, actually), is not simple (really?). But as I understand it's an expected behaviour.

Anyway all tests except broken earlier are passed.

@mcquay239
Copy link
Author

I don't know if it's necessary to split this PR: #353 is equivalent to b5c07c6, you can apply #353 and then think about this one.

@mcquay239
Copy link
Author

Updated PR, checked tests once more, please apply
@barendgehrels

@mcquay239 mcquay239 force-pushed the fix_cart_intersect branch 2 times, most recently from e479132 to 613020e Compare September 21, 2017 10:30
@awulkiew
Copy link
Member

awulkiew commented Jun 3, 2021

@barendgehrels Considering you've made many changes in the strategy since 2016 is this PR still relevant?

@awulkiew awulkiew added the bug label Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants