indoor routing - data model, data processing #3509

liyinxiao · 2022-01-13T15:26:14Z

Issue

In this PR, we make the changes to data processing and the data model, basically to get data into the tiles. The indoor elements are routable but all of the maneuvers will be of type "DirectionsLeg_Maneuver_Type_kContinue". Maneuver and narration support (with tests) can be added in another PR.

A few key ideas:

Elevator can be modeled as both a node (more common) and a way according to OSM Wiki, and we aim to support both. Alternatively, we can process nodes to create additional/fake ways during parsing stage and only support ways (@kevinkreiser). Use an elevator_penalty to account for the time to wait.
Level is stored as a string, to account for edges crossing multiple levels, for future use cases (such as level_filter in loki).
- There are 1388 distinct values of levels for nodes, and 1380 distinct values for ways on OSM today, including fractional values such as 3.6 and invalid values such as GF. Levels don't always increment by 0.5.
Prefer level:ref over level for narration. For example, prefer "take the elevator to Ground Level" to "take the elevator to Level 1", if level:ref exists.
Add indoor_ field to check EnterBuilding vs. ExitBuilding.

Test plan

cd build
mkdir -p valhalla_tiles
./valhalla_build_config --mjolnir-tile-dir ${PWD}/valhalla_tiles --mjolnir-tile-extract ${PWD}/valhalla_tiles.tar --mjolnir-timezone ${PWD}/valhalla_tiles/timezones.sqlite --mjolnir-admin ${PWD}/valhalla_tiles/admins.sqlite > valhalla.json
./valhalla_build_tiles -c valhalla.json ~/Downloads/massachusetts-latest.osm.pbf
find valhalla_tiles | sort -n | tar cf valhalla_tiles.tar --no-recursion -T -
./valhalla_service valhalla.json 1
curl http://localhost:8002/route --data '{"locations":[{"lat":42.361145,"lon":-71.0589,"type":"break","city":"Boston"},{"lat":42.358853,"lon":-71.057298,"type":"break","city":"Boston"}],"costing":"pedestrian","directions_options":{"units":"miles"}}' | jq '.'

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

kevinkreiser

It would be great to add some gurka unit tests which prove the edges and nodes are getting parsed properly with respect to their tags and values. it doesnt have to be in depth or anything just build the data in gurka and then assert that the properties on the nodes, edges and edgeinfos are set properly. looks great so far!

liyinxiao · 2022-01-13T23:25:24Z

Thanks @dnesbitt61, @kevinkreiser for the fast feedback. I will add a few gurka tests, to make sure each indoor element gets parsed properly.

…into indoor_routing

liyinxiao · 2022-01-14T19:25:34Z

Adding gurka tests:

make run-gurka_building_entrance
make run-gurka_elevator
make run-gurka_steps
make run-gurka_escalator
make run-gurka_levels

…into indoor_routing

kevinkreiser · 2022-01-14T20:27:10Z

@liyinxiao would you mind putting all of the tests into the same test file? i think it would be slightly more tractable if they were all together in an indoor routing gurka test suite rather than in separate files. you could even build one map with all the features you want to test rather than a different map for each one. since we already have over 100 gurka tests its starting to become difficult to find a particular test 😄

liyinxiao · 2022-01-14T21:19:24Z

Good idea! Re-running gruka tests:

make run-gurka_indoor
[100%] Built target gurka_indoor
[100%] Generating gurka_indoor.log
[PASS] gurka_indoor
[100%] Built target run-gurka_indoor

review comments were addressed

kevinkreiser · 2022-01-15T03:52:51Z

@liyinxiao looks like you need to run scripts/format.sh to lint the code as well

liyinxiao · 2022-01-15T18:20:31Z

CI tests failed. I looked into it, and when I run VERBOSE=1 make -C build check or make run-scripts on my macbook it gives me this error:

yinxiaoli@yinxiaoli-mbp build % VERBOSE=1 make -C build check   
[ 23%] Generating scripts.log
LOCPATH=/Users/yinxiaoli/Desktop/tmp/valhalla/locales ASAN_OPTIONS=verify_asan_link_order=0:detect_leaks=0 /bin/bash -cx "/opt/homebrew/Frameworks/Python.framework/Versions/3.8/bin/python3.8 -m unittest discover -s /Users/yinxiaoli/Desktop/tmp/valhalla/test/scripts -v > /Users/yinxiaoli/Desktop/tmp/valhalla/build/test/scripts.log 2>&1"
+ /opt/homebrew/Frameworks/Python.framework/Versions/3.8/bin/python3.8 -m unittest discover -s /Users/yinxiaoli/Desktop/tmp/valhalla/test/scripts -v
make[3]: *** [test/scripts/scripts.log] Error 1
make[2]: *** [test/scripts/CMakeFiles/run-scripts.dir/all] Error 2
make[1]: *** [test/CMakeFiles/check.dir/rule] Error 2
make: *** [check] Error 2

