Defaulting to ignoreSelfIntersections: false changes behavior of a number of Turf methods in v7

[pm0u]

Introduced in #2033

Defaulting to include self intersections in the @turf/line-intersect module has introduced breaking changes to a number of consuming modules as well as creating unexpected behaviors within those modules. Previously, I believe this algorithm did not / was not able to check for self intersections however now it does by default.

Examples

6.5.0
7.1.0 (includes sweepline module example)

Related issues

#2707
#2700
#2633
#2585

[pm0u]

While booleanIntersects (and others) can be worked around by adding an explicit { ignoreSelfIntersections: true }, some methods such as booleanCrosses do not expose this option and do not explicitly exclude self intersections themselves essentially rendering them broken (I do not believe the intention of crosses would be that when comparing two geometries, return true if one of the geometries crosses itself)

[smallsaucepan]

Thanks for pulling this together @pm0u. We're taking a look now.

[pm0u]

@smallsaucepan - thanks, happy to help with a PR if desired but I think where it stands now there are still some decisions to be made around how to handle this change before I could know where to go with that. let me know if I can help

[smallsaucepan]

Hi @pm0u. The current plan is to revert Turf to the ignore behaviour of 6.5.0 - across the board. These look to be undocumented breaking changes that we should fix as soon as possible.

Have created discussion #2728 to work through the details of a PR. Are you in a position to join in over there?

[smallsaucepan]

Hi @pm0u. Looks like discussion over at #2728 has concluded. Maintainers have agreed we should revert ignoreSelfIntersections behaviour to 6.5.0. Which I think means setting ignoreSelfIntersections default value to true wherever it appears in the current API.

Would you like to prepare a PR for this?

[pm0u]

I can work on that some this week - I am thinking some associated tests for this would be good as well. Some that include self intersections and are expected to return false for results at the very least (The other side may be useful as well?). I should be able to get the changes together but may reach out for some help on the PR as far as testing.

[pm0u]

@smallsaucepan I started in on this here: #2741

So far, that is only the boolean-disjoint module. I am wondering if it makes sense for review purposes to break this up by module for the time being and then plan to release all at once.

I am concerned that a large change like this could be difficult to review as one large PR.

Let me know your thoughts, happy to do whichever direction is deemed best.

[smallsaucepan]

A great start thank you @pm0u. Happy to go one module at a time - small PRs equal better reviews. Move #2741 out of draft mode when you're ready and we'll aim to get that reviewed and merged first.

[pm0u]

Summarizing the changes found to be needed:

• Change default arg values for line-intersect
• Update tests for line-intersect
• Change default arg values for boolean-disjoint
• Update tests for boolean-disjoint
• Change default arg values for boolean-intersects
• Update tests for boolean-intersects

Packages impacted, not requiring changes (These will resolve when dependencies are fixed):

• boolean-valid
• rectangle-grid
• boolean-crosses
• boolean-overlap

Identified packages not impacted:

• line-split - this relies on line-intersect and actually was not impacted by this change as it explicitly sets ignoreSelfIntersections: true when calling lineIntersect. It could be worth doing some "clean up" to remove the explicit setting since this is now expected to be the default or it could be desirable to be explicit about this behavior.

[pm0u]

I opened a PR for line-intersect as well. I think it would be best to start there as its the root and we can determine what other changes might still be necessary once that PR is in place #2742

[pm0u]

hey @smallsaucepan sorry to bother - I just wanted to see if there was anything more I needed to do to get #2742 ready for review?