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

Rewrite line-intersect module #2033

Merged
merged 28 commits into from
Apr 2, 2022
Merged

Rewrite line-intersect module #2033

merged 28 commits into from
Apr 2, 2022

Conversation

rowanwins
Copy link
Member

@rowanwins rowanwins commented Feb 13, 2021

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

This PR provides an overhaul of the line-intersect module primarily to make it more performant for larger geometries. The previous implementation used a fairly brute force approach while this PR make uses a sweepline.

I did have to comment out one test for handling GeometryCollection as I couldn't make typescript happy.

The benchmark shows the improvement, the only exception is the first case which i only a single segment.

// CURRENT
2-vertex-segment x 4,123,821 ops/sec ±12.92% (74 runs sampled)
double-intersect x 53,118 ops/sec ±4.22% (72 runs sampled)
multi-linestring x 16,417 ops/sec ±2.31% (77 runs sampled)
polygons-with-holes x 9,739 ops/sec ±2.55% (85 runs sampled)
same-coordinates x 51,303 ops/sec ±4.23% (71 runs sampled)

// PROPOSED
2-vertex-segment x 1,467,485 ops/sec ±1.74% (89 runs sampled)
double-intersect x 307,665 ops/sec ±1.70% (91 runs sampled)
multi-linestring x 81,337 ops/sec ±0.67% (94 runs sampled)
polygons-with-holes x 27,711 ops/sec ±0.70% (92 runs sampled)
same-coordinates x 521,277 ops/sec ±0.75% (92 runs sampled)

Resolves #2024
Resolves #2029

packages/turf-line-intersect/index.ts Outdated Show resolved Hide resolved
packages/turf-line-intersect/test.js Show resolved Hide resolved
packages/turf-line-intersect/index.ts Outdated Show resolved Hide resolved
@rowanwins rowanwins requested a review from mfedderly June 17, 2021 11:57
@mfedderly
Copy link
Collaborator

Any chance you can change this back to your npm sweeplines-intersections package and get it to pass the build that way like with point-in-polygon-hao? The lack of licensing header and babeled code breaking eslint makes me feel bad.

@mfedderly
Copy link
Collaborator

@rowanwins I think the build will pass if you go back to depending on your npm package now that we started targeting es2017? We should also add that package name to https://github.com/Turfjs/turf/blob/master/README.md#browser-support

* some cleanup

* clarify Intersection type
Copy link
Collaborator

@twelch twelch left a comment

Choose a reason for hiding this comment

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

I have one comment to remove geojson-rbush dep if no longer needed. approving otherwise.

packages/turf-line-intersect/package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@mfedderly mfedderly left a comment

Choose a reason for hiding this comment

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

This looks awesome. Can you publish the typescript defs in the package itself? I made two PRs to accomplish this:
rowanwins/sweepline-intersections#12
#2274

@mfedderly
Copy link
Collaborator

I'm going to go ahead and merge this

@mfedderly mfedderly merged commit b3076e7 into master Apr 2, 2022
@mfedderly mfedderly deleted the line-intersect-update branch April 2, 2022 16:37
@matthiasfeist
Copy link

Hi quick question as this PR is merged but it's not released yet to npm. When and how is that gonna happen?

@diogormsf
Copy link

diogormsf commented Nov 4, 2022

Hi quick question as this PR is merged but it's not released yet to npm. When and how is that gonna happen?

@matthiasfeist +1 on your comment. I would love to see this being merged to npm, it would allow me to remove an intersection algorithm created as a workaround.

@pm0u
Copy link
Contributor

pm0u commented Sep 24, 2024

I believe this PR introduced a breaking change that means that self intersections are checked by default where previously they were not checked at all / did not have the option to be checked.

#2707
#2722

I believe this has further sweeping changes than just booleanIntersects. For instance, booleanCrosses does not call out this option in its implementation but I think it would be expected that if checking if 2 geometries cross, a self intersection is not a helpful result.

var doLinesIntersect = lineIntersect(lineString1, lineString2);

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