This error occurs with or without my commits. Any suggestions around this error will be appreciated.

kevinkreiser · 2022-01-16T01:48:00Z

Ah yes, we semi-recently changed the python unit tests and unfortunately it seems we forgot to prints out the test logs when the test fails so you can tell what the heck is going. in this case just manually running VERBOSE=1 tells me:

LOCPATH=/home/kkreiser/sandbox/valhalla/locales python3 -m unittest discover -s /home/kkreiser/sandbox/valhalla/test/scripts -v

shows:

test_add_leaf_types (test_valhalla_build_config.TestBuildConfig) ... ok
test_nested_config (test_valhalla_build_config.TestBuildConfig)
tests if we get the values of nested dicts ... ok
test_override_config (test_valhalla_build_config.TestBuildConfig)
tests end to end if the arg parsing is working ... ok
test_get_tiles_with_bbox (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_geojson (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_graph (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_create_extracts (test_valhalla_build_extract.TestBuildExtract) ... FAIL

======================================================================
FAIL: test_create_extracts (test_valhalla_build_extract.TestBuildExtract)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kkreiser/sandbox/valhalla/test/scripts/test_valhalla_build_extract.py", line 26, in test_create_extracts
    self.check_tar(EXTRACT_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE)
  File "/home/kkreiser/sandbox/valhalla/test/scripts/test_valhalla_build_extract.py", line 36, in check_tar
    self.assertIn(t, exp_tuples)
AssertionError: (960512, 6549282, 6059792) not found in ((2560, 25568, 291912), (296448, 410441, 662496), (960512, 6549282, 6059608))

----------------------------------------------------------------------
Ran 7 tests in 0.071s

FAILED (failures=1)

this is basically saying the tile sizes have changed, which is completely expected in this case. the cool thing is that it means our canned Utrecht osm pbf data must have some ways tagged in such a way that you are parsing them out. unfortunately this makes for a bit of a brittle test. any time we change the data we'll get failures here. i think its ok though we simply update the numbers when we expect them to change.

in this case that first test was (960512, 6549282, 6059608) but should now be (960512, 6549282, 6059792) so it looks like the tile is bigger by ~200 bytes. so yeah id just upate the values in test/scripts/test_valhalla_build_extract.py

liyinxiao · 2022-01-16T14:59:16Z

Thanks, it worked! Yeah, your utrecht_netherlands.osm.pbf contains 128 OSM ways with level tag, which are going into the tiles.

test/gurka/gurka.cc

src/sif/pedestriancost.cc

proto/tripcommon.proto

…door_routing

…a which is now fixed

kevinkreiser · 2022-01-21T03:36:57Z

lua/graph.lua

+["corridor"] =          {["auto_forward"] = "false", ["truck_forward"] = "false", ["bus_forward"] = "false", ["taxi_forward"] = "false", ["moped_forward"] = "false", ["motorcycle_forward"] = "false", ["pedestrian_forward"] = "true",  ["bike_forward"] = "false"},
+["elevator"] =          {["auto_forward"] = "false", ["truck_forward"] = "false", ["bus_forward"] = "false", ["taxi_forward"] = "false", ["moped_forward"] = "false", ["motorcycle_forward"] = "false", ["pedestrian_forward"] = "true",  ["bike_forward"] = "false"}


there was just an unfortunate typo here, we needed to set pedestrian_forward rather than just pedestrian then later on we derive pedestrian_backward from forward unless the way is a one way only footway (like an escalator)

Thanks for catching this! don't know how I missed this..

kevinkreiser · 2022-01-21T03:37:24Z

test/gurka/test_indoor.cc

+        {"Cx", {{"highway", "steps"}, {"indoor", "yes"}, {"level", "1;2"}}},
+        {"xy", {{"highway", "steps"}, {"indoor", "yes"}, {"level", "2;3"}}},
+        {"yJ", {{"highway", "steps"}, {"indoor", "yes"}, {"level", "3"}}},


i added an alternate path here, you can take the steps rather than use the elevator to get to the 3rd floor

kevinkreiser · 2022-01-21T03:38:31Z

test/gurka/test_indoor.cc

+  const auto* node = graphreader.nodeinfo(node_id);
+  EXPECT_EQ(node->type(), baldr::NodeType::kBuildingEntrance);
+  EXPECT_TRUE(node->access() & baldr::kPedestrianAccess);

  node_id = gurka::findNode(graphreader, layout, "I");
+  node = graphreader.nodeinfo(node_id);
  EXPECT_EQ(graphreader.nodeinfo(node_id)->type(), baldr::NodeType::kElevator);
+  EXPECT_TRUE(node->access() & baldr::kPedestrianAccess);
 }

 TEST_F(Indoor, DirectedEdge) {
  baldr::GraphReader graphreader(map.config.get_child("mjolnir"));

-  auto directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "B", "C"));
+  const auto* directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "B", "C"));
  EXPECT_EQ(directededge->use(), baldr::Use::kSteps);
