Skip to content

Conversation

@barendgehrels
Copy link
Collaborator

This happens in rare cases. When they arrive there (or leave), their fraction should be 1 (or 0). The fraction is calculated and is sometimes nearly 1 (or ), but not considered so.
This can be fixed without any threshold because the information on arrival is already there.

It fixes ~10% of the remaining errors in the recursive_polygons_buffer test.

Several cases (added last PR) are fixed now. And I added 3 new cases (not yet fixed by this)

// Not yet fully tested for float and long double.
// The difference algorithm can generate (additional) slivers
BoostGeometryWriteExpectedFailures(10, 11, 24, 14);
BoostGeometryWriteExpectedFailures(10, 11, 24, 15);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this was already)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that there is no extra error here?

Copy link
Collaborator Author

@barendgehrels barendgehrels Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, these statements are mainly for myself, during transition of removing rescaling. This is one for long double. I probably didn't update it correctly last time. It's not updated automatically.
For this PR, I commented the code (updating the fraction) and it was still there - so it's not this PR for sure.

Main purpose is to see if we get there. For set operations, the version without rescaling is (nearly) on par, or better. For buffer it's still not good enough so I'm concentrating on those cases.

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks OK to me.

// Not yet fully tested for float and long double.
// The difference algorithm can generate (additional) slivers
BoostGeometryWriteExpectedFailures(10, 11, 24, 14);
BoostGeometryWriteExpectedFailures(10, 11, 24, 15);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that there is no extra error here?

@awulkiew awulkiew added the bug label Nov 18, 2020
@awulkiew
Copy link
Member

@barendgehrels Do you want it to be released with 1.75?

@barendgehrels
Copy link
Collaborator Author

@barendgehrels Do you want it to be released with 1.75?

Hi @awulkiew , no, it's not necessary - only for without rescaling, but that's currently not the default.

@awulkiew awulkiew added this to the 1.76 milestone Nov 25, 2020
{
ti.method = method;

// Intersection points are calculated on both segments, either one can be taken.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was wrong, I'll fix it and merge. Thanks for your reviews

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✔️

Then their fractions should be 1 or 0 (and not nearly so)
@barendgehrels barendgehrels force-pushed the fix/fraction-by-arrival branch from 2dcaa2d to 42bd7cf Compare November 25, 2020 09:22
@barendgehrels barendgehrels merged commit bc77b48 into boostorg:develop Nov 25, 2020
@barendgehrels barendgehrels deleted the fix/fraction-by-arrival branch November 25, 2020 09:24
@awulkiew awulkiew modified the milestones: 1.76, 1.75 Nov 25, 2020
@awulkiew
Copy link
Member

@barendgehrels since you merged it now I'll push it together with other fixes to master for 1.75.

@barendgehrels
Copy link
Collaborator Author

@barendgehrels since you merged it now I'll push it together with other fixes to master for 1.75.

@awulkiew , fine, thank you!

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.

3 participants