-
Notifications
You must be signed in to change notification settings - Fork 990
Compute multiFeatureLocation in addition to location property in @turf/nearest-point-on-line #2867
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
base: master
Are you sure you want to change the base?
Compute multiFeatureLocation in addition to location property in @turf/nearest-point-on-line #2867
Conversation
|
@smallsaucepan I just resolved a merge conflict so this PR can be cleanly merged again. I would appreciate if this was included in the next release. Please let me know if anything is missing from it or holding it back. |
…f/nearest-point-on-line Fix unit tests
fb75782 to
1f0c122
Compare
|
@smallsaucepan I have again resolved any merge conflicts of this PR. It would be great if this feature could be included in the upcoming 7.3.0 release. |
|
Oh @EmilJunker! Missed the release by just a few minutes. Will review and make sure we get into the next version 👍 |
|
Have been reviewing your PR (looks good!) and I think I gave you bad advice earlier. I'm not sure any more multiFeatureLOCATION is the best choice. Other functions use distance or a variation when describing how far along a line you've gone (see along). I started wondering if any other existing properties don't make sense ...
If we could start fresh, do you think the options below would be more clear or meaningful?
Trying to establish a plan before we go further, and would appreciate your thoughts on it. |
|
Thank you for the review. I agree with you that the current naming of the properties doesn't really make sense and that this would be a good opportunity to fix that. The fact that the location describes the distance along the line while dist is the distance to the closest point really confused me when I started using the nearestPointOnLine function. I also mostly agree with your suggested new names. I also agree with you that (going by my current implementation) multiFeatureLocation and location should basically switch roles since the former is probably more useful in practice than the latter. However, I think I would prefer "pointDistance" over "ptDistance" as a new name for "dist", because I'm generally not too fond of acronyms in source code. We aren't writing C++ here 😉 . Maybe the function parameter name could also be changed from "pt" to "point" while we are at it. But turf functions seem to be a bit inconsistent in that regard, e.g. pointToPolygonDistance has a "point" param while pointToLineDistance has a "pt" param. To make it even clearer that the distance is measured along the line, we could also call the other properties "lineDistance" and "multiFeatureLineDistance". In conclusion, I think the closest point should have the following properties:
I'd be happy to make the necessary naming changes as part of this pull request. My only concern is that this would be a breaking change, thus possibly deferring the release of this feature. Can you guarantee that it would still be included in the next turf release even with this breaking change? |
This adds a
multiFeatureLocationproperty to the points returned by thenearestPointOnLinefunction. If there is just one LineString, then this will be the same as the existinglocationproperty. But if you pass a MultiLineString, this will tell you the distance along the line between the start of the MultiLineString where the closest point was found and the closest point itself. See #2753 for an example why this is useful.Closes #2753.
Please provide the following when creating a PR: