-
Notifications
You must be signed in to change notification settings - Fork 944
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
New module @turf/nearest-point-to-line
#939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @stebogit , it's taken a long time between drinks on this module.
The only other thought is what we if we have 2 points at exactly the same distance? Could we pass back an array of points?
* @name nearestPointToLine | ||
* @param {FeatureCollection<Point>} points collection | ||
* @param {Feature<LineString>} line Feature | ||
* @param {string} [units=kilometers] can be degrees, radians, miles, or kilometers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param {string} [units=kilometers] can be degrees, radians, miles, or kilometer
My only comment would be saying that this is for the output property. So perhaps
@param {string} [units=kilometers] unit of the output distance property, can be degrees, radians, miles, or kilometer
} | ||
}); | ||
|
||
pt.properties.dist = dist; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call the output property distance
(rather than dist
) because it's slightly more explicit/obvious.
Perhaps this should be an upstream change in @turf/pointOnLine
as well...
Just my personal preference
@rowanwins I found some issues with (I think) the |
@rowanwins @DenisCarriere https://gist.github.com/anonymous/491d71542965d44b07f19a14b20a94d5?short_path=000ca83 https://gist.github.com/anonymous/dae55177159c3275f141b2accb96d1f4?short_path=5a48b33 |
I wouldn't worry too much about the 0.000011m difference, we can definitely add them to the test cases and look into this later.
I don't think we should return an Array of points (or FeatureCollection of Points), the expected output would be a single point and the chances that a point would fall exactly within a 0.000001 distance would be very slim. Any points which fall exactly on the same distance would simply be ignored and the first closest point would be returned.
We've been using |
/// <reference types="geojson" /> | ||
|
||
type Points = GeoJSON.FeatureCollection<GeoJSON.Point>; | ||
type LineString = GeoJSON.LineString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only allow Geometry LineString and not a Feature<LineString>
:
Correct definition for both Geometry & Feature support:
type LineString = GeoJSON.Feature<GeoJSON.LineString> | GeoJSON.LineString;
* | ||
* @name nearestPointToLine | ||
* @param {FeatureCollection<Point>} points collection | ||
* @param {Feature<LineString>} line Feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also support LineString as a Geometry?
/**
* @param {Feature<LineString>|LineString} line Line Feature
/** | ||
* http://turfjs.org/docs/#nearestpointtoline | ||
*/ | ||
declare function nearestPointToLine(points: Points, line: LineString): Point; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing units
as an optional param
* The returned point has a `dist` property indicating its distance to the line. | ||
* | ||
* @name nearestPointToLine | ||
* @param {FeatureCollection<Point>} points Point Collection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also support GeometryCollection
Updated a few things:
CC: @stebogit |
Yay!! 🎉 |
Ref #198
Not sure though how to interpret this discussion:
I don't see the case of parallel lines here @morganherlocker and @rowanwins.