Implement depth-first-search to match way_ids with edge_ids #2848

merkispavel · 2021-02-09T09:29:18Z

Issue

This implementation handles the case when you might choose wrong direction more than once while matching way_ids with graph_ids. See the gurka test for better understanding

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

Requirements / Relations

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

merkispavel · 2021-02-09T09:33:33Z

test/gurka/test_route.cc

+           {"type", "restriction"},
+           {"restriction", "no_left_turn"},
+       }},
+  };


So we have FECB restriction and ABCD edges have the same way_id.
Let's say we're starting with the B node and trying to match [3, 2, 1] way_ids.

Step 1: we go towards node A as way_id(BA) == 3. But this is wrong direction so we fallback to node B

Step 2: we go towards node D as way_id(BCD) == 3. BUt this is wrong direction as well.

Step 3: The GetGraphIds function return no match found but it must return BCEF

merkispavel · 2021-02-09T09:42:55Z

I built tiles with florida.osm.pbf from master and this branch and got mismatch in restriction counts. Need to check that new restrictions are legal and not produced by a bug

2021/02/08 19:09:46.507802 [INFO] Adding Restrictions at level 2
2021/02/08 19:10:05.361094 [INFO] --Forward restrictions added: 512
2021/02/08 19:10:05.361302 [INFO] --Reverse restrictions added: 345
2021/02/08 19:10:05.361928 [INFO] Adding Restrictions at level 1
2021/02/08 19:10:08.703185 [INFO] --Forward restrictions added: 1078/1088 (+10)
2021/02/08 19:10:08.703238 [INFO] --Reverse restrictions added: 1213/1223 (+10)
2021/02/08 19:10:08.703524 [INFO] Adding Restrictions at level 0
2021/02/08 19:10:10.306843 [INFO] --Forward restrictions added: 1033/1037 (+4)
2021/02/08 19:10:10.307888 [INFO] --Reverse restrictions added: 1065/1069 (+4)

merkispavel · 2021-02-09T16:19:39Z

Uffff... I came across another bug: we don't look at the access mode while matching way_ids with graph_ids

gknisely · 2021-02-09T19:43:43Z

@merkispavel is this ready for review or will you address the other issue you found

merkispavel · 2021-02-09T20:22:32Z

src/mjolnir/restrictionbuilder.cc