+  EXPECT_TRUE(directededge->forwardaccess() & baldr::kPedestrianAccess);
+  EXPECT_TRUE(directededge->reverseaccess() & baldr::kPedestrianAccess);

  directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "G", "H"));
  EXPECT_EQ(directededge->use(), baldr::Use::kElevator);
+  EXPECT_TRUE(directededge->forwardaccess() & baldr::kPedestrianAccess);
+  EXPECT_TRUE(directededge->reverseaccess() & baldr::kPedestrianAccess);

  directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "K", "L"));
  EXPECT_EQ(directededge->use(), baldr::Use::kEscalator);
+  EXPECT_TRUE(directededge->forwardaccess() & baldr::kPedestrianAccess);
+  EXPECT_TRUE(directededge->reverseaccess() & baldr::kPedestrianAccess);

  directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "D", "E"));
  EXPECT_EQ(directededge->indoor(), false);
+  EXPECT_TRUE(directededge->forwardaccess() & baldr::kPedestrianAccess);
+  EXPECT_TRUE(directededge->reverseaccess() & baldr::kPedestrianAccess);
+
  directededge = std::get<1>(gurka::findEdgeByNodes(graphreader, layout, "E", "F"));
  EXPECT_EQ(directededge->indoor(), true);
+  EXPECT_TRUE(directededge->forwardaccess() & baldr::kPedestrianAccess);
+  EXPECT_TRUE(directededge->reverseaccess() & baldr::kPedestrianAccess);


i added some hardening for all of these. what i did first was add teh routing test below but it failed immediately to even route at all. this made me immediately think that the access was messed up, so to check that i hardened all of these tests to see if pedestrians were even allowed on these edges/nodes. it turned out that no they weren't in some cases which was what the lua change fixed

kevinkreiser · 2022-01-21T03:39:20Z

test/gurka/test_indoor.cc

+  // first route should take the elevator node
+  auto result = gurka::do_action(valhalla::Options::route, map, {"E", "J"}, "pedestrian");
+  gurka::assert::raw::expect_path(result, {"EF", "FG", "GH", "HI", "IJ"});
+
+  // second route should take the stairs because we gave the elevator a huge penalty
+  result = gurka::do_action(valhalla::Options::route, map, {"E", "J"}, "pedestrian",
+                            {{"/costing_options/pedestrian/elevator_penalty", "3600"}});
+  gurka::assert::raw::expect_path(result, {"EF", "CF", "Cx", "xy", "yJ"});


this is pretty straightforward. first we take a route that only has a 5 second penalty to take the elevator, which is obviously cheaper than turning and then takign the steps with their penalty. then we jack up the elevator penalty so that the steps become more appealing

kevinkreiser · 2022-01-21T03:41:50Z

taginfo.json

@@ -4503,7 +4555,7 @@
      ],
      "description": "This tag contains a phonetic guide to pronouncing the official name for a country used in that country if it differs from what is in the name tag"
    },
-   {
+    {


i appreciate this clean up, thank you!

kevinkreiser

@liyinxiao thanks so much for getting the ball rolling on this awesome feature, i did a couple of documentation clean up things but this looks great and it will be great to get to the next step!

indoor routing - data model, data processing

55fd0ed

kevinkreiser requested review from kevinkreiser, dnesbitt61 and gknisely January 13, 2022 16:21

dnesbitt61 previously approved these changes Jan 13, 2022

View reviewed changes

kevinkreiser previously requested changes Jan 13, 2022

View reviewed changes

Merge branch 'master' into indoor_routing

4e6b44b

Yinxiao Li and others added 3 commits January 14, 2022 12:26

indoor routing - data model, data processing

a64d571

Merge branch 'valhalla:master' into indoor_routing

37311c1

Merge branch 'indoor_routing' of https://github.com/liyinxiao/valhalla …

633cfe4

…into indoor_routing

liyinxiao dismissed dnesbitt61’s stale review via 633cfe4 January 14, 2022 17:58

add gurka tests

9b859ad

Yinxiao Li added 2 commits January 14, 2022 14:45

add gurka tests

1c0cb8c

Merge branch 'indoor_routing' of https://github.com/liyinxiao/valhalla …

8d622da

…into indoor_routing

put gurka tests into one test file

396e6ac

kevinkreiser self-requested a review January 15, 2022 03:54

Yinxiao Li and others added 2 commits January 15, 2022 10:24

format.sh and update changelog

5f8bfbb

Merge branch 'master' into indoor_routing

2b2b49b

fix tile size in test_valhalla_build_extract

31f4ae1

Merge branch 'master' into indoor_routing

f0ebe5e

kevinkreiser reviewed Jan 19, 2022

View reviewed changes

test/gurka/gurka.cc Outdated Show resolved Hide resolved

optimize findNode in gurka test

93d9193

kevinkreiser reviewed Jan 19, 2022

View reviewed changes

src/sif/pedestriancost.cc Show resolved Hide resolved

kevinkreiser reviewed Jan 19, 2022

View reviewed changes

proto/tripcommon.proto Outdated Show resolved Hide resolved

Yinxiao Li and others added 2 commits January 19, 2022 15:55

Merge branch 'master' of https://github.com/valhalla/valhalla into in…

97d10ab

…door_routing

harden testing for indoor routing support exposed an access bug in lu…

e2a0751

…a which is now fixed

kevinkreiser reviewed Jan 21, 2022

View reviewed changes

Update CHANGELOG.md

6d04fee

kevinkreiser reviewed Jan 21, 2022

View reviewed changes

docs updates

a90e33d

kevinkreiser approved these changes Jan 21, 2022

View reviewed changes

kevinkreiser merged commit 1555d0f into valhalla:master Jan 21, 2022

liyinxiao mentioned this pull request Jan 22, 2022

indoor routing maneuvers #3519

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indoor routing - data model, data processing #3509

indoor routing - data model, data processing #3509

liyinxiao commented Jan 13, 2022 •

edited

Loading

kevinkreiser left a comment

liyinxiao commented Jan 13, 2022

liyinxiao commented Jan 14, 2022

kevinkreiser commented Jan 14, 2022 •

edited

Loading

liyinxiao commented Jan 14, 2022

kevinkreiser commented Jan 15, 2022

liyinxiao commented Jan 15, 2022

kevinkreiser commented Jan 16, 2022

liyinxiao commented Jan 16, 2022

kevinkreiser Jan 21, 2022

liyinxiao Jan 21, 2022

kevinkreiser Jan 21, 2022

kevinkreiser Jan 21, 2022

kevinkreiser Jan 21, 2022

kevinkreiser Jan 21, 2022

kevinkreiser left a comment

		["corridor"] = {["auto_forward"] = "false", ["truck_forward"] = "false", ["bus_forward"] = "false", ["taxi_forward"] = "false", ["moped_forward"] = "false", ["motorcycle_forward"] = "false", ["pedestrian_forward"] = "true", ["bike_forward"] = "false"},
		["elevator"] = {["auto_forward"] = "false", ["truck_forward"] = "false", ["bus_forward"] = "false", ["taxi_forward"] = "false", ["moped_forward"] = "false", ["motorcycle_forward"] = "false", ["pedestrian_forward"] = "true", ["bike_forward"] = "false"}

indoor routing - data model, data processing #3509

indoor routing - data model, data processing #3509

Conversation

liyinxiao commented Jan 13, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

kevinkreiser left a comment

Choose a reason for hiding this comment

liyinxiao commented Jan 13, 2022

liyinxiao commented Jan 14, 2022

kevinkreiser commented Jan 14, 2022 • edited Loading

liyinxiao commented Jan 14, 2022

kevinkreiser commented Jan 15, 2022

liyinxiao commented Jan 15, 2022

kevinkreiser commented Jan 16, 2022

liyinxiao commented Jan 16, 2022

kevinkreiser Jan 21, 2022

Choose a reason for hiding this comment

liyinxiao Jan 21, 2022

Choose a reason for hiding this comment

kevinkreiser Jan 21, 2022

Choose a reason for hiding this comment

kevinkreiser Jan 21, 2022

Choose a reason for hiding this comment

kevinkreiser Jan 21, 2022

Choose a reason for hiding this comment

kevinkreiser Jan 21, 2022

Choose a reason for hiding this comment

kevinkreiser left a comment

Choose a reason for hiding this comment

liyinxiao commented Jan 13, 2022 •

edited

Loading

kevinkreiser commented Jan 14, 2022 •

edited

Loading