Update review findings

CHANGELOG.md

@@ -1,10 +1,3 @@
-# 6.0.0
-  - Profiles:
-    - `turn_function` now does not return an integer but takes in a `turn` object and modifies it
-    - `uturn_penalty` is deprecated set it over the `turn_function`
-    - traffic light penalties now need to be set over the node function, `traffic_light_penalty` is deprecated:
-        result.weight_penalty and result.duration_penalty
-
 # 5.6.0
   - Changes from 5.5
     - Bugfixes
@@ -17,6 +10,13 @@
       - No longer emitting turns on ferries, if a ferry should use multiple docking locations
     - Profiles
       - Removed the `./profile.lua -> ./profiles/car.lua` symlink. Use specific profiles from the `profiles` directory.
+      - properties object has a new `weight_name` field, default value is "duration"
+      - properties object has a new `weight_precision` field that specifies a decimal precision of edge weights, default value 1
+      - in `way_function` can be set `forward_rate` and `backward_rate` fields of `ExtractionWay` that have the same interpretation for the way weight as `forward_speed` and `backward_speed` for the edge duration. The rate has units of distance per a unit weight, so higher values will be preferable during routing.
+      - `turn_function` now does not return an integer but takes in a `ExtractionTurn` object and can modify `weight` and `duration` fields
+      - `segment_function` now takes in a `ExtractionSegment` object and can modify `weight` and `duration` fields
+      - `properties.uturn_penalty` is deprecated. Set it in the `turn_function`
+      - `properties.traffic_light_penalty` is deprecated. Traffic light penalties now need to be set over in the turn function. Each turn with a traffic light is marked with `ExtractionTurn::has_traffic_light` = true
     - Infrastructure
       - Disabled link-time optimized (LTO) builds by default. Enable by passing `-DENABLE_LTO=ON` to `cmake` if you need the performance and know what you are doing.
       - Datafile versioning is now based on OSRM semver values, rather than source code checksums.

docs/http.md

@@ -56,7 +56,7 @@ Example: 2nd location use the default value for `option`:
 ```curl
 # Query on Berlin with three coordinates:
 curl 'http://router.project-osrm.org/route/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219?overview=false'
- 
+
 # Using polyline:
 curl 'http://router.project-osrm.org/route/v1/driving/polyline(ofp_Ik_vpAilAyu@te@g`E)?overview=false'
 ```
@@ -401,13 +401,15 @@ Represents a route through (potentially multiple) waypoints.
 - `distance`: The distance traveled by the route, in `float` meters.
 - `duration`: The estimated travel time, in `float` number of seconds.
 - `geometry`: The whole geometry of the route value depending on `overview` parameter, format depending on the `geometries` parameter. See `RouteStep`'s `geometry` field for a parameter documentation.
-  
+- `weight`: The calculated weight of the route.
+- `weight_name`: The name of the weight profile used during extraction phase.
+
 | overview   | Description                 |
 |------------|-----------------------------|
 | simplified | Geometry is simplified according to the highest zoom level it can still be displayed on full. |
 | full       | Geometry is not simplified. |
 | false      | Geometry is not added.      |
-  
+
 - `legs`: The legs between the given waypoints, an array of `RouteLeg` objects.
 
 #### Example
@@ -418,6 +420,8 @@ Three input coordinates, `geometry=geojson`, `steps=false`:
 {
   "distance": 90.0,
   "duration": 300.0,
+  "weight": 300.0,
+  "weight_name": "duration",
   "geometry": {"type": "LineString", "coordinates": [[120.0, 10.0], [120.1, 10.0], [120.2, 10.0], [120.3, 10.0]]},
   "legs": [
     {
@@ -442,15 +446,16 @@ Represents a route between two waypoints.
 
 - `distance`: The distance traveled by this route leg, in `float` meters.
 - `duration`: The estimated travel time, in `float` number of seconds.
+- `weight`: The calculated weight of the route leg.
 - `summary`: Summary of the route taken as `string`. Depends on the `steps` parameter:
-   
+
 | steps        |                                                                       |
 |--------------|-----------------------------------------------------------------------|
 | true         | Names of the two major roads used. Can be empty if route is too short.|
 | false        | empty `string`                                                        |
 
 - `steps`: Depends on the `steps` parameter.
-   
+
 | steps        |                                                                       |
 |--------------|-----------------------------------------------------------------------|
 | true         | array of `RouteStep` objects describing the turn-by-turn instructions |
@@ -460,8 +465,8 @@ Represents a route between two waypoints.
 
 | annotations  |                                                                       |
 |--------------|-----------------------------------------------------------------------|
-| true         | An `Annotation` object containing node ids, durations and distances   |
+| true         | An `Annotation` object containing node ids, durations distances and   |
-| false        | `undefined`                                                           |
+| false        | weights `undefined`                                                   |
 
 #### Example
 
@@ -471,6 +476,7 @@ With `steps=false` and `annotations=true`:
 {
   "distance": 30.0,
   "duration": 100.0,
+  "weight": 100.0,
   "steps": [],
   "annotation": {
     "distance": [5,5,10,5,5],
@@ -491,6 +497,7 @@ Annotation of the whole route leg with fine-grained information about each segme
 - `duration`: The duration between each pair of coordinates, in seconds
 - `datasources`: The index of the datasource for the speed between each pair of coordinates. `0` is the default profile, other values are supplied via `--segment-speed-file` to `osrm-contract`
 - `nodes`: The OSM node ID for each coordinate along the route, excluding the first/last user-supplied coordinates
+- `weight`: The weights between each pair of coordinates
 
 #### Example
 
@@ -499,7 +506,8 @@ Annotation of the whole route leg with fine-grained information about each segme
   "distance": [5,5,10,5,5],
   "duration": [15,15,40,15,15],
   "datasources": [1,0,0,0,1],
-  "nodes": [49772551,49772552,49786799,49786800,49786801,49786802]
+  "nodes": [49772551,49772552,49786799,49786800,49786801,49786802],
+  "weight": [15,15,40,15,15]
 }
 ```
 
