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

Primitive implementation of line intersect #1348

Merged
merged 2 commits into from
Jan 27, 2022
Merged

Conversation

OttyLab
Copy link
Contributor

@OttyLab OttyLab commented Jan 14, 2022

Summary

Implement line intersect which is equivalent to turf.lineIntersect with a primitive manner.

Discussion

  • It's O(nm) implementation as same as Turf-Swift. Please judge if this algorithm is acceptable or not as a stopgap solution.
  • The handling of line segment endpoint is different from Turf-Swift. In Turf-Swift endpoint is included while not in Turf-Java. This and this if conditions causes the difference. If they are like varA >= 0 && varA <= 1, the behavior becomes same. Which Swift/Java is the expected? (This is because the test is slightly different from Turf-Swift)

@OttyLab OttyLab force-pushed the implement-line-intersect branch 2 times, most recently from e84d36e to d7fc569 Compare January 14, 2022 08:00
Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

LGTM with one small change request

The handling of line segment endpoint is different from Turf-Swift.

Looks like TurfJS is inclusive as well so we might want to fix this up on the Java side.

Would you be able to cut a separate ticket/PR?

@@ -26,6 +26,43 @@ private TurfMisc() {
throw new AssertionError("No Instances.");
}

/**
* Takes lines {@link LineString} and returns intersect points.
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to mention time complexity and that it's not as efficient as the JS counterpart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I updated the comment

@OttyLab
Copy link
Contributor Author

OttyLab commented Jan 18, 2022

@LukasPaczos

I opened a PR which fixes lineIntersects. I'll update current PR after lineIntersects fix is merged because I want to change the test code.

@OttyLab OttyLab force-pushed the implement-line-intersect branch from 6379bb0 to 8b07285 Compare January 27, 2022 00:30
@OttyLab
Copy link
Contributor Author

OttyLab commented Jan 27, 2022

@LukasPaczos @pengdev

By following this comment, I've picked up the fix of lineIntersects in this PR and added the edge test.

Copy link
Member

@pengdev pengdev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@OttyLab OttyLab enabled auto-merge (squash) January 27, 2022 12:08
@OttyLab OttyLab disabled auto-merge January 27, 2022 12:09
@OttyLab OttyLab force-pushed the implement-line-intersect branch from 8b07285 to d49e824 Compare January 27, 2022 12:10
@OttyLab OttyLab merged commit 864dd68 into main Jan 27, 2022
@OttyLab OttyLab deleted the implement-line-intersect branch January 27, 2022 23:10
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.

3 participants