ignore destonly edges around ferries #4118
Conversation
…ity to not un-tag destonly edges further away from ferries, i.e. where there was a break to destonly continuity
| constexpr uint32_t kFerryUpClass = static_cast<uint32_t>(baldr::RoadClass::kPrimary); | ||
|
|
There was a problem hiding this comment.
the road class was already a constant, but was passed through the whole function chain only to end up where it's needed. rather make it a real constanct
| // Reclassify ferry connection edges - use the highway classification cutoff | ||
| baldr::RoadClass rc = baldr::RoadClass::kPrimary; | ||
| for (auto& level : TileHierarchy::levels()) { | ||
| if (level.name == "highway") { | ||
| rc = level.importance; | ||
| } | ||
| } |
There was a problem hiding this comment.
this was unnecessary, replaced by a constant in ferry_connections.h
| float penalty = | ||
| (first_edge_destonly ? (!pred_destonly && w.destination_only()) : w.destination_only()) | ||
| ? 300 | ||
| : 0; |
There was a problem hiding this comment.
add a penalty only if destonly should be removed AND the pred was not destonly, or if the connecting edge wasn't even destonly
|
note to self: eventual goal should be to remove all flags from edges which ended up being reclassified, regardless what the connecting edge did. rather remove too many (i.e. legitimate) than too little (i.e. huge detour). |
…only while reclassifying if the connecting edge was dest_only AND the pred was destonly, but we still untag ANY destonly if it showed up in the final upclassified path; the main change is: also remove the connecting edge's destonly, which #1905 was missing; also fix a bug in forming the upclassified path where the initial label index could be wrong
| @@ -26,7 +26,7 @@ DirectedEdgeBuilder::DirectedEdgeBuilder(const OSMWay& way, | |||
| const bool minor, | |||
| const uint32_t restrictions, | |||
| const uint32_t bike_network, | |||
| const bool reclass_ferry) | |||
| const bool remove_destonly) | |||
There was a problem hiding this comment.
the whole change in this file is only renaming the arg and update the comment, no logic changed
| while ((overall_access_after != baldr::kVehicularAccess) && | ||
| (overall_access_before != overall_access_after)) { |
There was a problem hiding this comment.
tiny optimization: if we actually found all modes in the first expansion, we can stop. before it would do another round, only to find the exact same path.
| @@ -89,13 +91,15 @@ uint32_t ShortestPath(const uint32_t start_node_idx, | |||
| // is reached | |||
| uint32_t n = 0; | |||
| uint32_t label_idx = 0; | |||
| uint32_t last_label_idx = 0; | |||
There was a problem hiding this comment.
we use this to initialize the label when forming the path
| @@ -196,8 +206,9 @@ uint32_t ShortestPath(const uint32_t start_node_idx, | |||
| uint16_t path_access = baldr::kVehicularAccess; | |||
| while (true) { | |||
| // Get the edge between this node and the predecessor | |||
| uint32_t idx = node_labels[label_idx].node_index; | |||
| uint32_t pred_node = node_labels[label_idx].pred_node_index; | |||
| const NodeLabel& node_lab = node_labels[last_label_idx]; | |||
There was a problem hiding this comment.
there was bug before where we were taking label_idx which was left over from the expansion. however, label_idx could be pointing to some random stuff in the adjacency list, just before the expansion was finished, and that label wouldn't even be part of the path.. so here we initialized the path forming with the wrong label
| } | ||
| ferry_endpoint_count++; | ||
|
|
||
| // Reclassify the first/start edge. Do this AFTER finding shortest path so | ||
| // we do not immediately determine we hit the specified classification | ||
| sequence<Edge>::iterator element = edges[edge.second]; | ||
| auto update_edge = *element; | ||
| update_edge.attributes.importance = rc; | ||
| update_edge.attributes.importance = kFerryUpClass; | ||
| update_edge.attributes.reclass_ferry = remove_destonly; |
There was a problem hiding this comment.
#1905 was missing this: the connecting edge itself also needs its destonly removed if there was one.
| // the X is bcs of https://github.com/valhalla/valhalla/issues/4164 | ||
| const std::string ascii_map = R"( | ||
| A--B--C--D------E--F-X-G--H--M | ||
| | | | | | ||
| I--J K---L | ||
| )"; |
There was a problem hiding this comment.
this was VERY annoying to find out what the problem was.. see #4164
fixes #3942
In the end #1905 was mostly missing to unset
destonlyon a ferry-connecting edge. I threw in some more bug fixes around the reclassification and a bit more logic to not penalize destonly edges if the ferry-connecting edge was destonly. If the ferry-connecting edge wasn't destonly, we will always penalize any destonly edges while expanding for reclassification.