-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Isochrone Plugin v2 #3652
Isochrone Plugin v2 #3652
Conversation
022c931
to
5018ef2
Compare
… This commit is the first step, the second step is a reverse search, still to come.
…to speed things up.
… step is to calculate a convex hull.
… todo: unpack geometry and backtrack, and calculate concave hull.
Include example viewer for isochrones (just point clouds so far).
…edges in the search radius, not sure why yet.
TIMER_START(TABLE_TIMER); | ||
std::sort(phantoms.begin(), phantoms.end(), [](const auto &a, const auto &b) { | ||
const auto &a_id = | ||
a.forward_segment_id.enabled ? a.forward_segment_id.id : a.reverse_segment_id.id; |
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.
Sadly the node ID is not a proxy for contraction level in our CH implementation: We keep the node ID constant starting in the EdgeBasedGraphFactory
, because we need to save the ID in the RTree which happens before contraction.
The way you could fix this would be to load the .osrm.level
file which contains exactly that (we use it for speeding up the contraction on successive contractions).
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.
Welp, that probably explains the missing edges in the test isochrones I've been making.
I'll see if I can get correct results using the .level
file, then we can discuss how we might optionally load it.
Optional loading is also desirable for #2764 (comment), so I'm feeling like there's a pattern here.
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.
Interestingly, the PHAST paper here: https://www.microsoft.com/en-us/research/wp-content/uploads/2010/09/phastTR.pdf
states:
The second subphase scans all vertices in G↓ in descending rank order.*
- For correctness, any reverse topological order will do.
I think I see a way to do that kind of sorting, I'll give it a go.
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.
@danpat I'm curious about what you have in mind for the sorting, would you care to share some details ?
is it posible to set a max speed like this isochrone server is able to? |
Update to align with changes in various interfaces: BasePlugin, DataFacade, PhantomNode, many-to-many search algorithm and others used by the Isochrone plug-in (submitted in Project-OSRM#3652) Note, max_weight extra argument added to many-to-many algorithm routines in the original implementation has not been ported as it appears to be unused.
Here in This allows to build Isochrone plugin with GCC 7 and VS2017. TODO: I'd appreciate if someone reviewed the merge and checked if it didn't loose any crucial parts. |
@mloskot Have you had any success fixing the issue with QueryHeap? |
@adomas-mazeikis Not yet, but the issue seems MSVC-specific annoyance and iterator debug levels interfering with testing execution of the plugin. I have asked for advice on boost-users: [heap] Singular handle issue with MSVC iterator debug facilities and copied to https://stackoverflow.com/q/45608426/151641
SOLVED: Initialisation of reference from bit fields. C++ allows const-reference (via temporary trick). Apparently, MSVC does not allow it (see restrictions on bit fields). If I run the plugin, I get (Soon, I'll be gone offline for 1-3 weeks, I will continue as soon as I'm back.) |
Update to align with changes in various interfaces: BasePlugin, DataFacade, PhantomNode, many-to-many search algorithm and others used by the Isochrone plug-in (submitted in Project-OSRM#3652) Note, max_weight extra argument added to many-to-many algorithm routines in the original implementation has not been ported as it appears to be unused.
{ | ||
|
||
// const auto range = parameters.range; | ||
const auto range = 300; |
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.
Is range
intentionally signed int
?
Compare with unsigned IsochroneParameters::range declaration.
The type difference actually does affect the isochrones result (see my next #3652 (comment)).
const EdgeWeight to_weight = weight + edge_weight; | ||
|
||
// No need to search further than our maximum radius | ||
if (weight > range * 10) |
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.
TL;TR: Is it intentional here to mismatch signed/unsigned types and rely on signed int
to unsigned int
implicit conversion (eg. to filter nodes with negative weights)?
In my previous #3652 (comment) above I asked about signed/unsigned mismatch of range
.
This weight > range * 10
test is where the difference matters:
weight
issigned int
- if
range
isunsigned int
(eg. pulled asconst auto range = parameters.range;
earlier)
then result of comparison test is different than, I assume, expected.
For example, if weight
value is negative, it is implicitly converted to unsigned int
, according to the usual arithmetic conversions pattern (C++/5/9), and overflow flips the result loosing some nodes.
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.
Never ascribe to cleverness that which can be explained by oversight.
I probably just didn't notice :-)
Note that this plugin has a serious problem with the reverse graph exploration currently - see the comments by @TheMarex above.
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.
@danpat for a moment, I assumed the obscure aims for actual utilisation of the C++ dark fu to benefit :-) One of those "failed by auto
" trips. I will correct in my 5.10/master-based branches of the plugin.
I'm exploring the plugin and trying to get over some basic bumps first.
const auto &a_id = | ||
a.forward_segment_id.enabled ? a.forward_segment_id.id : a.reverse_segment_id.id; | ||
const auto &b_id = | ||
b.forward_segment_id.enabled ? b.forward_segment_id.id : b.reverse_segment_id.id; |
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 initialisation of (const) reference from bit field is not allowed by MSVC (including a program flipping over with access violation at run-time).
It's a grey area. While C++ std does not allow to initialise pointer and non-const reference from a bit field, it does allow it for const references, but via a trick: binding a new copy-constructed temporary. Technically, there is no gain in defining auto&
vs plain auto
value here.
BTW, I have greped through the whole codebase looking for similar references assigned with SegmentID
members and I didn't found any apart from these in isochrone.cpp
. Looks good.
@adomas-mazeikis |
Update to align with changes in various interfaces: BasePlugin, DataFacade, PhantomNode, many-to-many search algorithm and others used by the Isochrone plug-in (submitted in Project-OSRM#3652) Note, max_weight extra argument added to many-to-many algorithm routines in the original implementation has not been ported as it appears to be unused.
Are there any updates on this project? I played with it a little. Some weird issues if anyone else uses this PR:
Thanks for your work though, it's very interesting. |
@ CraigglesO
I'm not aware of any. I stopped evaluating this PR in Aug 2017 This PR is far behind the current master (which, n.b., moves foward with light speed) and it would be nice to rebase it, so it's easier to try it out again. I may give it a go some time during next weeks.
What do you mean exactly? What map? Setup how?
Current master no longer requires STXXL, it's an optional dependency now. Again, rebase would be nice. |
Hi all, any news about this plugin? |
On 2 February 2018 at 16:56, Emanuele Notarnicola ***@***.***> wrote:
Hi all, any news about this plugin?
How about reading the very recent comments from 2-3 days ago?
|
LMAO. hey man. Thanks for the response. Sorry for not replying sooner. Got sidetracked and github is pretty gnarly about not showing old notifications. for setup inside osrm-backend I mean that if you run Good to hear about stxxl though. Thanks for your work its inspiring. I wrote an implementation that uses a 1080ti GTX and it takes about 1s for 15min car, 7s for 30min car and about 22-30s for 1hr car. However, if your not in a city, 1hr car takes less then a second BAHAHAA. It's kind of funny. I was having a lot of trouble wrapping my head around all of OSRM so I wrote a basic r-tree for all the nodes and edges and keep all of them on RAM over storage for now. LOTS of improvement to go. |
I would really, really like to see this functionality implemented. It seems multiple individuals have tried to create this plugin, and it was never accepted. Right now, I'm generating isochrones using a grid and durations table. This is not ideal, either in terms of efficiency or accuracy. With the grid, there is always a trade-off between accuracy and calculation time. Taking 8 minutes to generate isochrones for a 1000x1000 grid is just too long, when using this R-Tree method would seem to be much more efficient and effective. Yet, where is it after all this time? I've searched and searched this repository and documentation. Is this being implemented or planned for this project? How can the efforts of those who have spent a lot of time on this already be rewarded? |
Sadly, this was a side project for me, and I have not had time to revisit it. If you really need isochrones, I second @mloskot suggestion of using Valhalla. Another option, which uses the same approach as Valhalla, is https://github.com/mapbox/mapbox-isochrone by @peterqliu - it uses the table/matrix plugin to sample, and calculates a concave polygon around the points (Valhalla does this internally too). This script could be modified to hit an OSRM instance if you like, instead of the Mapbox Matrix API. This PR was about generating a precise isochrone. Maybe I'll get back to it one day.... |
closing stale PR. Reopen if still relevant. |
This PR adds an
isochrone
plugin, which can retrieve travel-time isochrones from the OSRM routing graph.This is an alternative implementation to #2477 that does not need to load the node-based-graph into memory - instead, it derives a temporary edge-based-graph from the CH. It's not super fast, but it works :-)
It functions as follows:
range
x MAX_SPEED_METERS_PER_SECOND (200km/h for this implementation)range
have been exploredUsage is:
A 300 second isochrone on the
car
profile takes about 350ms to return.Currently, the plugin returns a GeoJSON
FeatureCollection
for every edge-based-node geometry within therange
.TODO:
type
parameter, and support:exact
- return a custom JSON structure for the full isochrone treepoints
- return a big GeoJSONFeatureCollection
ofPoint
features with distance/duration for each point, for use with third-party topography tools, like gdal_contourpolygon
- return the boundary polygon for the isochrone (TODO: what type? convex? concave? other params?)