Fix for Sign retrieval #3208
Fix for Sign retrieval #3208
Conversation
…ng the given graph_tile_ptr. This alternate approach solves the problem without that concern.
…gs" because of a Boost header that has warnings on mac.
kevinkreiser
dismissed
their stale review
more than one way to skin a cat really, but not good ways this time
src/thor/triplegbuilder.cc
Outdated
| // GraphReader::GetGraphTile(DirectedEdge *) returns the graph-tile for | ||
| // the "end node". Hence, I need to obtain the opposing-directed-edge | ||
| // and then ask for it's graph tile. (Sigh) | ||
| GraphId opp_intersecting_de = graphtile->GetOpposingEdgeId(intersecting_de); |
There was a problem hiding this comment.
this call call is actually very dangerous and i think we should remove it have it check and throw if its the wrong tile! it assumes that the endnode of intersecting_de is in the tile you are calling it on. if intersecting_de is an edge that ends in another tile, this will get the wrong node and get the wrong opposing edge id: https://github.com/valhalla/valhalla/blob/master/valhalla/baldr/graphtile.h#L250
if the edge doesnt leave the tile it would work. also nitpick naming this with "_de" is confusing because its a graphid not a "de"
src/thor/triplegbuilder.cc
Outdated
| // the "end node". Hence, I need to obtain the opposing-directed-edge | ||
| // and then ask for it's graph tile. (Sigh) | ||
| GraphId opp_intersecting_de = graphtile->GetOpposingEdgeId(intersecting_de); | ||
| graph_tile_ptr opp_tile = graphreader.GetGraphTile(opp_intersecting_de); |
There was a problem hiding this comment.
nothing wrong here, assuming we really wanted to get the tile in which the opposing edge lives this would be just fine
src/thor/triplegbuilder.cc
Outdated
| // and then ask for it's graph tile. (Sigh) | ||
| GraphId opp_intersecting_de = graphtile->GetOpposingEdgeId(intersecting_de); | ||
| graph_tile_ptr opp_tile = graphreader.GetGraphTile(opp_intersecting_de); | ||
| size_t edge_idx = intersecting_de - opp_tile->directededge(0); |
There was a problem hiding this comment.
here we have another problem. if intersecting_de is not in the same tile as its opposing edge then this pointer arithmetic would be completely off because intersecting_de doesnt actually occur inside of opp_tiles directected edges array
src/thor/triplegbuilder.cc
Outdated
| GraphId opp_intersecting_de = graphtile->GetOpposingEdgeId(intersecting_de); | ||
| graph_tile_ptr opp_tile = graphreader.GetGraphTile(opp_intersecting_de); | ||
| size_t edge_idx = intersecting_de - opp_tile->directededge(0); | ||
| std::vector<SignInfo> edge_signs = opp_tile->GetSigns(edge_idx); |
There was a problem hiding this comment.
this is totally fine although if we have the graphid of the edge we can just call edge_id.id() to get the index into the directed edges.
Is there any chance you know more specifically where it was happening along that route? Like what OSM way are we talking about it, it was OSM data right? I think that the fix would be to collapse all those changed lines into one line: //graphtile is the tile that contains intersecting_de, so we just need to get its index to grab the signs from the tile
std::vector<SignInfo> edge_signs = graphtile->GetSigns(intersecting_de - graphtile->directededge(0));If that doesnt give you back the sign that you think should be present in your output you need to check which exact edge in the graph you expect the sign to be on and check that that sign is associated to that edge. Let me know if you want to pair up a bit on this. |
|
@kevinkreiser Hey... ugh. So I do believe I found the issue. I was passing the wrong graph-tile-ptr to The code was "working" with my changes because when I asked for the opp-intersecting-de the edge returned was hierarchy level = 1, which then retrieved the correct tile inadvertently. |
src/thor/triplegbuilder.cc
Outdated
| @@ -755,7 +753,7 @@ void AddIntersectingEdges(const AttributesController& controller, | |||
| continue; | |||
| } | |||
|
|
|||
| AddTripIntersectingEdge(controller, graphreader, start_tile, directededge, prev_de, | |||
| AddTripIntersectingEdge(controller, graphreader, endtile, directededge, prev_de, | |||
There was a problem hiding this comment.
Was incorrectly passing start_tile before... need to pass endtile.
|
Thanks to @kevinkreiser for questioning and @dgearhart and @gknisely for helping debug my error. |
I didn't realize
graphreader.GetBeginNodeId()might modify the givengraph_tile_ptr. This modified approach for retrieving Sign's solves the problem without that concern.Issue
There were some routing issues. These coordinates exposed the issue: -116.768599,36.9116110;-116.72504902777372,36.220713408337346101001
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?