-
Notifications
You must be signed in to change notification settings - Fork 963
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/shortest-path
#956
Conversation
Hey @stebogit There a bunch of known algorithms for this sort of thing. For example try googling "dijkstra's algorithm", I think this is a semi-famous one that might help. Might be easier and more robust to follow a known pattern. |
👍 Love it! @stebogit 🎩 off to you for trying to implement this directly with only TurfJS modules.
Agreed, would be neat to have a comparable to see if @stebogit's implementation is faster & accurate to those other algorithms. I'm 👍 for any direction we decide to go to, as long as it's fast and accurate. |
packages/turf-shortest-path/index.js
Outdated
if (getType(start) !== 'Point') throw new Error('start must be Point'); | ||
if (!end) throw new Error('end point is required'); | ||
if (getType(end) !== 'Point') throw new Error('end must be Point'); | ||
if (!obstacles) throw new Error('obstacles collection is required'); |
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 possibly have obstacles as an optional parameter? In case you want to use this module over an iterative process and maybe in some scenarios you would have 2 points with or without an obstacle, we could simply just return a lineString from both points (same as an empty featureCollection
).
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.
Yes! Great idea 👍
packages/turf-shortest-path/index.js
Outdated
collection.features.push(start); | ||
collection.features.push(end); | ||
|
||
var box = bbox(scale(bboxPolygon(bbox(collection)), 1.15)); // extend 15% |
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.
Couldn't extend
also be an optional parameter instead of hardcoded as 15%?
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.
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.
Fair enough... that is a pretty silly scenario 😝
@@ -0,0 +1,1981 @@ | |||
{ |
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've recently just been removing/excluding the package-lock.json
files, I'm not too sure if we should be having two lock
files (yarn + npm).
I've added package-lock.json
to the .gitignore
, we could always add it back, but it seems like two lock files is a bit overkill (we only need to stick with one).
"grid-to-matrix": "1.3.0", | ||
"javascript-astar": "0.4.1", | ||
"pathfinding": "0.4.18" | ||
} |
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.
There's definitely missing some dependencies in the package.json
, this is one of the reasons why the TravisCI tests will fail.
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.
All this is absolutely "work-in-progress", I added the whole @turf/turf
for convenience while developing.
Sorry, I should have titled this more clearly as "proposal" (like the label); I was just interested at this point in you suggestions and check with you if my approach was reasonably sound so far.
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 great to me so far! Was just trying to get the tests to pass
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.
Really awesome! Looking forward to add more tests to this, overall this looks like a great library.
A few concerns I mentioned in the comments, but looks great to me! 👍
@stebogit Do you have an ESLint plugin installed in your current text editor? TravisCI Error
|
@turf/shortest-path
@turf/shortest-path
(work-in-progress)
@rowanwins thanks for the tip. Thanks guys! |
good luck with it @stebogit ! I find it really hard reading some of those maths/computer science papers and interpreting what they're saying, but if we can follow them they should lead us to decent performant implementations, and I think they teach us some good skills in queues and sweeplines etc that we should be able to apply elsewhere. |
I like this image: |
@stebogit Other possible module name would be |
@turf/shortest-path
(work-in-progress)@turf/shortest-path
(⚠️ Work in progress %80)
I don't know if you guys would consider but shortest sea routes may be killer, either with reverse altitude or based on depth colors on maps. here is an example based on color. |
Hi @bulutkartal Are you able to elaborate on your idea, are you saying that altitude or depth have a bearing on the path calculation? This module is for the actual calculation of the shortest path, it doesn't take into account how to visualise the result. |
I was trying to mean the reserve altitude can be used to calculate shortest sea routes(only water based), but using elevation services may be quite expensive and I don't think it is very suitable for turf. But instead of elevation services , map background colors can be used to calculate shortest sea routes. Lines will not pass through the land, only will stay in water colors. The link I sent above as an example works that way. It calculates the shortest distance between two ports and the line script creates stays in water. I saw the approach when I opened their bundled js. It was kinda cool what they did. This wouldn't be beneficial for land routes. I just saw the example was created at sea and idea just came out and found that link :) |
Hi @bulutkartal Using the proposed module you will be able to pass in your start and end point (presumably ports), as well as a bunch of polygons (which represent land borders), the output line will show you the route between the points avoiding the land. So it wont have anything to do with elevation. I hope that helps clear things up. |
using elevation was just an another idea to prevent line going through land since elevation will be negative in water. Was just an idea, nothing more :) But using colors are easier and faster approach. |
@stebogit Agreed, the 2nd map looks like it would be a better solution. 👍 to push this at the next minor release |
@turf/shortest-path
(⚠️ Work in progress %80)@turf/shortest-path
🎉 Merged & Published |
Ref. #788
There is a lot of room for improvements, but what do you think of this first draft @DenisCarriere @rowanwins ? Any suggestion to improve speed and/or final result is more than welcome.