valhalla icon indicating copy to clipboard operation
valhalla copied to clipboard

GTFS Support: Convert PBFs to Transit Tiles

Open chris-jpark opened this issue 2 years ago • 1 comments

From #3669 Adding from the last PR, now with the ability to build the transit tiles from the protobufs created before. Multiple tiles can be created and stitched together, but is yet to be tested with gurka.

Edited files: mjolnir/convert_transit.cc test/gurka/test_gtfs.cc

chris-jpark avatar Aug 08 '22 12:08 chris-jpark

This pull request deals wth the parsing of protobuf files to build the Valhalla transit tiles, and a clean up of building protobufs.

While building protobufs, I made changes to ensure that the test cases simulate stitching (i.e. One transit trip spans multiple tiles), and that in that case, a pair of stops can acknowledge that one of the stops can be outside the current tile. In this case, we process the intra-stop pairs first and then proceed to 'stitch' together the missing links inter-tiles.

To meet the requirements to create transit tiles pre-set by Valhalla, there needed to be additional data added on top of GTFS's data. This includes:

  • finding the nearest OSM way_id from a given transit object, such that the transit data can be connected to the road data. This is done by projecting the transit map onto the road map, and iterating through the way_ids in the given tile, and checking the perpendicular distance of each way_id.
  • generating virtual egresses and parent stations when GTFS does not provide them.

Most of the debugging has been completed, but there still persist bugs when simulating transit routing. Currently, it thinks that the transit station is 'too far' and refuses to create the route.

chris-jpark avatar Sep 11 '22 02:09 chris-jpark

ok i made a bunch of comments. most of them are small but at least a couple will require a little bit more work. i really want to merge this but id prefer not to merge it until its a little cleaner. basically im happy to merge even if its still not working but not until the review comments are addressed

kevinkreiser avatar Sep 30 '22 11:09 kevinkreiser

ok the reason this is currently failing is because the transit graph is not connected to the regular graph. the reason for that is because the regular graph has already gone through the creation of heirarchies but the old code expected this to not have been the case. remember the old code expected transitland to just provide the right way id from the beginning but now we have to do a pass ourselves to get it. we have two options to fix this:

  1. keep using the naive algorithm which finds the closest edge by simply looking at all edges in the tile but update it to also check other levels of the heirarchy
  2. use loki to find the best candidates which requires some cmake code motion to get the dependencies right. this is the best code re-use solution but makes things complicated in that mjolnir library code would depend on loki library code. an obvious proper organization of the code has not come to me yet..

kevinkreiser avatar Dec 14 '22 15:12 kevinkreiser

Right, that's a bit painful.. Duplicating much of loki's search would be annoying, but so is depending on loki in mjolnir a bit. Would be much worse though if it'd be the other way around. We don't (yet) explicitly provide options to only build mjolnir or exclude loki in a library build, right? Are there other reasons beside removing some potential modularization and some CMake complication?

nilsnolde avatar Dec 14 '22 18:12 nilsnolde

yeah so instead of tackling using loki from inside mjolnir, i'm just keeping the naive logic that does a simple closest edge in same tile. i updated it as i mentioned above to look at other levels of the heirarchy and that seems to work though we have problems in transit builder now.

  1. if you find an edge that isnt in level 2 to try to make the connection to, it complains about making a really long transit connect edge (which makes me think it wont use it). for the time being i made the graph all residential (and put a TODO) so it gets past this bit
  2. when it tries to add stuff to the graph it breaks the tiles in such a way that we throw a nodeinfo index out of bounds error during heirarchy builder.

so yeah a few more things to iron out yet :smile:

kevinkreiser avatar Dec 15 '22 04:12 kevinkreiser

Ok so this PR is much closer than it was several months ago however its still not working and there are several problems. ive enumerated these in the pr description. I would recommend that we are ready to merge this as a work in progress that we can burn down the list above to get it into beta and released

kevinkreiser avatar Jan 30 '23 01:01 kevinkreiser

@nilsnolde @dnesbitt61 @gknisely im happy to review this with you before merging, i know its large so i'd be happy to walk you through it

kevinkreiser avatar Jan 30 '23 02:01 kevinkreiser

@kevinkreiser I think CI failing is just a tiny typo when you were hacking in some proto stuff.

nilsnolde avatar Jan 31 '23 15:01 nilsnolde

@nilsnolde yeah, as soon as we get end to end working, we can go back and delete one of the protos and the fetch transit binary. it currently doesnt really work anyway so there is no point in maintaining it after it stops being a useful "illustration" of how it used to work

kevinkreiser avatar Jan 31 '23 17:01 kevinkreiser