Direct edges in contractor correctly and add better graph validation.

contractor/edge_based_graph_factory.cpp

@@ -41,35 +41,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <limits>
 
 
-bool checkInvariant(const NodeBasedDynamicGraph& graph, const NodeID source, const EdgeID edge)
-{
-    const auto& data = graph.GetEdgeData(edge);
-    if (!data.forward)
-    {
-        auto target = graph.GetTarget(edge);
-        if (target == SPECIAL_NODEID)
-        {
-            SimpleLogger().Write(logWARNING) << "Invalid target";
-            return false;
-        }
-
-        auto rev_edge = graph.FindEdge(target, source);
-        if (rev_edge == SPECIAL_EDGEID)
-        {
-            SimpleLogger().Write(logWARNING) << "Edge not found";
-            return false;
-        }
-
-        const auto& rev_data = graph.GetEdgeData(rev_edge);
-        if (!rev_data.forward)
-        {
-            SimpleLogger().Write(logWARNING) << "Reverse edge is not forward";
-            return false;
-        }
-    }
-
-    return true;
-}
 
 EdgeBasedGraphFactory::EdgeBasedGraphFactory(std::shared_ptr<NodeBasedDynamicGraph> node_based_graph,
                                    std::shared_ptr<RestrictionMap> restriction_map,
@@ -264,32 +235,6 @@ void EdgeBasedGraphFactory::Run(const std::string &original_edge_data_filename,
                                 const std::string &geometry_filename,
                                 lua_State *lua_state)
 {
-
-#ifndef NDEBUG
-    SimpleLogger().Write() << "Verifying the graph structure before compression:";
-    for (const auto current_node : osrm::irange(0u, m_node_based_graph->GetNumberOfNodes()))
-    {
-        for (const auto current_edge : m_node_based_graph->GetAdjacentEdgeRange(current_node))
-        {
-            EdgeData &edge_data = m_node_based_graph->GetEdgeData(current_edge);
-            if (!edge_data.forward)
-            {
-
-                auto target = m_node_based_graph->GetTarget(current_edge);
-                BOOST_ASSERT(target != SPECIAL_NODEID);
-
-                auto rev_edge = m_node_based_graph->FindEdge(target, current_node);
-                BOOST_ASSERT(rev_edge != SPECIAL_EDGEID);
-
-                const auto& rev_data = m_node_based_graph->GetEdgeData(rev_edge);
-                BOOST_ASSERT(rev_data.forward);
-                continue;
-            }
-        }
-    }
-    SimpleLogger().Write() << " -> graph is ok.";
-#endif
-
     TIMER_START(geometry);
     CompressGeometry();
     TIMER_STOP(geometry);
@@ -466,28 +411,20 @@ void EdgeBasedGraphFactory::CompressGeometry()
                 reverse_weight2 + (has_node_penalty ? speed_profile.traffic_signal_penalty : 0));
             ++removed_node_count;
 
-#ifndef NDEBUG
+
-            if (!checkInvariant(*m_node_based_graph, node_u, forward_e1))
-            {
-                SimpleLogger().Write(logWARNING) << "Contracting " << node_u << " " << node_v << " " << node_w;
-                SimpleLogger().Write(logWARNING) << " coordinates "
-                    << "(" << m_node_info_list[node_u].lat << ", " << m_node_info_list[node_u].lon << ") "
-                    << "(" << m_node_info_list[node_v].lat << ", " << m_node_info_list[node_v].lon << ") "
-                    << "(" << m_node_info_list[node_w].lat << ", " << m_node_info_list[node_w].lon << ") ";
-                BOOST_ASSERT_MSG(false, "Graph invariant is not fulfilled.");
         }
-            if (!checkInvariant(*m_node_based_graph, node_w, reverse_e1))
+
+#ifndef NDEBUG
+        if (!validateNeighborHood(*m_node_based_graph, node_v))
         {
             SimpleLogger().Write(logWARNING) << "Contracting " << node_u << " " << node_v << " " << node_w;
             SimpleLogger().Write(logWARNING) << " coordinates "
-                    << "(" << m_node_info_list[node_u].lat << ", " << m_node_info_list[node_u].lon << ") "
+                << "(" << (m_node_info_list[node_u].lat/COORDINATE_PRECISION) << ", " << (m_node_info_list[node_u].lon/COORDINATE_PRECISION) << ") "
-                    << "(" << m_node_info_list[node_v].lat << ", " << m_node_info_list[node_v].lon << ") "
+                << "(" << (m_node_info_list[node_v].lat/COORDINATE_PRECISION) << ", " << (m_node_info_list[node_v].lon/COORDINATE_PRECISION) << ") "
-                    << "(" << m_node_info_list[node_w].lat << ", " << m_node_info_list[node_w].lon << ") ";
+                << "(" << (m_node_info_list[node_w].lat/COORDINATE_PRECISION) << ", " << (m_node_info_list[node_w].lon/COORDINATE_PRECISION) << ") ";
             BOOST_ASSERT_MSG(false, "Graph invariant is not fulfilled.");
         }
 #endif
-
-        }
     }
     SimpleLogger().Write() << "removed " << removed_node_count << " nodes";
     m_geometry_compressor.PrintStatistics();
