Correctly set has_highway, has_ferry and has_toll attributes when directions_type is set to none (valhalla#4706)

Author: michaelhrabanek

CHANGELOG.md

@@ -49,6 +49,7 @@
    * FIXED: Fix segfault in OSRM serializer with bannerInstructions when destination is on roundabout [#4480](https://github.com/valhalla/valhalla/pull/4481)
    * FIXED: Fix segfault in costmatrix (date_time and time zone always added). [#4530](https://github.com/valhalla/valhalla/pull/4530)
    * FIXED: Fixed roundoff issue in Tiles Row and Col methods [#4585](https://github.com/valhalla/valhalla/pull/4585)
+   * FIXED: Fix for assigning attributes has_(highway, ferry, toll) if directions_type is none [#4465](https://github.com/valhalla/valhalla/issues/4465)
 * **Enhancement**
    * UPDATED: French translations, thanks to @xlqian [#4159](https://github.com/valhalla/valhalla/pull/4159)
    * CHANGED: -j flag for multithreaded executables to override mjolnir.concurrency [#4168](https://github.com/valhalla/valhalla/pull/4168)

proto/common.proto

@@ -391,3 +391,13 @@ enum TransitType {
   kGondola = 6;
   kFunicular = 7;
 }
+
+message Summary {
+  float length = 1;          // kilometers or miles based on units
+  double time = 2;           // seconds
+  BoundingBox bbox = 3;      // Bounding box of the shape
+  bool has_time_restrictions = 4; // Does the route contain any time restrictions?
+  bool has_toll = 5;
+  bool has_ferry = 6;
+  bool has_highway = 7;
+}

proto/directions.proto

@@ -6,15 +6,6 @@ import public "sign.proto";
 
 message DirectionsLeg {
 
-  message Summary {
-    float length = 1;          // kilometers or miles based on units
-    double time = 2;           // seconds
-    BoundingBox bbox = 3;      // Bounding box of the shape
-    bool has_time_restrictions = 4; // Does the route contain any time restrictions?
-    bool has_toll = 5;
-    bool has_ferry = 6;
-    bool has_highway = 7;
-  }
 
   message GuidanceView {
     enum Type{

proto/trip.proto

@@ -286,6 +286,7 @@ message TripLeg {
   repeated Incident incidents = 11;
   repeated string algorithms = 12;
   repeated Closure closures = 13;
+  Summary summary = 14;
 }
 
 message TripRoute {

src/odin/directionsbuilder.cc

@@ -95,9 +95,6 @@ void DirectionsBuilder::PopulateDirectionsLeg(const Options& options,
                                               EnhancedTripLeg* etp,
                                               std::list<Maneuver>& maneuvers,
                                               DirectionsLeg& trip_directions) {
-  bool has_toll = false;
-  bool has_highway = false;
-  bool has_ferry = false;
   // Populate trip and leg IDs
   trip_directions.set_trip_id(etp->trip_id());
   trip_directions.set_leg_id(etp->leg_id());
@@ -147,15 +144,12 @@ void DirectionsBuilder::PopulateDirectionsLeg(const Options& options,
     trip_maneuver->set_end_shape_index(maneuver.end_shape_index());
     if (maneuver.portions_toll()) {
       trip_maneuver->set_portions_toll(maneuver.portions_toll());
-      has_toll = true;
     }
     if (maneuver.portions_highway()) {
       trip_maneuver->set_portions_highway(maneuver.portions_highway());
-      has_highway = true;
     }
     if (maneuver.ferry()) {
       trip_maneuver->set_portions_ferry(maneuver.ferry());
-      has_ferry = true;
     }
 
     trip_maneuver->set_has_time_restrictions(maneuver.has_time_restrictions());
@@ -439,9 +433,9 @@ void DirectionsBuilder::PopulateDirectionsLeg(const Options& options,
   trip_directions.mutable_summary()->set_has_time_restrictions(has_time_restrictions);
 
   // Populate toll, highway, ferry tags
-  trip_directions.mutable_summary()->set_has_toll(has_toll);
-  trip_directions.mutable_summary()->set_has_highway(has_highway);
-  trip_directions.mutable_summary()->set_has_ferry(has_ferry);
+  trip_directions.mutable_summary()->set_has_toll(etp->summary().has_toll());
+  trip_directions.mutable_summary()->set_has_highway(etp->summary().has_highway());
+  trip_directions.mutable_summary()->set_has_ferry(etp->summary().has_ferry());
 }
 
 } // namespace odin

src/thor/triplegbuilder.cc

@@ -1754,6 +1754,9 @@ void TripLegBuilder::Build(
   // from the edge index that we assigned to them earlier in route_action
   auto intermediate_itr = trip_path.mutable_location()->begin() + 1;
   double total_distance = 0;
+  bool has_toll = false;
+  bool has_ferry = false;
+  bool has_highway = false;
 
   // loop over the edges to build the trip leg
   for (auto edge_itr = path_begin; edge_itr != path_end; ++edge_itr, ++edge_index) {
@@ -1767,6 +1770,16 @@ void TripLegBuilder::Build(
     const uint8_t travel_type = travel_types[static_cast<uint32_t>(mode)];
     const auto& costing = mode_costing[static_cast<uint32_t>(mode)];
 
+    if (directededge->toll()) {
+      has_toll = true;
+    }
+    if (directededge->use() == Use::kFerry) {
+      has_ferry = true;
+    }
+    if (directededge->classification() == baldr::RoadClass::kMotorway) {
+      has_highway = true;
+    }
+
     // Set node attributes - only set if they are true since they are optional
     graph_tile_ptr start_tile = graphtile;
     graphreader.GetGraphTile(startnode, start_tile);
@@ -2077,6 +2090,11 @@ void TripLegBuilder::Build(
     trip_path.set_osm_changeset(osmchangeset);
   }
 
+  Summary* summary = trip_path.mutable_summary();
+  summary->set_has_toll(has_toll);
+  summary->set_has_ferry(has_ferry);
+  summary->set_has_highway(has_highway);
+
   // Add that extra costing information if requested
   AccumulateRecostingInfoForward(options, start_pct, end_pct, forward_time_info, invariant,
                                  graphreader, trip_path);

test/gurka/test_route_summary.cc

@@ -205,3 +205,83 @@ TEST(TestRouteSummary, DupSummaryFix) {
 
   std::filesystem::remove_all(workdir);
 }
+
+TEST(Standalone, TripLegSummary) {
+  const std::string ascii_map = R"(
+      A---B---C---D
+    )";
+
+  const gurka::ways ways = {{"AB", {{"highway", "motorway"}}},
+                            {"BC", {{"highway", "motorway"}, {"toll", "yes"}}},
+                            {"CD", {{"route", "ferry"}}}};
+  const double gridsize = 100;
+  const auto layout = gurka::detail::map_to_coordinates(ascii_map, gridsize);
+  const std::string workdir = "test/data/gurka_test_route_summary";
+
+  std::string result_json;
+  rapidjson::Document result;
+
+  valhalla::gurka::map map = gurka::buildtiles(layout, ways, {}, {}, workdir);
+
+  valhalla::Api result0 = gurka::do_action(valhalla::Options::route, map, {"A", "B"}, "auto",
+                                           {{"/directions_type", "none"}}, {}, &result_json);
+  EXPECT_TRUE(result0.trip().routes(0).legs(0).summary().has_highway());
+  EXPECT_FALSE(result0.trip().routes(0).legs(0).summary().has_toll());
+  EXPECT_FALSE(result0.trip().routes(0).legs(0).summary().has_ferry());
+
+  result.Parse(result_json.c_str());
+  auto trip = result["trip"].GetObject();
+  auto summary = trip["summary"].GetObject();
+
+  EXPECT_TRUE(summary["has_highway"].GetBool());
+  EXPECT_FALSE(summary["has_toll"].GetBool());
+  EXPECT_FALSE(summary["has_ferry"].GetBool());
+  result_json.erase();
+
+  valhalla::Api result1 = gurka::do_action(valhalla::Options::route, map, {"B", "C"}, "auto",
+                                           {{"/directions_type", "none"}});
+  EXPECT_TRUE(result1.trip().routes(0).legs(0).summary().has_highway());
+  EXPECT_TRUE(result1.trip().routes(0).legs(0).summary().has_toll());
+  EXPECT_FALSE(result1.trip().routes(0).legs(0).summary().has_ferry());
+
+  valhalla::Api result2 = gurka::do_action(valhalla::Options::route, map, {"C", "D"}, "auto",
+                                           {{"/directions_type", "none"}});
+  EXPECT_FALSE(result2.trip().routes(0).legs(0).summary().has_highway());
+  EXPECT_FALSE(result2.trip().routes(0).legs(0).summary().has_toll());
+  EXPECT_TRUE(result2.trip().routes(0).legs(0).summary().has_ferry());
+
+  // Validate that the presence of highway, toll, and ferry tags in route summaries is consistent
+  // and does not depend on the `directions_type` value
+
+  valhalla::Api result3 = gurka::do_action(valhalla::Options::route, map, {"A", "D"}, "auto",
+                                           {{"/directions_type", "none"}}, {}, &result_json);
+
+  EXPECT_TRUE(result3.trip().routes(0).legs(0).summary().has_highway());
+  EXPECT_TRUE(result3.trip().routes(0).legs(0).summary().has_toll());
+  EXPECT_TRUE(result3.trip().routes(0).legs(0).summary().has_ferry());
+
+  result.Parse(result_json.c_str());
+  trip = result["trip"].GetObject();
+  summary = trip["summary"].GetObject();
+
+  EXPECT_TRUE(summary["has_highway"].GetBool());
+  EXPECT_TRUE(summary["has_toll"].GetBool());
+  EXPECT_TRUE(summary["has_ferry"].GetBool());
+  result_json.erase();
+
+  valhalla::Api result4 = gurka::do_action(valhalla::Options::route, map, {"A", "D"}, "auto",
+                                           {{"/directions_type", "maneuvers"}}, {}, &result_json);
+  EXPECT_TRUE(result4.trip().routes(0).legs(0).summary().has_highway());
+  EXPECT_TRUE(result4.trip().routes(0).legs(0).summary().has_toll());
+  EXPECT_TRUE(result4.trip().routes(0).legs(0).summary().has_ferry());
+
+  result.Parse(result_json.c_str());
+  trip = result["trip"].GetObject();
+  summary = trip["summary"].GetObject();
+
+  EXPECT_TRUE(summary["has_highway"].GetBool());
+  EXPECT_TRUE(summary["has_toll"].GetBool());
+  EXPECT_TRUE(summary["has_ferry"].GetBool());
+
+  std::filesystem::remove_all(workdir);
+}

valhalla/odin/enhancedtrippath.h

@@ -88,6 +88,10 @@ class EnhancedTripLeg {
     return trip_path_.bbox();
   }
 
+  const ::valhalla::Summary& summary() const {
+    return trip_path_.summary();
+  }
+
   std::unique_ptr<EnhancedTripLeg_Node> GetEnhancedNode(const int node_index);
 
   std::unique_ptr<EnhancedTripLeg_Edge> GetPrevEdge(const int node_index, int delta = 1);