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

https://github.com/Turfjs/turf/issues/2707 https://github.com/Turfjs/turf/issues/2700 https://github.com/Turfjs/turf/issues/2633 https://github.com/Turfjs/turf/issues/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 https://github.com/Turfjs/turf/discussions/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: https://github.com/Turfjs/turf/pull/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:

• [x] Change default arg values for line-intersect
• [x] Update tests for line-intersect
• [x] Change default arg values for boolean-disjoint
• [x] Update tests for boolean-disjoint
• [x] Change default arg values for boolean-intersects
• [x] 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?

[pm0u]

hi @smallsaucepan - i have 2 new PRs that should finish addressing this issue.

https://github.com/Turfjs/turf/pull/2772 https://github.com/Turfjs/turf/pull/2773

they seem to have a failing test for directional-mean but that also seems to be happening on master as well for me.

Curious what if you think this last one is worth addressing?

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.

what do you think makes sense? would you prefer this left explicit or should this get cleaned up?

[smallsaucepan]

Thanks heaps @pm0u. Getting those last two merged now. That will allow us to close off the four issues linked in the description above, yes?

r.e. line-split, let's leave it explicit for now - feel it's a form of self documentation.

[pm0u]

afaik, yes - I can take a look tomorrow and see if there are any easy test cases included in those issues that we can compare against these updates just to confirm for certain

[smallsaucepan]

Cool. I'll proceed optimistically and mark those four as resolved once #2773 is merged.

Aiming to do a release on the 26 or 27th with these fixes, so if you find anything untoward in the meantime let me know.

[pm0u]

ok - will there be a pre release available as an alpha on NPM? i am actually thinking through how i can test these, i think at this point the only way i could is locally with a local build of turf. unless you have any ideas?

[smallsaucepan]

Not planning on an alpha, as we're keen to get the behaviour reverted to what it should be.

If you want to capture specific examples from issues, you can add them to test.ts in the relevant package. Either just run them locally for added peace of mind, or commit them for posterity (will need to be a separate PR).

Did this recently over on #2729 if you want a couple of examples.

If you want to there's also publishing your development copy to a local NPM registry - https://github.com/Turfjs/turf/wiki/Contributing#deploy-to-a-local-node-registry

It's handy if you're trouble shooting packaging issues, though maybe overkill for what you have in mind.