From ad594cb2e408f30e3ee793bdfe4504c1e71d2e3a Mon Sep 17 00:00:00 2001 From: Michael Krasnyk Date: Thu, 26 Jan 2017 17:28:27 +0100 Subject: [PATCH] Update review findings --- CHANGELOG.md | 14 +++--- docs/http.md | 44 ++++++++++++------- features/{testbot => car}/side_bias.feature | 0 features/options/profiles/version0.feature | 8 ++-- features/testbot/weight.feature | 39 ++++------------ include/engine/guidance/assemble_geometry.hpp | 18 ++++---- include/engine/guidance/assemble_leg.hpp | 12 ++++- include/engine/guidance/route.hpp | 3 +- include/engine/guidance/route_leg.hpp | 3 +- include/extractor/profile_properties.hpp | 5 +-- src/engine/api/json_factory.cpp | 14 +++--- src/engine/guidance/assemble_route.cpp | 6 ++- src/extractor/guidance/turn_discovery.cpp | 4 +- src/extractor/scripting_environment_lua.cpp | 3 -- unit_tests/library/route.cpp | 2 + 15 files changed, 90 insertions(+), 85 deletions(-) rename features/{testbot => car}/side_bias.feature (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0f1d042a..a426cf398 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/http.md b/docs/http.md index e908f2940..d8103a2ed 100644 --- a/docs/http.md +++ b/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 | -| false | `undefined` | +| true | An `Annotation` object containing node ids, durations distances and | +| 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 diff --git a/features/testbot/side_bias.feature b/features/car/side_bias.feature similarity index 100% rename from features/testbot/side_bias.feature rename to features/car/side_bias.feature diff --git a/features/options/profiles/version0.feature b/features/options/profiles/version0.feature index fe47d8b08..5884b48b1 100644 --- a/features/options/profiles/version0.feature +++ b/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 | d | ac,cd,cd | 16.7s | + | a | b | ac,cb,cb | 24.2s | + | a | d | ac,cd,cd | 24.2s | | a | e | ac,ce,ce | 20s | diff --git a/features/testbot/weight.feature b/features/testbot/weight.feature index 43acae992..fc96fd153 100644 --- a/features/testbot/weight.feature +++ b/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 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 + Scenario: Start and target on the same and adjacent edge 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 | - | s,t | abc,abc | 30m,0m | 31,0 | 3.1s,0s | 31:3.1:30.026527:0 | - | t,s | abc,abc | 30m,0m | 31,0 | 3.1s,0s | 31:3.1:30.026527:0 | + | waypoints | route | distances | weights | times | annotation | + | s,t | abc,abc | 20m,0m | 2.1,0 | 2.1s,0s | 3:3:20.017685: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 diff --git a/include/engine/guidance/assemble_geometry.hpp b/include/engine/guidance/assemble_geometry.hpp index 337d1e81b..da86a96ee 100644 --- a/include/engine/guidance/assemble_geometry.hpp +++ b/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 forward_datasources = facade.GetUncompressedForwardDatasources(target_node.packed_geometry_id); - // FIXME this is wrong. We need to check for traversal direction here - // and for the case of a local path (target and source on the same edge) - geometry.annotations.emplace_back( - LegGeometry::Annotation{current_distance, - target_node.forward_duration / 10., - target_node.forward_weight / 10., - forward_datasources[target_node.fwd_segment_position]}); + // FIXME if source and target phantoms are on the same segment then duration and weight + // will be from one projected point till end of segment + // testbot/weight.feature:Start and target on the same and adjacent edge + geometry.annotations.emplace_back(LegGeometry::Annotation{ + current_distance, + (reversed_target ? target_node.reverse_duration : target_node.forward_duration) / 10., + (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); diff --git a/include/engine/guidance/assemble_leg.hpp b/include/engine/guidance/assemble_leg.hpp index c523c6346..233ef4330 100644 --- a/include/engine/guidance/assemble_leg.hpp +++ b/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 diff --git a/include/engine/guidance/route.hpp b/include/engine/guidance/route.hpp index bde2d5fca..461da4cc7 100644 --- a/include/engine/guidance/route.hpp +++ b/include/engine/guidance/route.hpp @@ -10,8 +10,9 @@ namespace guidance struct Route { - double duration; double distance; + double duration; + double weight; }; } } diff --git a/include/engine/guidance/route_leg.hpp b/include/engine/guidance/route_leg.hpp index 5ecca4faa..c9f4e2ab5 100644 --- a/include/engine/guidance/route_leg.hpp +++ b/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 steps; }; diff --git a/include/extractor/profile_properties.hpp b/include/extractor/profile_properties.hpp index 4ba616095..c560c59e9 100644 --- a/include/extractor/profile_properties.hpp +++ b/include/extractor/profile_properties.hpp @@ -14,10 +14,7 @@ const constexpr auto DEFAULT_MAX_SPEED = 180 / 3.6; // 180kmph -> m/s struct ProfileProperties { - enum - { - MAX_WEIGHT_NAME_LENGTH = 255 - }; + static constexpr int MAX_WEIGHT_NAME_LENGTH = 255; ProfileProperties() : traffic_signal_penalty(0), u_turn_penalty(0), diff --git a/src/engine/api/json_factory.cpp b/src/engine/api/json_factory.cpp index 79a2a693e..442f8c522 100644 --- a/src/engine/api/json_factory.cpp +++ b/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["weight"] = step.weight; // We should round to weight_precision here + route_step.values["duration"] = step.duration; + 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["duration"] = std::round(leg.duration * 10) / 10.; + route_leg.values["distance"] = leg.distance; + 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; diff --git a/src/engine/guidance/assemble_route.cpp b/src/engine/guidance/assemble_route.cpp index c30d821ff..a2b4fc444 100644 --- a/src/engine/guidance/assemble_route.cpp +++ b/src/engine/guidance/assemble_route.cpp @@ -19,8 +19,12 @@ Route assembleRoute(const std::vector &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 diff --git a/src/extractor/guidance/turn_discovery.cpp b/src/extractor/guidance/turn_discovery.cpp index 4b603db1d..fc8f868d9 100644 --- a/src/extractor/guidance/turn_discovery.cpp +++ b/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 diff --git a/src/extractor/scripting_environment_lua.cpp b/src/extractor/scripting_environment_lua.cpp index 5067837b9..2464c4e3e 100644 --- a/src/extractor/scripting_environment_lua.cpp +++ b/src/extractor/scripting_environment_lua.cpp @@ -382,9 +382,6 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context) "duration", &InternalExtractorEdge::duration_data); - // context.state.new_usertype( - // "WeightData", "weight", &InternalExtractorEdge::WeightData::weight_data); - context.state.new_usertype("EdgeTarget", "lon", &lonToDouble, diff --git a/unit_tests/library/route.cpp b/unit_tests/library/route.cpp index e5400b524..bcbfb55c8 100644 --- a/unit_tests/library/route.cpp +++ b/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.},