@@ -515,6 +523,7 @@ step.
 - `distance`: The distance of travel from the maneuver to the subsequent step, in `float` meters.
 - `duration`: The estimated travel time, in `float` number of seconds.
 - `geometry`: The unsimplified geometry of the route segment, depending on the `geometries` parameter.
+- `weight`: The calculated weight of the step.
 
 | `geometry` |                                                                    |
 |------------|--------------------------------------------------------------------|
@@ -537,6 +546,7 @@ step.
    "geometry" : "{lu_IypwpAVrAvAdI",
    "mode" : "driving",
    "duration" : 15.6,
+   "weight" : 15.6,
    "intersections" : [
       {  "bearings" : [ 10, 92, 184, 270 ],
          "lanes" : [
@@ -583,7 +593,7 @@ step.
   direction of travel immediately after the maneuver.  Range 0-359.
 - `type` A string indicating the type of maneuver. **new identifiers might be introduced without API change**
    Types  unknown to the client should be handled like the `turn` type, the existance of correct `modifier` values is guranteed.
-  
+
 | `type`           | Description                                                  |
 |------------------|--------------------------------------------------------------|
 | `turn`           | a basic turn into direction of the `modifier`                |
@@ -607,7 +617,7 @@ step.
   between all instructions. They only offer a fallback in case nothing else is to report.
 
 - `modifier` An optional `string` indicating the direction change of the maneuver.
-  
+
 | `modifier`        | Description                               |
 |-------------------|-------------------------------------------|
 | `uturn`           | indicates  reversal of direction          |
@@ -618,18 +628,18 @@ step.
 | `slight left`     | a slight turn to the left                 |
 | `left`            | a normal turn to the left                 |
 | `sharp left`      | a sharp turn to the left                  |
-  
+
   The list of turns without a modifier is limited to: `depart/arrive`. If the source/target location is close enough to the `depart/arrive` location, no modifier will be given.
-  
+
   The meaning depends on the `type` field.
-  
+
 | `type`                 | Description                                                                                                               |
 |------------------------|---------------------------------------------------------------------------------------------------------------------------|
 | `turn`                 | `modifier` indicates the change in direction accomplished through the turn                                                |
 | `depart`/`arrive`      | `modifier` indicates the position of departure point and arrival point in relation to the current direction of travel      |
-  
+
 - `exit` An optional `integer` indicating number of the exit to take. The field exists for the following `type` field:
-  
+
 | `type`                 | Description                                                                                                               |
 |------------------------|---------------------------------------------------------------------------------------------------------------------------|
 | `roundabout`/`rotary`         | Number of the roundabout exit to take. If exit is `undefined` the destination is on the roundabout.                       |
@@ -645,7 +655,7 @@ A `Lane` represents a turn lane at the corresponding turn location.
 **Properties**
 
 - `indications`: a indication (e.g. marking on the road) specifying the turn lane. A road can have multiple indications (e.g. an arrow pointing straight and left). The indications are given in an array, each containing one of the following types. Further indications might be added on without an API version change.
-  
+
 | `value`                | Description                                                                                                               |
 |------------------------|---------------------------------------------------------------------------------------------------------------------------|
 | `none`                 | No dedicated indication is shown.                                                                                         |
@@ -657,7 +667,7 @@ A `Lane` represents a turn lane at the corresponding turn location.
 | `slight left`          | An indication indicating a slight left turn (i.e. slightly bend arrow).                                                   |
 | `left`                 | An indication indicating a left turn (i.e. bend arrow).                                                                   |
 | `sharp left`           | An indication indicating a sharp left turn (i.e. strongly bend arrow).                                                    |
-  
+
 - `valid`: a boolean flag indicating whether the lane is a valid choice in the current maneuver
 
 #### Example

features/options/profiles/version0.feature

@@ -72,7 +72,7 @@ end
 
 function turn_function (angle)
   print('turn_function ' .. angle)
-  return angle == 0 and 0 or 17
+  return angle == 0 and 0 or 42
 end
 
 function segment_function (source, target, distance, weight)
@@ -82,7 +82,7 @@ end
         And the node map
            """
                a
-              bcd
+             b c d
                e
            """
         And the ways
@@ -102,6 +102,6 @@ end
 
         When I route I should get
            | from | to | route    | time  |
-           | a    | b  | ac,cb,cb | 16.7s |
+           | a    | b  | ac,cb,cb | 24.2s |
-           | a    | d  | ac,cd,cd | 16.7s |
+           | a    | d  | ac,cd,cd | 24.2s |
            | a    | e  | ac,ce,ce | 20s   |

features/testbot/weight.feature

@@ -37,48 +37,27 @@ Feature: Weight tests
             | s,t       | abc,cde,cde | 3.1s,3s,0s | duration    | 3.1,3,0 |
 
     # FIXME include/engine/guidance/assemble_geometry.hpp:95
-    @todo
+    Scenario: Start and target on the same and adjacent edge
-    Scenario: Start and target on the same edge
-        Given the query options
-            | annotations | true |
-
-        Given the node map
-            """
-            a-------b
-              ·   ·
-              s   t
-            """
-
-        And the ways
-            | nodes |
-            | ab    |
-
-        When I route I should get
-            | waypoints | route   | distances | weights | times | annotation         |
-            | s,t       | abc,abc | 20m,0m    | 20,0    | 2s,0s | 29:2.9:20.017685:0 |
-            | t,s       | abc,abc | 20m,0m    | 20,0    | 2s,0s | 29:2.9:20.017685:0 |
-
-    # FIXME include/engine/guidance/assemble_geometry.hpp:95
-    @todo
-    Scenario: Start and target on adjacent edges
         Given the query options
             | annotations | true |
 
         Given the node map
             """
             a-------b-------c
-              ·           ·
+              ·   ·   ·
-              s           t
+              s   t   e
             """
 
         And the ways
             | nodes |
-            | ab    |
+            | abc   |
 
         When I route I should get
-            | waypoints | route   | distances | weights | times   | annotation         |
+            | waypoints | route   | distances | weights | times   | annotation                          |
-            | s,t       | abc,abc | 30m,0m    | 31,0    | 3.1s,0s | 31:3.1:30.026527:0 |
+            | s,t       | abc,abc | 20m,0m    | 2.1,0   | 2.1s,0s | 3:3:20.017685:0                     |
-            | t,s       | abc,abc | 30m,0m    | 31,0    | 3.1s,0s | 31:3.1:30.026527:0 |
+            | t,s       | abc,abc | 20m,0m    | 2.1,0   | 2.1s,0s | 3.1:3.1:20.017685:0                 |
+            | s,e       | abc,abc | 40m,0m    | 4.1,0   | 4.1s,0s | 3.1:3.1:30.026527:0,1:1:10.008842:0 |
+            | e,s       | abc,abc | 40m,0m    | 4.1,0   | 4.1s,0s | 1:1:10.008842:0,3.1:3.1:30.026527:0 |
 
 
     Scenario: Step weights -- way_function: fail if no weight or weight_per_meter property

include/engine/guidance/assemble_geometry.hpp

@@ -78,7 +78,7 @@ inline LegGeometry assembleGeometry(const datafacade::BaseDataFacade &facade,
         geometry.annotations.emplace_back(
             LegGeometry::Annotation{current_distance,
                                     path_point.duration_until_turn / 10.,
-                                    path_point.weight_until_turn / 10.,
+                                    path_point.weight_until_turn / facade.GetWeightMultiplier(),
                                     path_point.datasource_id});
         geometry.locations.push_back(std::move(coordinate));
         geometry.osm_node_ids.push_back(facade.GetOSMNodeIDOfNode(path_point.turn_via_node));
@@ -92,13 +92,15 @@ inline LegGeometry assembleGeometry(const datafacade::BaseDataFacade &facade,
     const std::vector<DatasourceID> forward_datasources =
         facade.GetUncompressedForwardDatasources(target_node.packed_geometry_id);
 
-    // FIXME this is wrong. We need to check for traversal direction here
+    // FIXME if source and target phantoms are on the same segment then duration and weight
-    // and for the case of a local path (target and source on the same edge)
+    // will be from one projected point till end of segment
-    geometry.annotations.emplace_back(
+    // testbot/weight.feature:Start and target on the same and adjacent edge
-        LegGeometry::Annotation{current_distance,
+    geometry.annotations.emplace_back(LegGeometry::Annotation{
-                                target_node.forward_duration / 10.,
+        current_distance,
-                                target_node.forward_weight / 10.,
+        (reversed_target ? target_node.reverse_duration : target_node.forward_duration) / 10.,
-                                forward_datasources[target_node.fwd_segment_position]});
+        (reversed_target ? target_node.reverse_weight : target_node.forward_weight) /
+            facade.GetWeightMultiplier(),
+        forward_datasources[target_node.fwd_segment_position]});
 
     geometry.segment_offsets.push_back(geometry.locations.size());
     geometry.locations.push_back(target_node.location);

include/engine/guidance/assemble_leg.hpp

@@ -138,6 +138,10 @@ inline RouteLeg assembleLeg(const datafacade::BaseDataFacade &facade,
         route_data.begin(), route_data.end(), 0, [](const double sum, const PathData &data) {
             return sum + data.duration_until_turn;
         });
+    auto weight = std::accumulate(
+        route_data.begin(), route_data.end(), 0, [](const double sum, const PathData &data) {
+            return sum + data.weight_until_turn;
+        });
 
     //                 s
     //                 |
@@ -165,6 +169,8 @@ inline RouteLeg assembleLeg(const datafacade::BaseDataFacade &facade,
     {
         duration -= (target_traversed_in_reverse ? source_node.reverse_duration
                                                  : source_node.forward_duration);
+        weight -=
+            (target_traversed_in_reverse ? source_node.reverse_weight : source_node.forward_weight);
     }
 
     std::string summary;
@@ -196,7 +202,11 @@ inline RouteLeg assembleLeg(const datafacade::BaseDataFacade &facade,
         summary = boost::algorithm::join(summary_names, ", ");
     }
 
-    return RouteLeg{duration / 10., distance, summary, {}};
+    return RouteLeg{std::round(distance * 10.) / 10.,
+                    duration / 10.,
+                    duration / facade.GetWeightMultiplier(),
+                    summary,
+                    {}};
 }
 
 } // namespace guidance

include/engine/guidance/route.hpp

@@ -10,8 +10,9 @@ namespace guidance
 
 struct Route
 {
-    double duration;
     double distance;
+    double duration;
+    double weight;
 };
 }
 }

include/engine/guidance/route_leg.hpp

@@ -17,8 +17,9 @@ namespace guidance
 
 struct RouteLeg
 {
-    double duration;
     double distance;
+    double duration;
+    double weight;
     std::string summary;
     std::vector<RouteStep> steps;
 };

include/extractor/profile_properties.hpp

@@ -14,10 +14,7 @@ const constexpr auto DEFAULT_MAX_SPEED = 180 / 3.6; // 180kmph -> m/s
 
 struct ProfileProperties
 {
-    enum
+    static constexpr int MAX_WEIGHT_NAME_LENGTH = 255;
-    {
-        MAX_WEIGHT_NAME_LENGTH = 255
-    };
 
     ProfileProperties()
         : traffic_signal_penalty(0), u_turn_penalty(0),

src/engine/api/json_factory.cpp

@@ -241,8 +241,8 @@ util::json::Object makeRouteStep(guidance::RouteStep step, util::json::Value geo
 {
     util::json::Object route_step;
     route_step.values["distance"] = std::round(step.distance * 10) / 10.;
-    route_step.values["duration"] = std::round(step.duration * 10) / 10.;
+    route_step.values["duration"] = step.duration;
-    route_step.values["weight"] = step.weight; // We should round to weight_precision here
+    route_step.values["weight"] = step.weight;
     route_step.values["name"] = std::move(step.name);
     if (!step.ref.empty())
         route_step.values["ref"] = std::move(step.ref);
@@ -280,9 +280,10 @@ util::json::Object makeRoute(const guidance::Route &route,
                              const char *weight_name)
 {
     util::json::Object json_route;
+    json_route.values["distance"] = route.distance;
+    json_route.values["duration"] = route.duration;
+    json_route.values["weight"] = route.weight;
     json_route.values["weight_name"] = weight_name;
-    json_route.values["distance"] = std::round(route.distance * 10) / 10.;
-    json_route.values["duration"] = std::round(route.duration * 10) / 10.;
     json_route.values["legs"] = std::move(legs);
     if (geometry)
     {
@@ -309,8 +310,9 @@ util::json::Object makeWaypoint(const util::Coordinate location, std::string nam
 util::json::Object makeRouteLeg(guidance::RouteLeg leg, util::json::Array steps)
 {
     util::json::Object route_leg;
-    route_leg.values["distance"] = std::round(leg.distance * 10) / 10.;
+    route_leg.values["distance"] = leg.distance;
-    route_leg.values["duration"] = std::round(leg.duration * 10) / 10.;
+    route_leg.values["duration"] = leg.duration;
+    route_leg.values["weight"] = leg.weight;
     route_leg.values["summary"] = std::move(leg.summary);
     route_leg.values["steps"] = std::move(steps);
     return route_leg;

src/engine/guidance/assemble_route.cpp

@@ -19,8 +19,12 @@ Route assembleRoute(const std::vector<RouteLeg> &route_legs)
         route_legs.begin(), route_legs.end(), 0., [](const double sum, const RouteLeg &leg) {
             return sum + leg.duration;
         });
+    auto weight = std::accumulate(
+        route_legs.begin(), route_legs.end(), 0., [](const double sum, const RouteLeg &leg) {
+            return sum + leg.weight;
+        });
 
-    return Route{duration, distance};
+    return Route{distance, duration, weight};
 }
 
 } // namespace guidance

src/extractor/guidance/turn_discovery.cpp

@@ -41,7 +41,7 @@ bool findPreviousIntersection(const NodeID node_v,
      * PREVIOUS_ID. To verify that find, we check the intersection using our PREVIOUS_ID candidate
      * to check the intersection at NODE for via_edge
      */
-    const constexpr double COMBINE_WEIGHT_CUTOFF = 30;
+    const constexpr double COMBINE_DISTANCE_CUTOFF = 30;
 
     const auto coordinate_extractor = intersection_generator.GetCoordinateExtractor();
     const auto coordinates_along_via_edge =
@@ -53,7 +53,7 @@ bool findPreviousIntersection(const NodeID node_v,
 
     // we check if via-edge is too short. In this case the previous turn cannot influence the turn
     // at via_edge and the intersection at NODE_W
-    if (via_edge_length > COMBINE_WEIGHT_CUTOFF)
+    if (via_edge_length > COMBINE_DISTANCE_CUTOFF)
         return false;
 
     // Node -> Via_Edge -> Intersection[0 == UTURN] -> reverse_of(via_edge) -> Intersection at

src/extractor/scripting_environment_lua.cpp

@@ -382,9 +382,6 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
                                                       "duration",
                                                       &InternalExtractorEdge::duration_data);
 
-    // context.state.new_usertype<InternalExtractorEdge::WeightData>(
-    //     "WeightData", "weight", &InternalExtractorEdge::WeightData::weight_data);
-
     context.state.new_usertype<ExternalMemoryNode>("EdgeTarget",
                                                    "lon",
                                                    &lonToDouble<ExternalMemoryNode>,

unit_tests/library/route.cpp

@@ -50,12 +50,14 @@ BOOST_AUTO_TEST_CASE(test_route_same_coordinates_fixture)
           json::Array{{json::Object{
               {{"distance", 0.},
                {"duration", 0.},
+               {"weight", 0.},
                {"weight_name", "duration"},
                {"geometry", "yw_jGupkl@??"},
                {"legs",
                 json::Array{{json::Object{
                     {{"distance", 0.},
                      {"duration", 0.},
+                     {"weight", 0.},
                      {"summary", "Boulevard du Larvotto"},
                      {"steps",
                       json::Array{{{json::Object{{{"duration", 0.},