Yuzhe/bug fix leg discontinuity multi routes #2049
Conversation
src/thor/map_matcher.cc
Outdated
| @@ -194,7 +193,7 @@ MapMatcher::FormPath(meili::MapMatcher* matcher, | |||
| // Check if connected to prior edge | |||
| if (prior_edge.Is_Valid() && | |||
| !matcher->graphreader().AreEdgesConnectedForward(prior_edge, edge_id)) { | |||
| disconnected_edges.emplace_back(prior_edge, edge_id); | |||
| disconnected_edges.insert({prior_edge, edge_id}); | |||
There was a problem hiding this comment.
is there actually a semantic difference here? sends rvalue ref to insert and the previous forwards the args to in place constructor.
There was a problem hiding this comment.
ah yeah derp, sorry for the confusion
|
Previous was std vector emplace_back, now changed to unordered map insert.
unordered_map emplace is not generally recommended since it creates object
first then try to look for existence.
…On Wed, Nov 13, 2019 at 8:35 AM Kevin Kreiser ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/thor/map_matcher.cc
<#2049 (comment)>:
> @@ -194,7 +193,7 @@ MapMatcher::FormPath(meili::MapMatcher* matcher,
// Check if connected to prior edge
if (prior_edge.Is_Valid() &&
!matcher->graphreader().AreEdgesConnectedForward(prior_edge, edge_id)) {
- disconnected_edges.emplace_back(prior_edge, edge_id);
+ disconnected_edges.insert({prior_edge, edge_id});
is there actually a semantic difference here? sends rvalue ref to insert
and the previous forwards the args to in place constructor.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXW7GMAZGAVVTILMMALQTQUE5A5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLN2NKY#pullrequestreview-316384939>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXSYADFLLY3COBDUGULQTQUE5ANCNFSM4JMJCCCQ>
.
|
src/thor/trace_route_action.cc
Outdated
| for (const auto& edge : m_temp_path_edges) { | ||
| m_temp_disjoint_edge_groups.back().emplace_back(edge); | ||
| if (m_temp_disconnected_edges.count(edge.edgeid) > 0) { | ||
| m_temp_disjoint_edge_groups.emplace_back(); | ||
| } | ||
| } |
There was a problem hiding this comment.
a description of what this is doing would be great for the sake of clarity. the variable names tell us a little bit but honestly not enough to really grasp what we are doing here. seems we are saying if we see the same edge id twice then we need to start a new group of edges? if that is true then i want to ask what happens when we do a map match that has a loop in the path (which is a completely reasonable thing to occur)?
valhalla/thor/map_matcher.h
Outdated
| @@ -28,7 +28,7 @@ class MapMatcher { | |||
| const std::vector<meili::EdgeSegment>& edge_segments, | |||
| const std::shared_ptr<sif::DynamicCost>* mode_costing, | |||
| const sif::TravelMode mode, | |||
| std::vector<std::pair<baldr::GraphId, baldr::GraphId>>& disconnected_edges, | |||
| std::unordered_map<baldr::GraphId, baldr::GraphId>& disconnected_edges, | |||
There was a problem hiding this comment.
this is dubious for the reason i mentioned before. once its unordered you cant know what part of the rout eyou are talking about if the route has loops
|
I have a couple of questions: Will this return multiple routes, or a single route with multiple legs? I would like to piece the separate routes/legs back into a single route and leg with a special step and maneuver over the part of the input polyline that is unmatched. That way my navigation and reroute code remains the same. (Note that I have a custom serializer that bypasses OSRM or Valhalla serializers and outputs a custom pbf). |
|
This returns multi routes in mapmatch. Change will not affect the trace
attributes logic.
…On Fri, Nov 15, 2019 at 7:56 AM David Nesbitt ***@***.***> wrote:
I have a couple of questions:
Will this return multiple routes, or a single route with multiple legs?
Is there a way to also access the portion of the input polyline where the
disconnections occur?
I would like to piece the separate routes/legs back into a single route
and leg with a special step and maneuver over the part of the input
polyline that is unmatched. That way my navigation and reroute code remains
the same. (Note that I have a custom serializer that bypasses OSRM or
Valhalla serializers and outputs a custom pbf).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXUT4EFDCJLEUJCELYDQT3BBFA5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEF3RMQ#issuecomment-554416306>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXU3OMOK7VAE6DLZJU3QT3BBFANCNFSM4JMJCCCQ>
.
|
| // TODO - perhaps also throw exception if use_timestamps and disconnected path? | ||
| if (options.action() == Options::trace_route && | ||
| (!m_temp_disconnected_edges.empty() || m_temp_path_edges.empty())) { | ||
| if (m_temp_path_edges.empty() && options.action() == Options::trace_route) { |
There was a problem hiding this comment.
we should try not to delete comments from the code. as you know this stuff is complicated. we need to keep comments in so that when the next person gets to the code its easier for them to understand what is going on. here we are throwing if you are doing trace route and we found no route at all.
|
Testing this I am seeing an output of 2 routes but the first route has 0 legs. In trace_route_action I see 2 disjoint edge groups with 6 path edges in the first and 5 in the second. The first group never triggers a call to TripLegBuilder::Build. Is this because it is looking for a Location with type == kBreak? |
|
could provide the test cases? I want to explore and play it accordingly.
Thanks!
…On Mon, Nov 18, 2019 at 8:10 AM David Nesbitt ***@***.***> wrote:
Testing this I am seeing an output of 2 routes but the first route has 0
legs. In trace_route_action I see 2 disjoint edge groups with 6 path edges
in the first and 5 in the second. The first group never triggers a call to
TripLegBuilder::Build. Is this because it is looking for a Location with
type == kBreak?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXVA2SCHTLF5D2PRSSTQUK46XA5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEK7AEY#issuecomment-555085843>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXWGLMFW4EYXWNA64E3QUK46XANCNFSM4JMJCCCQ>
.
|
|
Yes, the logic here only build legs on type == kBreak. What do you think
would be the best behavior for this? Thanks!
…On Mon, Nov 18, 2019 at 8:10 AM David Nesbitt ***@***.***> wrote:
Testing this I am seeing an output of 2 routes but the first route has 0
legs. In trace_route_action I see 2 disjoint edge groups with 6 path edges
in the first and 5 in the second. The first group never triggers a call to
TripLegBuilder::Build. Is this because it is looking for a Location with
type == kBreak?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXVA2SCHTLF5D2PRSSTQUK46XA5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEK7AEY#issuecomment-555085843>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXWGLMFW4EYXWNA64E3QUK46XANCNFSM4JMJCCCQ>
.
|
|
I was expecting each route would have 1 leg constructed from the path edges for each of the disjoint edge groups. I would think each would have a depart step and an arrive step. Even though you are not departing or arriving at a kBreak location you would be "arriving" at the point in the original input path where there is no map match, and would be "departing" at the point where match results start again. |
|
@dnesbitt61 i expected that, wherever there was a |
|
I have code working on my side where there are 2 locations (short route) with a discontinuity. I am returning 2 route, using my custom code to combine the routes into 1 route with 1 leg and (finally) adding a "false" step that is the portion of the input shape that is not matched. I'm not sure it is robust enough for all conditions, but basically once the origin_iter is advanced, I have a second loop that finds the dest_iter (advances while the iter->edgeid.Is_Valid and not a kBreak). Then I just form the origin_location, destination_location using the 2 iters, have the add_path_edge code, then call TripLegBuilder::Build (with the full path_infos). |
…inuity occurs and build legs on them accordingly
|
The test route still seems to serialize only 1 route, though I do see 2 calls to build trip legs and there are 2 routes in the trip result. Do I have to add anything to the request to get multiple routes back? The code does work properly in my custom code that uses the pbf directly. Also, what is the purpose of last_leg_index? These seem to be 0 in the test case I ran. |
|
formart:osrm will get two routes back each of them having 1 leg.
…On Wed, Nov 20, 2019 at 5:25 AM David Nesbitt ***@***.***> wrote:
The test route still seems to serialize only 1 route, though I do see 2
calls to build trip legs and there are 2 routes in the trip result. Do I
have to add anything to the request to get multiple routes back? The code
does work properly in my custom code that uses the pbf directly.
Also, what is the purpose of last_leg_index? These seem to be 0 in the
test case I ran.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXWOEIBH6VL7ZMVDKB3QUU3FNA5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEER6XZQ#issuecomment-556002278>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXS3YR33H6LYYFKSAT3QUU3FNANCNFSM4JMJCCCQ>
.
|
|
For clearity, it should be called last edge index. If you only have 1 edge
and 1 leg in the route, it should be 0.
…On Wed, Nov 20, 2019 at 5:25 AM David Nesbitt ***@***.***> wrote:
The test route still seems to serialize only 1 route, though I do see 2
calls to build trip legs and there are 2 routes in the trip result. Do I
have to add anything to the request to get multiple routes back? The code
does work properly in my custom code that uses the pbf directly.
Also, what is the purpose of last_leg_index? These seem to be 0 in the
test case I ran.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2049?email_source=notifications&email_token=AEAYSXWOEIBH6VL7ZMVDKB3QUU3FNA5CNFSM4JMJCCC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEER6XZQ#issuecomment-556002278>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAYSXS3YR33H6LYYFKSAT3QUU3FNANCNFSM4JMJCCCQ>
.
|
|
My somewhat limited testing this is working for me. I have seen it create more multiple routes properly where there are disconnects. |
Issue
This PR is targeting make valhalla to handle mapmatch with disconnected edge groups. When match results have disconnected groups, the old logic return no route. This can be an issue when user input several location with small radius. The match result may only contain one matched edge group and thus a valid route is expected as output. However, when user increase the search radius, edges that previous could not be found now could be discovered and when those edges has discontinuity, no routes will be returned. This PR solves: "Why when I increase the radius, no route will be found?".
The PR put disjoint edge groups into multi-routes.
Tasklist