Fix degenerate linestring relation on boundary of areal

[mjacobse]

Basically makes it so covered_by returns true for LINESTRING(0 0,0 0) and POLYGON((0 0, 0 2, 2 2, 2 0, 0 0)) instead of false as it did previously, which could be quite surprising.

Fixes #1473

The bit flags for interrupt aren't exactly pretty, but it follows the present implementation.

[barendgehrels]

Should we handle pig == 0 here too?

[mjacobse]

A degenerate linestring does not have a boundary itself, so this case shouldn't be possible I believe.

[barendgehrels]

All right

[barendgehrels]

Thanks for the PR, is it a bit larger than earlier? Anyway, that's not a problem, will continue the review later

[mjacobse]

Thanks for the PR, is it a bit larger than earlier? Anyway, that's not a problem, will continue the review later

Adding the handling of the interrupt flags for this case now made it a bit bigger yes

[tinko92]

Thank you for the PR. The changes look suitable to me for but the goal of this PR is but I have not yet run the full test suite locally.

I have two general questions on this approach:

1. Have you considered your approach against an implementation on the dispatch level, i.e. before the dispatch to the detail/relate/linear_areal code test the linestring for degeneracy and call the detail/relate/point_geometry code in case of degeneracy? This way may be easier and and more clear to wrap into a macro guard to allow restoring the old behaviour.
2. Related to this (and mentioned in another code comment) since we change behavior here, do we want a macro that can be defined to restore the old behavior?

[tinko92]

This is not so easy to review. Something like

private:
constexpr static unsigned flag_interior_interior = 0x01;
// ...
constexpr static unsigned flag_interior_boundary = 0x10;
constexpr static unsigned flag_all = flag_interior_interior | /* ... */ | flag_interior_boundary;
// ...
m_interrupt_flags |= flag_interior_boundary;

may be more clear.

[mjacobse]

I agree, I was just following what was there before, trying to keep the change set small. I am happy to follow the suggestion to make it more readable if desired

[tinko92]

Do we want some BOOST_GEOMETRY_... guard macro around this? Since this changes behavior we may want to include in the next changelog after this gets pulled as breaking, and if a user (possibly unknowingly since past issue reports suggest the geometric predicate names can be non-intuitive to some) relies on the old behaviour, they could define it.

[barendgehrels]

I would think that we don't need macro's for changed behaviour on degenerate linestrings.
But we should add it to the release notes.

[barendgehrels]

2. Related to this (and mentioned in another code comment) since we change behavior here, do we want a macro that can be defined to restore the old behavior?

I'm not sure if this is necessary for this fix.

[mjacobse]

1. Have you considered your approach against an implementation on the dispatch level, i.e. before the dispatch to the detail/relate/linear_areal code test the linestring for degeneracy and call the detail/relate/point_geometry code in case of degeneracy? This way may be easier and and more clear to wrap into a macro guard to allow restoring the old behaviour.

My concern with that was that checking the linestring for degeneracy didn't seem completely trivial. Checking boost::geometry::is_valid for failure_wrong_topological_dimension would have been my first approach which would be O(n) I guess? Probably negligible compared to what follows, but still some non-trivial extra code only to make some arguably out-of-contract behavior less surprising. On the other hand the present approach basically just handles a natural third case in an already present if statement. Seemed simpler, but I'd be happy to explore the dispatch level option if that's preferred.