@@ -515,14 +452,15 @@ void EdgeBasedGraphFactory::CompressGeometry()
  */
 void EdgeBasedGraphFactory::RenumberEdges()
 {
-    // renumber edge based node IDs
+    // renumber edge based node of outgoing edges
     unsigned numbered_edges_count = 0;
     for (const auto current_node : osrm::irange(0u, m_node_based_graph->GetNumberOfNodes()))
     {
         for (const auto current_edge : m_node_based_graph->GetAdjacentEdgeRange(current_node))
         {
             EdgeData &edge_data = m_node_based_graph->GetEdgeData(current_edge);
-            // FIXME when does that happen? why can we skip here?
+
+            // this edge is an incoming edge
             if (!edge_data.forward)
             {
                 continue;

data_structures/node_based_graph.hpp

@@ -74,7 +74,92 @@ struct NodeBasedEdgeData
 
 using NodeBasedDynamicGraph = DynamicGraph<NodeBasedEdgeData>;
 
+inline bool validateNeighborHood(const NodeBasedDynamicGraph& graph, const NodeID source)
+{
+    for (auto edge = graph.BeginEdges(source); edge < graph.EndEdges(source); ++edge)
+    {
+        const auto& data = graph.GetEdgeData(edge);
+        if (!data.forward && !data.backward)
+        {
+            SimpleLogger().Write(logWARNING) << "Invalid edge directions";
+            return false;
+        }
+
+        auto target = graph.GetTarget(edge);
+        if (target == SPECIAL_NODEID)
+        {
+            SimpleLogger().Write(logWARNING) << "Invalid edge target";
+            return false;
+        }
+
+        bool found_reverse = false;
+        for (auto rev_edge = graph.BeginEdges(target); rev_edge < graph.EndEdges(target); ++rev_edge)
+        {
+            auto rev_target = graph.GetTarget(rev_edge);
+            if (rev_target == SPECIAL_NODEID)
+            {
+                SimpleLogger().Write(logWARNING) << "Invalid reverse edge target";
+                return false;
+            }
+
+            if (rev_target != source)
+            {
+                continue;
+            }
+
+            if (found_reverse)
+            {
+                SimpleLogger().Write(logWARNING) << "Found more than one reverse edge";
+                return false;
+            }
+
+            const auto& rev_data = graph.GetEdgeData(rev_edge);
+
+            // edge is incoming, this must be an outgoing edge
+            if (data.backward && !rev_data.forward)
+            {
+                SimpleLogger().Write(logWARNING) << "Found no outgoing edge to an incoming edge!";
+                return false;
+            }
+
+            // edge is bi-directional, reverse must be as well
+            if (data.forward && data.backward && (!rev_data.forward || !rev_data.backward))
+            {
+                SimpleLogger().Write(logWARNING) << "Found bi-directional edge that is not bi-directional to both ends";
+                return false;
+            }
+
+            found_reverse = true;
+
+        }
+
+        if (!found_reverse)
+        {
+            SimpleLogger().Write(logWARNING) << "Could not find reverse edge";
+            return false;
+        }
+    }
+
+    return true;
+}
+
+// This function checks if the overal graph is undirected (has an edge in each direction).
+inline bool validateNodeBasedGraph(const NodeBasedDynamicGraph& graph)
+{
+    for (auto source = 0u; source < graph.GetNumberOfNodes(); ++source)
+    {
+        if (!validateNeighborHood(graph, source))
+        {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 // Factory method to create NodeBasedDynamicGraph from NodeBasedEdges
+// The since DynamicGraph expects directed edges, we need to insert
+// two edges for undirected edges.
 inline std::shared_ptr<NodeBasedDynamicGraph>
 NodeBasedDynamicGraphFromImportEdges(int number_of_nodes, std::vector<NodeBasedEdge> &input_edge_list)
 {
@@ -83,9 +168,27 @@ NodeBasedDynamicGraphFromImportEdges(int number_of_nodes, std::vector<NodeBasedE
 
     DeallocatingVector<NodeBasedDynamicGraph::InputEdge> edges_list;
     NodeBasedDynamicGraph::InputEdge edge;
+
+    // Since DynamicGraph assumes directed edges we have to make sure we transformed
+    // the compressed edge format into single directed edges. We do this to make sure
+    // every node also knows its incoming edges, not only its outgoing edges and use the backward=true
+    // flag to indicate which is which.
+    //
+    // We do the transformation in the following way:
+    //
+    // if the edge (a, b) is split:
+    //   1. this edge must be in only one direction, so its a --> b
+    //   2. there must be another directed edge b --> a somewhere in the data
+    // if the edge (a, b) is not split:
+    //   1. this edge be on of a --> b od a <-> b
+    //      (a <-- b gets reducted to b --> a)
+    //   2. a --> b will be transformed to a --> b and b <-- a
+    //   3. a <-> b will be transformed to a <-> b and b <-> a (I think a --> b and b <-- a would work as well though)
     for (const NodeBasedEdge &import_edge : input_edge_list)
     {
-        BOOST_ASSERT(import_edge.forward || import_edge.backward);
+        // edges that are not forward get converted by flipping the end points
+        BOOST_ASSERT(import_edge.forward);
+
         if (import_edge.forward)
         {
             edge.source = import_edge.source;
@@ -93,20 +196,10 @@ NodeBasedDynamicGraphFromImportEdges(int number_of_nodes, std::vector<NodeBasedE
             edge.data.forward = import_edge.forward;
             edge.data.backward = import_edge.backward;
         }
-        else
-        {
-            edge.source = import_edge.target;
-            edge.target = import_edge.source;
-            edge.data.backward = import_edge.forward;
-            edge.data.forward = import_edge.backward;
-        }
 
-        if (edge.source == edge.target)
+        BOOST_ASSERT(edge.source != edge.target);
-        {
-            continue;
-        }
 
-        edge.data.distance = (std::max)(static_cast<int>(import_edge.weight), 1);
+        edge.data.distance = static_cast<int>(import_edge.weight);
         BOOST_ASSERT(edge.data.distance > 0);
         edge.data.shortcut = false;
         edge.data.roundabout = import_edge.roundabout;
@@ -126,82 +219,17 @@ NodeBasedDynamicGraphFromImportEdges(int number_of_nodes, std::vector<NodeBasedE
         }
     }
 
-    // sort edges by source node id
     tbb::parallel_sort(edges_list.begin(), edges_list.end());
 
-    // this code removes multi-edges
+    auto graph = std::make_shared<NodeBasedDynamicGraph>(
-    // my merging mutli-edges bi-directional edges can become directional again!
-    // Consider the following example:
-    // a --5-- b
-    //  `--1--^
-    // After merging we need to split {a, b, 5} into (a, b, 1) and (b, a, 5)
-    NodeID edge_count = 0;
-    for (NodeID i = 0; i < edges_list.size();)
-    {
-        const NodeID source = edges_list[i].source;
-        const NodeID target = edges_list[i].target;
-        // remove eigenloops
-        if (source == target)
-        {
-            i++;
-            continue;
-        }
-        NodeBasedDynamicGraph::InputEdge forward_edge;
-        NodeBasedDynamicGraph::InputEdge reverse_edge;
-        forward_edge = reverse_edge = edges_list[i];
-        forward_edge.data.forward = reverse_edge.data.backward = true;
-        forward_edge.data.backward = reverse_edge.data.forward = false;
-        forward_edge.data.shortcut = reverse_edge.data.shortcut = false;
-        forward_edge.data.distance = reverse_edge.data.distance = std::numeric_limits<int>::max();
-        // remove parallel edges and set current distance values
-        while (i < edges_list.size() && edges_list[i].source == source &&
-               edges_list[i].target == target)
-        {
-            if (edges_list[i].data.forward)
-            {
-                forward_edge.data.distance =
-                    std::min(edges_list[i].data.distance, forward_edge.data.distance);
-            }
-            if (edges_list[i].data.backward)
-            {
-                reverse_edge.data.distance =
-                    std::min(edges_list[i].data.distance, reverse_edge.data.distance);
-            }
-            ++i;
-        }
-        // merge edges (s,t) and (t,s) into bidirectional edge
-        if (forward_edge.data.distance == reverse_edge.data.distance)
-        {
-            if (static_cast<int>(forward_edge.data.distance) != std::numeric_limits<int>::max())
-            {
-                forward_edge.data.backward = true;
-                BOOST_ASSERT(edge_count < i);
-                edges_list[edge_count++] = forward_edge;
-            }
-        }
-        else
-        { // insert seperate edges
-          // this case can only happen if we merged a bi-directional edge with a directional
-          // edge above, this incrementing i and making it safe to overwrite the next element
-          // as well
-            if (static_cast<int>(forward_edge.data.distance) != std::numeric_limits<int>::max())
-            {
-                BOOST_ASSERT(edge_count < i);
-                edges_list[edge_count++] = forward_edge;
-            }
-            if (static_cast<int>(reverse_edge.data.distance) != std::numeric_limits<int>::max())
-            {
-                BOOST_ASSERT(edge_count < i);
-                edges_list[edge_count++] = reverse_edge;
-            }
-        }
-    }
-    edges_list.resize(edge_count);
-    SimpleLogger().Write() << "merged " << edges_list.size() - edge_count << " edges out of "
-                           << edges_list.size();
-
-    return std::make_shared<NodeBasedDynamicGraph>(
         static_cast<NodeBasedDynamicGraph::NodeIterator>(number_of_nodes), edges_list);
+
+
+#ifndef NDEBUG
+    BOOST_ASSERT(validateNodeBasedGraph(*graph));
+#endif
+
+    return graph;
 }
 
 #endif // NODE_BASED_GRAPH_HPP

extractor/extraction_containers.cpp

@@ -184,6 +184,7 @@ void ExtractionContainers::PrepareEdges()
     {
         if (edge_iterator->result.source < node_iterator->node_id)
         {
+            edge_iterator->result.source = SPECIAL_NODEID;
             ++edge_iterator;
             continue;
         }
@@ -193,6 +194,15 @@ void ExtractionContainers::PrepareEdges()
             continue;
         }
 
+        // remove loops
+        if (edge_iterator->result.source == edge_iterator->result.target)
+        {
+            edge_iterator->result.source = SPECIAL_NODEID;
+            edge_iterator->result.target = SPECIAL_NODEID;
+            ++edge_iterator;
+            continue;
+        }
+
         BOOST_ASSERT(edge_iterator->result.source == node_iterator->node_id);
 
         // assign new node id
@@ -222,13 +232,16 @@ void ExtractionContainers::PrepareEdges()
     edge_iterator = all_edges_list.begin();
     while (edge_iterator != all_edges_list.end() && node_iterator != all_nodes_list.end())
     {
+        // skip all invalid edges
+        if (edge_iterator->result.source == SPECIAL_NODEID)
+        {
+            ++edge_iterator;
+            continue;
+        }
+
         if (edge_iterator->result.target < node_iterator->node_id)
         {
-            // mark edge as invalid
-            edge_iterator->result.source = SPECIAL_NODEID;
             edge_iterator->result.target = SPECIAL_NODEID;
-
-            // FIXME we are skipping edges here: That means the data is broken!
             ++edge_iterator;
             continue;
         }
@@ -237,11 +250,11 @@ void ExtractionContainers::PrepareEdges()
             ++node_iterator;
             continue;
         }
+
         BOOST_ASSERT(edge_iterator->result.target == node_iterator->node_id);
-        if (edge_iterator->source_coordinate.lat != std::numeric_limits<int>::min() &&
+        BOOST_ASSERT(edge_iterator->weight_data.speed >= 0);
-            edge_iterator->source_coordinate.lon != std::numeric_limits<int>::min())
+        BOOST_ASSERT(edge_iterator->source_coordinate.lat != std::numeric_limits<int>::min());
-        {
+        BOOST_ASSERT(edge_iterator->source_coordinate.lon != std::numeric_limits<int>::min());
-            BOOST_ASSERT(edge_iterator->weight_data.speed != -1);
 
         const double distance = coordinate_calculation::euclidean_distance(
             edge_iterator->source_coordinate.lat, edge_iterator->source_coordinate.lon,
@@ -257,21 +270,22 @@ void ExtractionContainers::PrepareEdges()
                 case InternalExtractorEdge::WeightType::SPEED:
                     return (distance * 10.) / (data.speed / 3.6);
                     break;
-                    default:
+                case InternalExtractorEdge::WeightType::INVALID:
                     osrm::exception("invalid weight type");
             }
             return -1.0;
         }(edge_iterator->weight_data);
 
         auto& edge = edge_iterator->result;
-            edge.weight = std::max(1, (int)std::floor(weight + .5));
+        edge.weight = std::max(1, static_cast<int>(std::floor(weight + .5)));
 
         // assign new node id
         auto id_iter = external_to_internal_node_id_map.find(node_iterator->node_id);
         BOOST_ASSERT(id_iter != external_to_internal_node_id_map.end());
         edge.target = id_iter->second;
 
-            // if source id > target id -> swap
+        // orient edges consistently: source id < target id
+        // important for multi-edge removal
         if (edge.source > edge.target)
         {
             std::swap(edge.source, edge.target);
@@ -281,15 +295,6 @@ void ExtractionContainers::PrepareEdges()
             edge.forward = edge.backward;
             edge.backward = temp;
         }
-        }
-        else
-        {
-            // mark edge as invalid
-            edge_iterator->result.source = SPECIAL_NODEID;
-            edge_iterator->result.target = SPECIAL_NODEID;
-
-            // FIXME we should print a warning here.
-        }
         ++edge_iterator;
     }
     TIMER_STOP(compute_weights);
@@ -303,13 +308,18 @@ void ExtractionContainers::PrepareEdges()
     std::cout << "ok, after " << TIMER_SEC(sort_edges_by_renumbered_start) << "s" << std::endl;
 
     BOOST_ASSERT(all_edges_list.size() > 0);
-    for (unsigned i = 1; i < all_edges_list.size();)
+    for (unsigned i = 0; i < all_edges_list.size();)
     {
         // only invalid edges left
         if (all_edges_list[i].result.source == SPECIAL_NODEID)
         {
             break;
         }
+        // skip invalid edges
+        if (all_edges_list[i].result.target == SPECIAL_NODEID)
+        {
+            continue;
+        }
 
         unsigned start_idx = i;
         NodeID source = all_edges_list[i].result.source;
@@ -320,6 +330,7 @@ void ExtractionContainers::PrepareEdges()
         unsigned min_forward_idx = std::numeric_limits<unsigned>::max();
         unsigned min_backward_idx = std::numeric_limits<unsigned>::max();
 
+        // find minimal edge in both directions
         while (all_edges_list[i].result.source == source &&
                all_edges_list[i].result.target == target)
         {
@@ -338,26 +349,34 @@ void ExtractionContainers::PrepareEdges()
 
         BOOST_ASSERT(min_forward_idx == std::numeric_limits<unsigned>::max() || min_forward_idx < i);
         BOOST_ASSERT(min_backward_idx == std::numeric_limits<unsigned>::max() || min_backward_idx < i);
+        BOOST_ASSERT(min_backward_idx != std::numeric_limits<unsigned>::max() ||
+                     min_forward_idx != std::numeric_limits<unsigned>::max());
 
-        // reset direction for both edges
+        if (min_backward_idx == min_forward_idx)
-        if (min_forward_idx != std::numeric_limits<unsigned>::max())
         {
-            all_edges_list[min_forward_idx].result.forward = false;
+            all_edges_list[min_forward_idx].result.is_split = false;
-            all_edges_list[min_forward_idx].result.backward = false;
-        }
-        if (min_backward_idx != std::numeric_limits<unsigned>::max())
-        {
-            all_edges_list[min_backward_idx].result.forward = false;
-            all_edges_list[min_backward_idx].result.backward = false;
-        }
-
-        // set directions that were chosen as min
-        // note that this needs to come after the ifs above, since
-        // the minimal forward and backward edge can be the same
-        if (min_forward_idx != std::numeric_limits<unsigned>::max())
             all_edges_list[min_forward_idx].result.forward = true;
-        if (min_backward_idx != std::numeric_limits<unsigned>::max())
+            all_edges_list[min_forward_idx].result.backward = true;
-            all_edges_list[min_backward_idx].result.backward = true;
+        }
+        else
+        {
+            bool has_forward = min_forward_idx != std::numeric_limits<unsigned>::max();
+            bool has_backward = min_backward_idx != std::numeric_limits<unsigned>::max();
+            if (has_forward)
+            {
+                all_edges_list[min_forward_idx].result.forward = true;
+                all_edges_list[min_forward_idx].result.backward = false;
+                all_edges_list[min_forward_idx].result.is_split = has_backward;
+            }
+            if (has_backward)
+            {
+                std::swap(all_edges_list[min_backward_idx].result.source,
+                          all_edges_list[min_backward_idx].result.target);
+                all_edges_list[min_backward_idx].result.forward = true;
+                all_edges_list[min_backward_idx].result.backward = false;
+                all_edges_list[min_backward_idx].result.is_split = has_forward;
+            }
+        }
 
         // invalidate all unused edges
         for (unsigned j = start_idx; j < i; j++)

util/graph_loader.hpp

@@ -142,12 +142,26 @@ NodeID loadEdgesFromFile(std::istream &input_stream,
 
     input_stream.read((char *) edge_list.data(), m * sizeof(NodeBasedEdge));
 
+    BOOST_ASSERT(edge_list.size() > 0);
+
 #ifndef NDEBUG
-    for (const auto& edge : edge_list)
+    SimpleLogger().Write() << "Validating loaded edges...";
+    std::sort(edge_list.begin(), edge_list.end(),
+            [](const NodeBasedEdge& lhs, const NodeBasedEdge& rhs)
             {
+                return (lhs.source < rhs.source) || (lhs.source == rhs.source && lhs.target < rhs.target);
+            });
+    for (auto i = 1u; i < edge_list.size(); ++i)
+    {
+        const auto& edge = edge_list[i];
+        const auto& prev_edge = edge_list[i-1];
+
         BOOST_ASSERT_MSG(edge.weight > 0, "loaded null weight");
-        BOOST_ASSERT_MSG(edge.forward || edge.backward, "loaded invalid direction");
+        BOOST_ASSERT_MSG(edge.forward, "edge must be oriented in forward direction");
         BOOST_ASSERT_MSG(edge.travel_mode != TRAVEL_MODE_INACCESSIBLE, "loaded non-accessible");
+
+        BOOST_ASSERT_MSG(edge.source != edge.target, "loaded edges contain a loop");
+        BOOST_ASSERT_MSG(edge.source != prev_edge.source || edge.target != prev_edge.target, "loaded edges contain a multi edge");
     }
 #endif