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

Revert self intersection behavior: line-intersect #2742

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pm0u
Copy link

@pm0u pm0u commented Oct 30, 2024

Purpose: Bug fix

Partially Addresses:
#2728
#2722

line-intersect

  • Sets default value of options property ignoreSelfIntersections to true
  • Adds a geojson file test case for a feature collection with a geometry with a self intersection but no intersections between geometries
  • Adds a test case for default behavior of ignoreSelfIntersections and explicitly setting to false, using same geometry from the geojson test case added.

Copy link
Member

@smallsaucepan smallsaucepan left a comment

Choose a reason for hiding this comment

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

Hi @pm0u. Added a couple of comments. The code change itself is fine. Would you mind adding some more tests to better lock down the current ignores behaviour with other feature types?

@@ -18,16 +18,13 @@ import { sweeplineIntersections as findIntersections } from "./lib/sweepline-int
* @param {GeoJSON} line1 any LineString or Polygon
* @param {GeoJSON} line2 any LineString or Polygon
* @param {Object} [options={}] Optional parameters
* @param {boolean} [options.removeDuplicates=true] remove duplicate intersections
* @param {boolean} [options.ignoreSelfIntersections=false] ignores self-intersections on input features
* @param {boolean} [options.removeDuplicates=true] remove duplicate intersections (default `true`)
Copy link
Member

Choose a reason for hiding this comment

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

No need to add (default true) - JSDoc appends that automatically. See generated README.md where it gets repeated.

@@ -132,3 +132,48 @@ test("turf-line-intersect - polygon support #586", (t) => {
t.equal(results.features.length, 1, "should return single point");
t.end();
});

test("turf-line-intersect - self intersection behavior", (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

Are there advantages to running the same features through both "in code" and as in/out fixture tests?

Would you mind adding some simple tests for MultiLineString, Polygon, and MultiPolygon as well e.g.

Screenshot 2024-11-13 at 11 04 55 PM

To be honest I'm not sure what the expected behaviour of those are. Should ignoreSelfIntersections apply to the entire MultiLineString, or only take effect within each individual LineString? If you're got the time let's take this opportunity to embed whatever the behaviour is into some baseline tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants