Bug: bidirerctionalastar ignores complex restriction #2781

merkispavel · 2021-01-19T13:24:55Z

Issue

Bug details: #2781 (comment)
the PR has both a gurka test which fails on master and the suggested fix

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

Requirements / Relations

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

merkispavel · 2021-01-19T13:25:59Z

test/gurka/test_route.cc

+
+  try {
+    auto result = gurka::route(map, "D", "E", "auto");
+    gurka::assert::raw::expect_path(result, {"Unexpected path found"});


It returns DC-CB-BE

From my view the bug is around this line

valhalla/src/thor/bidirectional_astar.cc

Line 764 in ff25b11

if (rev_pred.on_complex_rest()) {

rev_pred == DC and rev_pred.on_complex_restr() returns false

I guess the problem is in restrictionbuilder.cc.

Let's say we have a restriction A -> B -> C. So we take the edge AB and it's marked as start_restriction. We go forward and reach the node C. We go backwards and finally have the edges {CB, BA}. After that we add reverse restriction using the BA edge as from. BUT we add it to the tile of the AB edge.
tldr: if nodes A and B belong to different tiles (Houston) we got a problem

the good news is this should be incredibly rare! but also wow, good find, what a needle in a haystack. i suppose we have to detect when restrictions cross tile boundaries in the restrictions builder and patch it to handle that case.

the good news is this should be incredibly rare

agreed!

merkispavel · 2021-01-20T19:00:55Z

src/mjolnir/restrictionbuilder.cc

@@ -331,8 +329,7 @@ void build(const std::string& complex_restriction_from_file,
            if (restriction.type() < RestrictionType::kOnlyRightTurn ||
                restriction.type() > RestrictionType::kOnlyStraightOn) {

-              GraphId currentNode = GraphId(tile->id().tileid(), tile->id().level(), i);
-              GraphId tileid = tile->id();
+              GraphId currentNode = directededge.endnode();


Turned out that both forward and it's opposite edge are marked with start_restriction flag.
So let's say we have a restriction A - B - C. Both AB & BA are start_restriction-s.
Before this change:

algorithm starts with the node A and succeeds when directededge == AB

After this change

algorithm starts with the node A and succeeds when directededge == BA

so the PR doesn't breaks anything

merkispavel · 2021-01-22T14:58:20Z

Going to build large enough area to compare the number of restrictions against master to make sure the data is static

merkispavel · 2021-01-26T09:45:12Z

Going to build large enough area to compare the number of restrictions against master to make sure the data is static

Did a test with florida-latest.osm.pbf. Suprisingly I got 1 more restriction with my changes. Investigating 👀 ..

UPD: the mismatched restriction https://www.openstreetmap.org/relation/7680229#map=18/25.74885/-80.21806

master

2021/01/25 14:07:30.819573 [INFO] Adding Restrictions at level 2
2021/01/25 14:07:42.640216 [INFO] --Forward restrictions added: 534
2021/01/25 14:07:42.640241 [INFO] --Reverse restrictions added: 345
2021/01/25 14:07:42.640766 [INFO] Adding Restrictions at level 1
2021/01/25 14:07:44.755671 [INFO] --Forward restrictions added: 1079
2021/01/25 14:07:44.755694 [INFO] --Reverse restrictions added: 1214
2021/01/25 14:07:44.755969 [INFO] Adding Restrictions at level 0
2021/01/25 14:07:46.014482 [INFO] --Forward restrictions added: 1033
2021/01/25 14:07:46.014505 [INFO] --Reverse restrictions added: 1065

this branch

2021/01/25 14:05:33.527092 [INFO] Adding Restrictions at level 2
2021/01/25 14:05:46.330749 [INFO] --Forward restrictions added: 535 // vs 534 in master
2021/01/25 14:05:46.330792 [INFO] --Reverse restrictions added: 345
2021/01/25 14:05:46.331898 [INFO] Adding Restrictions at level 1
2021/01/25 14:05:48.692780 [INFO] --Forward restrictions added: 1079
2021/01/25 14:05:48.692811 [INFO] --Reverse restrictions added: 1214
2021/01/25 14:05:48.693068 [INFO] Adding Restrictions at level 0
2021/01/25 14:05:50.047939 [INFO] --Forward restrictions added: 1033
2021/01/25 14:05:50.047961 [INFO] --Reverse restrictions added: 1065

merkispavel · 2021-01-26T21:02:11Z

src/mjolnir/restrictionbuilder.cc


-                  if (tmp_ids.size()) {
+                  if (tmp_ids.size() && tmp_ids.back().Tile_Base() == tile_id) {


it happens if we split an osmway into a few edges and not all edges go to the same tile
E.g. if you have a restriction "from ABC to CD"

A -- B -- C | D

there are cases that A-B-C has the same osmway property. And if the edges AB and BC are from the different tiles then master code produces redundant restrictions: they are harmless but useless: both AB & BC edges are marked as start restrictions but finally you'll get the only restriction "from (BC) to (CD)"

merkispavel · 2021-01-26T21:04:27Z

Going to build large enough area to compare the number of restrictions against master to make sure the data is static

Did a test with florida-latest.osm.pbf. Suprisingly I got 1 more restriction with my changes. Investigating 👀 ..

UPD: the mismatched restriction https://www.openstreetmap.org/relation/7680229#map=18/25.74885/-80.21806

master

2021/01/25 14:07:30.819573 [INFO] Adding Restrictions at level 2
2021/01/25 14:07:42.640216 [INFO] --Forward restrictions added: 534
2021/01/25 14:07:42.640241 [INFO] --Reverse restrictions added: 345
2021/01/25 14:07:42.640766 [INFO] Adding Restrictions at level 1
2021/01/25 14:07:44.755671 [INFO] --Forward restrictions added: 1079
2021/01/25 14:07:44.755694 [INFO] --Reverse restrictions added: 1214
2021/01/25 14:07:44.755969 [INFO] Adding Restrictions at level 0
2021/01/25 14:07:46.014482 [INFO] --Forward restrictions added: 1033
2021/01/25 14:07:46.014505 [INFO] --Reverse restrictions added: 1065

this branch

2021/01/25 14:05:33.527092 [INFO] Adding Restrictions at level 2
2021/01/25 14:05:46.330749 [INFO] --Forward restrictions added: 535 // vs 534 in master
2021/01/25 14:05:46.330792 [INFO] --Reverse restrictions added: 345
2021/01/25 14:05:46.331898 [INFO] Adding Restrictions at level 1
2021/01/25 14:05:48.692780 [INFO] --Forward restrictions added: 1079
2021/01/25 14:05:48.692811 [INFO] --Reverse restrictions added: 1214
2021/01/25 14:05:48.693068 [INFO] Adding Restrictions at level 0
2021/01/25 14:05:50.047939 [INFO] --Forward restrictions added: 1033
2021/01/25 14:05:50.047961 [INFO] --Reverse restrictions added: 1065

Everything's fine. The master code used to loose a restriction. This PR fixes it. Ready for review

purew

LGTM

kevinkreiser · 2021-01-27T15:51:00Z

CHANGELOG.md

@@ -130,6 +130,7 @@
   * FIXED: Remove redundant return statements [#2776](https://github.com/valhalla/valhalla/pull/2776)
   * FIXED: Add support for geos-3.9 c++ api [#2739](https://github.com/valhalla/valhalla/issues/2739)
   * FIXED: Fix check for live speed validness [#2797](https://github.com/valhalla/valhalla/pull/2797)
+   * FIXED: Store restrictions in the right tile [#2781](https://github.com/valhalla/valhalla/pull/2781)


we made a release please move this to the correct section above

bug: bdirastar ignores complex restriction

f837ff5

merkispavel commented Jan 19, 2021

View reviewed changes

purew self-requested a review January 19, 2021 15:48

merkispavel added 7 commits January 20, 2021 18:02

Use directedededge endnode as start node

4136318

Small cleanup: get rid of tileid parameter

4579949

Merge branch 'master' into bdir-ignores-restriction

32497c3

Update changelog

ba8f76f

Ignore cmakelists.txt.user

8b1b0d7

lint

5445d10

Merge branch 'master' into bdir-ignores-restriction

5636538

merkispavel force-pushed the bdir-ignores-restriction branch from fb1b19c to 5636538 Compare January 20, 2021 18:10

Remove unused variable after cleanup

11ff496

merkispavel commented Jan 20, 2021

View reviewed changes

merkispavel requested review from kevinkreiser and gknisely January 20, 2021 19:46

Merge branch 'master' into bdir-ignores-restriction

e40d4d3

Check that we save restriction to the right tile

0a0a963

merkispavel self-assigned this Jan 26, 2021

Merge branch 'master' into bdir-ignores-restriction

320285e

merkispavel commented Jan 26, 2021

View reviewed changes

Fixup after merging the master: use new gurka api

dfb826f

purew previously approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into bdir-ignores-restriction

f9bfc33

kevinkreiser reviewed Jan 27, 2021

View reviewed changes

Update changelog

1b9dd75

merkispavel dismissed purew’s stale review via 1b9dd75 January 27, 2021 15:58

merkispavel requested a review from purew January 27, 2021 15:59

merkispavel requested a review from kevinkreiser January 27, 2021 15:59

purew approved these changes Jan 27, 2021

View reviewed changes

merkispavel merged commit 1577f88 into master Jan 27, 2021

merkispavel deleted the bdir-ignores-restriction branch January 27, 2021 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: bidirerctionalastar ignores complex restriction #2781

Bug: bidirerctionalastar ignores complex restriction #2781

merkispavel commented Jan 19, 2021 •

edited

Loading

merkispavel Jan 19, 2021

merkispavel Jan 19, 2021

merkispavel Jan 20, 2021

kevinkreiser Jan 20, 2021

merkispavel Jan 20, 2021 •

edited

Loading

merkispavel Jan 20, 2021 •

edited

Loading

merkispavel commented Jan 22, 2021

merkispavel commented Jan 26, 2021 •

edited

Loading

merkispavel Jan 26, 2021 •

edited

Loading

merkispavel commented Jan 26, 2021 •

edited

Loading

purew left a comment

kevinkreiser Jan 27, 2021


		if (tmp_ids.size()) {
		if (tmp_ids.size() && tmp_ids.back().Tile_Base() == tile_id) {

Bug: bidirerctionalastar ignores complex restriction #2781

Bug: bidirerctionalastar ignores complex restriction #2781

Conversation

merkispavel commented Jan 19, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

merkispavel Jan 19, 2021

Choose a reason for hiding this comment

merkispavel Jan 19, 2021

Choose a reason for hiding this comment

merkispavel Jan 20, 2021

Choose a reason for hiding this comment

kevinkreiser Jan 20, 2021

Choose a reason for hiding this comment

merkispavel Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel commented Jan 22, 2021

merkispavel commented Jan 26, 2021 • edited Loading

merkispavel Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel commented Jan 26, 2021 • edited Loading

purew left a comment

Choose a reason for hiding this comment

kevinkreiser Jan 27, 2021

Choose a reason for hiding this comment

merkispavel commented Jan 19, 2021 •

edited

Loading

merkispavel Jan 20, 2021 •

edited

Loading

merkispavel Jan 20, 2021 •

edited

Loading

merkispavel commented Jan 26, 2021 •

edited

Loading

merkispavel Jan 26, 2021 •

edited

Loading

merkispavel commented Jan 26, 2021 •

edited

Loading