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

Fix problem of narrow angles locateOnLine #79

Merged
merged 2 commits into from
Feb 1, 2019
Merged

Conversation

LePetitTim
Copy link
Collaborator

Hello,

This pull request is link with : #32

1 : without
capture d ecran de 2019-01-31 17-27-08

2 : with
capture d ecran de 2019-01-31 17-28-06

The problem come from that when a subline is really long, the value delta/hypotenuse will be smaller than tolerance from belongsSegment for much more lines. In the example above, the point is linked with multiple sublines on the same line. But without a tolerance we will get only a random subline and then get the wrong locateOnLine value (linked with the wrong subline).

@LePetitTim LePetitTim requested a review from leplatrem February 1, 2019 08:52
Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Good catch :)

Don't forget that this library has unit tests for every method. It would be a pity to start making changes in the code without taking care of them...

@@ -540,7 +540,7 @@ L.GeometryUtil = L.extend(L.GeometryUtil || {}, {
var l1 = latlngs[i],
l2 = latlngs[i+1];
portion = lengths[i];
if (L.GeometryUtil.belongsSegment(point, l1, l2)) {
if (L.GeometryUtil.belongsSegment(point, l1, l2, 0.0001)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This value seems pretty arbitrary :) Where does it come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's arbitrary a little bit like 0.2 :) i can't put 0 because it's not possible to find the point on the line with this value, it should be because the point is on the line but i think that it's not perfectly on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha!

Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

👍

@LePetitTim LePetitTim merged commit f6326ec into master Feb 1, 2019
LePetitTim added a commit that referenced this pull request Feb 1, 2019
    * Fix `locateOnLine()` doesn't return correct subline (#79, thanks @LePetitTim)
@LePetitTim
Copy link
Collaborator Author

Who is publishing on npm the new releases @leplatrem ?

@leplatrem
Copy link
Collaborator

Who is publishing on npm the new releases @leplatrem ?

I used to do it... do you have permissions?

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