+        bool found;
+
+        // expand with the next way_id
+        found = ExpandFromNode(reader, lock, last_node, edge_ids, way_ids, way_id_index + 1, tile,


Calling ExpandFromNode(..., way_id_index + 1, ...) before ExpandFromNode(..., way_id_index, ...) let us not to worry about multiple edges with the last way id. So we need not remove duplicated way_ids in the prefix only

merkispavel · 2021-02-09T20:23:56Z

src/mjolnir/restrictionbuilder.cc

@@ -489,7 +424,7 @@ void build(const std::string& complex_restriction_from_file,
                res_way_ids.push_back(restriction_to.to());

                // walk in the forward direction (reverse in relation to the restriction)
-                std::deque<GraphId> tmp_ids = GetGraphIds(currentNode, reader, lock, res_way_ids);
+                std::vector<GraphId> tmp_ids = GetGraphIds(currentNode, reader, lock, res_way_ids);


I just don't see sense in using deque here. It just confused me. But I can easily revert it if there's some profit

merkispavel · 2021-02-09T20:25:49Z

test/gurka/gurka.cc

-        throw std::invalid_argument("Osm way id has already been used");
-      }
-      way_id = id;
+      way_id = std::stoull(found->second);


So I needed a gurka test when multiple edges have the same way id. And this exception used to be thrown. Let me know if there's a better way to get such gurka test(in case you want to revert this piece of code)

@merkispavel we have added exception tests for the path and eta like this here But, it you don't think it will be useful for more tests in the future, I am fine how it is now.

merkispavel · 2021-02-09T21:04:14Z

@merkispavel is this ready for review or will you address the other issue you found

@gknisely This PR doesn't break existing restrictions. What about the new restrictions - some of them are valid and some of them are not. But it's caused by another(unrelated to the current PR) bug (#2849)
So I guess it's ok to review it and merge imho

mandeepsandhu · 2021-02-09T21:42:05Z

@merkispavel have you RAD tested this? If yes, were there any diffs?

gknisely · 2021-02-10T03:47:45Z

@merkispavel is this ready for review or will you address the other issue you found

@gknisely This PR doesn't break existing restrictions. What about the new restrictions - some of them are valid and some of them are not. But it's caused by another(unrelated to the current PR) bug (#2849)
So I guess it's ok to review it and merge imho

This looks good, but can you send me a sample restriction for the above PR? #2849 I assume you saw it in the FL data set. You should be able to dump out the osmid for the restriction relation. Wondering if we really should be tossing relations if the access/modes on the ways don't match the access/modes on the relations for the restriction.

merkispavel · 2021-02-10T11:57:32Z

This looks good, but can you send me a sample restriction for the above PR? #2849 I assume you saw it in the FL data set. You should be able to dump out the osmid for the restriction relation. Wondering if we really should be tossing relations if the access/modes on the ways don't match the access/modes on the relations for the restriction.

@gknisely

https://www.openstreetmap.org/relation/9946366

merkispavel · 2021-02-10T12:09:45Z

@merkispavel have you RAD tested this? If yes, were there any diffs?

not yet @mandeepsandhu. I'll let you know RAD test status when i run it

merkispavel · 2021-02-15T18:56:09Z

Tested master vs PR diff in restrictions: lgtm

And noticed one buggy restriction https://www.openstreetmap.org/relation/9689483. I guess it should lane-specific restriction

Waiting for the rad results and we are good

merkispavel · 2021-02-16T18:05:01Z

Rad tested the changes. Looks good

Implement depth-first-search to match way_ids with edge_ids

5505587

merkispavel commented Feb 9, 2021

View reviewed changes

merkispavel added 2 commits February 9, 2021 13:36

Gurka: allow to set the same way id to multiple edges

086ef46

Small improvements: naming, anonymous namespace..

ce9d64d

merkispavel force-pushed the get-graph-ids branch from 7fe0d9b to ce9d64d Compare February 9, 2021 10:37

merkispavel added 3 commits February 9, 2021 23:11

Lint, minor renaming

041d9c8

Changelog

4efa170

Merge branch 'master' into get-graph-ids

c280375

merkispavel commented Feb 9, 2021

View reviewed changes

merkispavel marked this pull request as ready for review February 9, 2021 21:04

merkispavel requested a review from gknisely February 9, 2021 21:04

gknisely mentioned this pull request Feb 10, 2021

Honor access mode while matching OSMRestriction with graph #2849

Merged

4 tasks

merkispavel added 2 commits February 11, 2021 20:59

Bugfix: keep track of visited nodes to avoid infinite recursion

6fe4ded

Bugfix bugfix: replace != with ==

9917e45

Merge branch 'master' into get-graph-ids

2d8eef1

gknisely approved these changes Feb 16, 2021

View reviewed changes

Merge branch 'master' into get-graph-ids

837c81f

merkispavel merged commit 1cd806d into master Feb 16, 2021

merkispavel deleted the get-graph-ids branch March 9, 2021 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement depth-first-search to match way_ids with edge_ids #2848

Implement depth-first-search to match way_ids with edge_ids #2848

merkispavel commented Feb 9, 2021 •

edited

Loading

merkispavel Feb 9, 2021 •

edited

Loading

merkispavel commented Feb 9, 2021 •

edited

Loading

merkispavel commented Feb 9, 2021 •

edited

Loading

gknisely commented Feb 9, 2021

merkispavel Feb 9, 2021

merkispavel Feb 9, 2021

merkispavel Feb 9, 2021

gknisely Feb 16, 2021

merkispavel commented Feb 9, 2021

mandeepsandhu commented Feb 9, 2021

gknisely commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 15, 2021 •

edited

Loading

merkispavel commented Feb 16, 2021

Implement depth-first-search to match way_ids with edge_ids #2848

Implement depth-first-search to match way_ids with edge_ids #2848

Conversation

merkispavel commented Feb 9, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

merkispavel Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel commented Feb 9, 2021 • edited Loading

merkispavel commented Feb 9, 2021 • edited Loading

gknisely commented Feb 9, 2021

merkispavel Feb 9, 2021

Choose a reason for hiding this comment

merkispavel Feb 9, 2021

Choose a reason for hiding this comment

merkispavel Feb 9, 2021

Choose a reason for hiding this comment

gknisely Feb 16, 2021

Choose a reason for hiding this comment

merkispavel commented Feb 9, 2021

mandeepsandhu commented Feb 9, 2021

gknisely commented Feb 10, 2021 • edited Loading

merkispavel commented Feb 10, 2021 • edited Loading

merkispavel commented Feb 10, 2021 • edited Loading

merkispavel commented Feb 15, 2021 • edited Loading

merkispavel commented Feb 16, 2021

merkispavel commented Feb 9, 2021 •

edited

Loading

merkispavel Feb 9, 2021 •

edited

Loading

merkispavel commented Feb 9, 2021 •

edited

Loading

merkispavel commented Feb 9, 2021 •

edited

Loading

gknisely commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 10, 2021 •

edited

Loading

merkispavel commented Feb 15, 2021 •

edited

Loading