diff --git a/include/extractor/area/dijkstra.hpp b/include/extractor/area/dijkstra.hpp index a859f9a419a..682d8350775 100644 --- a/include/extractor/area/dijkstra.hpp +++ b/include/extractor/area/dijkstra.hpp @@ -140,13 +140,33 @@ template class Dijkstra distances[s] = 0; pq.insert(s); + // Which vertices have been popped, and so have their final distance. + // + // Without this the tie-break below can rewrite the predecessor of a vertex that + // was settled long ago. Two adjacent vertices the same distance from the source + // then end up pointing at each other, and the predecessor array, which every + // caller walks to recover a path, has a cycle in it. + std::vector settled(vertices.size(), false); + while (!pq.empty()) { size_t u = pq.pop(); + if (settled[u]) + { + continue; + } + settled[u] = true; double dist_u = distances[u]; for (Edge e : adj[u]) { size_t v = e.other; + if (settled[v]) + { + // Its distance is final and so is the way it was reached. This is + // also what keeps the predecessors acyclic: a vertex only ever + // points at one that settled before it did. + continue; + } const double candidate = dist_u + e.weight; if (significantly_shorter(candidate, distances[v])) { @@ -157,6 +177,8 @@ template class Dijkstra else if (approximately_equal(candidate, distances[v]) && vertices[u] < vertices[predecessors[v]]) { + // Equally short, so the choice is free and is made the same way on + // every platform and in every run: the smaller vertex wins. predecessors[v] = u; pq.insert_or_decrease(v); } diff --git a/src/extractor/area/area_mesher.cpp b/src/extractor/area/area_mesher.cpp index c6d71769d4b..c9a13372c12 100644 --- a/src/extractor/area/area_mesher.cpp +++ b/src/extractor/area/area_mesher.cpp @@ -504,11 +504,22 @@ std::set AreaMesher::run_dijkstra(const OsmiumPolygon &poly, for (index_t target = 0; target < d.num_vertices(); ++target) { index_t v = target; - while (v != u && v != predecessors.at(v)) + // A tree of n vertices has no path longer than n edges, so a walk that has + // not reached the root by then is going round a cycle. Dijkstra does not + // produce one, but this is a loop over every vertex of every area in the + // input, and the cost of it being wrong is that extraction never finishes: + // no output, no error, nothing to look at. So it is bounded here and says + // so, rather than being trusted. + for (index_t step = 0; v != u && v != predecessors.at(v); ++step) { + if (step > d.num_vertices()) + { + util::Log(logWARNING) + << "Shortest-path tree has a cycle at node " << d.get_vertex(v).ref() + << ", giving up on this entry point."; + break; + } auto s = OsmiumSegment(d.get_vertex(v), d.get_vertex(predecessors.at(v))); - util::Log(logDEBUG) - << " Collecting: " << s.first.ref() << " -> " << s.second.ref(); result.emplace(s); v = predecessors.at(v); } diff --git a/unit_tests/extractor/area/dijkstra.cpp b/unit_tests/extractor/area/dijkstra.cpp index 2e09a3ee88f..dbb56cd8f61 100644 --- a/unit_tests/extractor/area/dijkstra.cpp +++ b/unit_tests/extractor/area/dijkstra.cpp @@ -132,4 +132,53 @@ BOOST_AUTO_TEST_CASE(area_dijkstra_equal_distance_tie_break_test) CHECK_EQUAL_RANGES(d.get_predecessors(), expected); } +// Following the predecessors from anywhere reaches the source. +// +// This is what every caller of get_predecessors() does, and none of them can check it +// first: recovering a path means walking the array until the root turns up. If two +// entries point at each other the walk never ends, and in an extractor that means the +// whole run never finishes, with no output and no error to look at. +// +// The shape that breaks it needs an edge of no length between two vertices the same +// distance from the source, which sounds contrived and is not. Plaza rings routinely +// carry two nodes at the same location, and a ring edge between them weighs zero. +// +// With one, both vertices settle at the same distance, and the tie-break that prefers +// the lower-numbered predecessor then fires in both directions: each is a better +// predecessor for the other than the source is, so each ends up pointing at the other. +BOOST_AUTO_TEST_CASE(area_dijkstra_leaves_no_cycle_in_the_predecessors) +{ + // Vertex numbers deliberately out of step with insertion order, because the + // tie-break compares the vertices and the walk uses their indices. + Dijkstra d; + d.add_edge(10, 3, 1.0); // the source, and one arm + d.add_edge(10, 2, 1.0); // the other arm, exactly as long + d.add_edge(3, 2, 0.0); // two nodes mapped at the same place + + const auto source = d.index_of(10); + BOOST_CHECK_NO_THROW(d.run(source)); + + const auto &predecessors = d.get_predecessors(); + for (std::size_t start = 0; start < d.num_vertices(); ++start) + { + std::size_t v = start; + std::size_t steps = 0; + while (v != source && v != predecessors.at(v)) + { + v = predecessors.at(v); + BOOST_REQUIRE_MESSAGE(++steps <= d.num_vertices(), + "predecessors cycle, walking from vertex " + << d.get_vertex(start)); + } + } + + // Both arms are still a distance of one away, and the tie is still broken the same + // way every run: vertex 2 is reached through vertex 3, because going that way costs + // the same and 3 is the lower predecessor. Only the loop is gone, not the choice. + BOOST_CHECK_CLOSE(d.get_distances().at(d.index_of(3)), 1.0, 1e-9); + BOOST_CHECK_CLOSE(d.get_distances().at(d.index_of(2)), 1.0, 1e-9); + BOOST_CHECK_EQUAL(predecessors.at(d.index_of(3)), source); + BOOST_CHECK_EQUAL(predecessors.at(d.index_of(2)), d.index_of(3)); +} + BOOST_AUTO_TEST_SUITE_END()