Aggregation updates: update opposing local idx after aggregating the edges, added classification check for aggregation, and shortcut length changes #4570
Conversation
…s a catastrophic error - so no use continuing.
…original edge of the shortcut can lead to roundoff issues that lead to edgeinfo offset errors when adding elevation.
…ifference in elevation. No longer add each elevation along the edge.
… directed edge lengths) we need to add a roundoff to the length comparison when recovering shortcuts.
…plete. Also, added classification check due to ferry reclassification
|
co-author @dnesbitt61 |
gknisely
changed the title
| auto d = std::abs(heights[i] - heights[i + 1]); | ||
| diff = d < diff ? diff : d; | ||
| } | ||
| auto d = std::abs(heights[i] - heights[i - 1]); |
There was a problem hiding this comment.
this was missing the first height. Made this consistent with the bss check below. No need for if check anymore either.
| LOG_ERROR("GraphTileBuilder TileID: " + std::to_string(header_->graphid().tileid()) + | ||
| " offset stored in directed edge: = " + std::to_string(offset) + | ||
| " current ei offset= " + std::to_string(edge_info_offset_)); | ||
| throw std::runtime_error("EdgeInfo offsets incorrect when reading GraphTile"); |
There was a problem hiding this comment.
This is a critical error. If we see one, we likely see a bunch more throughout the tile. I think we should throw an error here.
| // Names can be different in the forward and backward direction | ||
| bool diff_names = tilebuilder.OpposingEdgeInfoDiffers(tile, directededge); | ||
|
|
||
| // Get the length from the shape. This prevents roundoff issues when forming | ||
| // elevation. | ||
| uint32_t length = valhalla::midgard::length(shape); |
There was a problem hiding this comment.
Length of shortcut is not always equal to the sum of the links of the directed edges comprising the shortcut due to directed edge length being cast to integer - thus leading to roundoff errors. This discrepancy between shortcut length (stored in the directed edge) and shape length can lead to elevation encoding issues.
src/mjolnir/elevationbuilder.cc
Outdated
| for (size_t i = 1; i < heights.size(); i++) { | ||
| auto d = std::abs(heights[i] - heights[i - 1]); | ||
| diff = d < diff ? diff : d; | ||
| LOG_INFO(" " + std::to_string(heights[i])); |
There was a problem hiding this comment.
why the change to INFO though? I remember this being crazy noisy, like 100s mb logs per minute. likely rather the function above, not the btf one, but IMO debug logging is enough for all of them
There was a problem hiding this comment.
ughh...this is a mistake. sorry.
| if (current_edge == nullptr || accumulated_length > shortcut->length()) { | ||
| ++edge_count; | ||
| roundoff_error = std::round(edge_count * 0.5) + 1; | ||
| if (current_edge == nullptr || accumulated_length > (shortcut->length() + roundoff_error)) { |
There was a problem hiding this comment.
this check may not be needed. Accumulating edge lengths (which have been truncated to int) should never lead to being greater than the shortcut length computed from the shape (and then truncated to int).
| @@ -183,7 +189,7 @@ struct shortcut_recovery_t { | |||
| } | |||
|
|
|||
| // we somehow got to the end via a shorter path | |||
| if (accumulated_length < shortcut->length()) { | |||
| if (accumulated_length < (shortcut->length() - roundoff_error)) { | |||
There was a problem hiding this comment.
but using roundoff here is definitely needed.
… aggregation_fixes
|
Is it possible to come up with a gurka test map for the roundabout/ferry cases this PR fixes? |
@nilsnolde I tried to create one for roundabout and could not make it happen as you need large number of ways and nodes. As for the ferry, I did not try and was wondering if I can add it later? |
Issue
While running through the valhalla regression suite of tests, we noticed that our tests at roundabouts were failing. This is due to the graph connectivity is broken at the roundabouts. Did not appear to be happening at all roundabouts. Issue was due to not updating the opposing local idx after aggregating the edges.
Additional issue was found with ferry reclassification. Determined that some edges were split at pedestrian edges had different classifications due to the reclassification of ferry edges (e.g., https://www.openstreetmap.org/way/204337649) Therefore, we shut off aggregation on these edges.
Also, when building shortcuts it was seen that the shortcut length was different than the sum of the lengths of the edges comprising the shortcut. This can cause issues with elevation builder which relies on the length of the edge and the length of the shape being nearly equal. When creating shortcuts, we previously simply added the lengths of each directed edge of the shortcut. Since these are stored as integer precision this can lead to roundoff errors. This PR changes the length of the shortcut to be the length of the shape rather than the sum of the lengths of the edges. This requires a change to shortcut recovery to allow some roundoff when checking shortcut length compared to sum of the edge lengths.
Tasklist