Making the turn function more flexible #4789

chaupow · 2018-01-11T23:47:46Z

Issue

Targeting issue #4775
In short: The goal is to make the turn function in lua profiles more flexible and powerful by exposing more information to it.

Tasklist

Complete todo list in Implement more flexible turn weight function #4775
CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

Requirements / Relations

maybe none

emiltin · 2018-01-16T07:25:34Z

include/extractor/extraction_turn.hpp

-                    bool is_link, int number_of_lanes, int highway_turn_classification, int access_turn_classification, int speed)
+                    bool is_link, int number_of_lanes,
+                    int highway_turn_classification,
+                    int access_turn_classification, int speed, bool is_incoming,


i think you also need incoming/outgoing status for other modes. e.g for bicycle routing you also need information about car traffic

Thanks! I'll target that in another issue. I'd prefer getting the changes ready and see how these changes make a difference in better ETA & route planning & extraction memory usage peak. But your comment is well noted and will not be forgotten! :)

oxidase

Looks good to me, but some changes are needed as PR adds information about connected roads into ExtractionTurn and this will cause a merge conflict with splitting of EBGF and guidance code.

oxidase · 2018-01-18T07:48:52Z

docs/profiles.md

-target_mode          | Read        | Enum    | Travel mode after the turn. Defined in `include/extractor/travel_mode.hpp`
+Attribute                          | Read/write?   | Type      | Notes
+---------------------              | ------------- | --------- | ------------------------------------------------------
+angle                              | Read          | Float     | Angle of turn in degrees (`0-360`: `0`=u-turn, `180`=straight on)


Angle in ExtractionTurn is initialized with 180. - angle: 0 is straight on, positive values for right turns, negative values for left turns, 180 is a U-turn

oxidase · 2018-01-18T08:46:57Z

features/options/extract/turn_function.feature

+Feature: Turn Function Information
+
+    Scenario: Turns should have correct information of source and target
+        Given the profile file


Test profile with printing can be moved to a background section and removed from scenarios. Profile customization also can be placed directly in the feature file as: ``` Background: Given the profile file """ functions = require('car')
function test_setup()
profile = functions.setup()
profile.highway_turn_classification = {
['motorway'] = 4,
['motorway_link'] = 4,
['trunk'] = 4,
['trunk_link'] = 4,
['primary'] = 4,
['primary_link'] = 4,
['secondary'] = 3,
['secondary_link'] = 3,
['tertiary'] = 2,
['tertiary_link'] = 2,
['residential'] = 1,
['living_street'] = 1,
}

profile.access_turn_classification = {
['discouraged'] = 1;
['permissive'] = 1;
['private'] = 1;
['customers'] = 1;
['dismount'] = 1;
}
return profile
end

function turn_leg_string (leg)
return 'speed: ' .. tostring(leg.speed)
.. ', is_incoming: ' .. tostring(leg.is_incoming)
.. ', is_outgoing: ' .. tostring(leg.is_outgoing)
.. ', highway_turn_classification: ' .. tostring(leg.highway_turn_classification)
.. ', access_turn_classification: ' .. tostring(leg.access_turn_classification)
end

function print_turn (profile, turn)
print ('source_restricted ' .. string.format("%s", tostring(turn.source_restricted)))
print ('source_is_motorway ' .. string.format("%s", tostring(turn.source_is_motorway)))
print ('source_is_link ' .. string.format("%s", tostring(turn.source_is_link)))
print ('source_number_of_lanes ' .. string.format("%s", tostring(turn.source_number_of_lanes)))
print ('source_highway_turn_classification ' .. string.format("%s", tostring(turn.source_highway_turn_classification)))
print ('source_access_turn_classification ' .. string.format("%s", tostring(turn.source_access_turn_classification)))
print ('source_speed ' .. string.format("%s", tostring(turn.source_speed)))

print ('target_restricted ' .. string.format("%s", tostring(turn.target_restricted)))
print ('target_is_motorway ' .. string.format("%s", tostring(turn.target_is_motorway)))
print ('target_is_link ' .. string.format("%s", tostring(turn.target_is_link)))
print ('target_number_of_lanes ' .. string.format("%s", tostring(turn.target_number_of_lanes)))
print ('target_highway_turn_classification ' .. string.format("%s", tostring(turn.target_highway_turn_classification)))
print ('target_access_turn_classification ' .. string.format("%s", tostring(turn.target_access_turn_classification)))
print ('target_speed ' .. string.format("%s", tostring(turn.target_speed)))

print ('number_of_roads ' .. string.format("%s", tostring(turn.number_of_roads)))
if not turn.is_u_turn then
for roadCount, road in ipairs(turn.roads_on_the_right) do
print('roads_on_the_right [' .. tostring(roadCount) .. '] ' .. turn_leg_string(road))
end

for roadCount, road in ipairs(turn.roads_on_the_left) do print('roads_on_the_left [' .. tostring(roadCount) .. '] ' .. turn_leg_string(road)) end

end
end

return {
setup = test_setup,
process_way = functions.process_way,
process_node = functions.process_node,
process_turn = print_turn
}
"""

</details>

Done thanks 👍

oxidase · 2018-01-18T08:57:16Z

profiles/feature/car_turnbot.lua

@@ -0,0 +1,483 @@
+-- Adjusted car profile with print output for testing


The file can be easily out of sync with the original car profile. I would suggest to load car profile functions and set testing functions directly in the feature file.

oxidase · 2018-01-18T09:10:50Z

include/extractor/node_based_edge.hpp

@@ -27,6 +27,9 @@ struct NodeBasedEdgeClassification
    std::uint8_t startpoint : 1;                      // 1
    std::uint8_t restricted : 1;                      // 1
    guidance::RoadClassification road_classification; // 16 2
+    std::uint8_t highway_turn_classification : 4;     // 4
+    std::uint8_t access_turn_classification : 4;      // 4
+    std::uint8_t speed;                               // 8


speed can be moved to NodeBasedEdge and grouped with weight and duration

I tried calculating speed by edge length and duration. Many calculated speeds are within 10% of the actual speed. Some are not. Here is an extract of the differences running on Berlin:

calculated speed 50.000792 real speed 56 calculated speed 41.707704 real speed 48 calculated speed 41.707704 real speed 48 calculated speed 41.707704 real speed 48 calculated speed 30.748476 real speed 40 calculated speed 30.748476 real speed 40 calculated speed 32.505609 real speed 40 calculated speed 32.505609 real speed 40 calculated speed 30.754826 real speed 48 calculated speed 30.754826 real speed 48 calculated speed 30.754826 real speed 48 calculated speed 31.699496 real speed 40 calculated speed 31.699496 real speed 40 calculated speed 19.930782 real speed 25 calculated speed 19.930782 real speed 25 calculated speed 26.538870 real speed 55 calculated speed 26.538870 real speed 55 calculated speed 26.538870 real speed 55 calculated speed 19.930782 real speed 25

in favour of using less memory, @oxidase do you think this is preferable?

@chaupow both speeds values have different meaning. For example, an NBG edge from https://www.openstreetmap.org/node/5339762636 to https://www.openstreetmap.org/node/1736058469 consists of two segments:

a part of https://www.openstreetmap.org/way/27801361 with profile speed 40 kmh (maxspeed=50)

a part of https://www.openstreetmap.org/way/168945697 with profile speed 24 kmh (maxspeed=30)

The speed value of the NBG edge is 40 kmh as the value of the first smaller segment.

The computed speed value is an average that also includes traffic light penalties.
In the example, the average speed is 26.44 kmh without traffic light penalty and 19.39 kmh with 2 seconds traffic light penalty.

I think average values are preferable over speed values of single segments.

I think average values are preferable over speed values of single segments.

Agreed! Done thanks 👍

oxidase · 2018-01-18T09:12:47Z

src/extractor/extractor_callbacks.cpp

@@ -405,6 +405,9 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
                                                                  forward_classes,
                                                                  parsed_way.forward_travel_mode,
                                                                  parsed_way.is_left_hand_driving});
+
+        std::uint8_t speed = parsed_way.forward_speed > 255 ? 255 : parsed_way.forward_speed;


here can be speed = std::min(parsed_way.forward_speed, 255)

oxidase · 2018-01-18T09:18:00Z

src/extractor/edge_based_graph_factory.cpp

+                    return road.eid == turn.eid;
+                });
+
+            // if the turn is a u turn, store all information in road_legs_on_the_right


Here can be an edge case for U-turns in left-driving countries.

Do you think I should change this in left-driving countries?

I wonder whether it is important whether roads are on the left or right when you do a u turn and want to assign turn weights for u turns. Maybe it doesn't really matter here?

agree, it should not matter, but would good to mention u-turns in the description of ExtractionTurn:roads_on_the_right property

Agreed. Mentioned https://github.com/Project-OSRM/osrm-backend/pull/4789/files#diff-fdaae96cd1c2c7521458af30135c4d77R235 and here https://github.com/Project-OSRM/osrm-backend/pull/4789/files#diff-fdaae96cd1c2c7521458af30135c4d77R256

oxidase · 2018-01-18T10:03:06Z

src/extractor/edge_based_graph_factory.cpp

@@ -562,7 +562,8 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges(
            const auto node_based_edge_to,
            const auto incoming_bearing,
            const auto &turn,
-            const auto entry_class_id) {
+            const auto entry_class_id,
+            const auto &intersection) {


This change conflicts with #4674 which removes guidance intersections analysis code from the edge-based-graph factory. To resolve the conflict a convertToIntersectionView call must be included back in #4674 and intersection_view must be used instead of intersection.

I would suggest to move initialization of road_legs_on_the_right and road_legs_on_the_left vectors out of the lambda and split intersection into two arrays after lines

osrm-backend/src/extractor/edge_based_graph_factory.cpp

Lines 860 to 865 in 8778c86

const auto turn =

std::find_if(intersection.begin(),

intersection.end(),

[edge = outgoing_edge.edge](const auto &road) {

return road.eid == edge;

});

To resolve the conflict a convertToIntersectionView call must be included back in #4674

How should we approach this. Are you going to change it in #4674 or should I make the changes?

oxidase · 2018-01-18T10:54:34Z

profiles/lib/way_handlers.lua

+  local access = way:get_value_by_key("access")
+
+  if profile.highway_turn_classification[highway] then
+    assert(profile.highway_turn_classification[highway] < 16, "highway_turn_classification must be smaller than 16")


Asserts can can be directly checked before assignments below.

oxidase · 2018-01-18T11:02:42Z

features/support/hooks.js

@@ -50,6 +50,9 @@ module.exports = function () {
            .defer(mkdirp, logDir)
            .defer(rimraf, this.scenarioLogFile)
            .awaitAll(callback);
+        // Question @review is this line nice to have here (commented, but ready to use and uncomment if needed?)
+        // i think this is super useful and would love to have it here
+        // console.log("  Writing logging output to " + this.scenarioLogFile)


oxidase · 2018-01-18T11:10:08Z

features/options/extract/turn_function.feature

+        And stdout should contain "source_number_of_lanes 3"
+        And stdout should contain "target_is_motorway false"
+        And stdout should contain "target_is_link true"
+        # Question @review: should number_of_lanes when untagged be 0 or 1?


👍 for using 0 number of lanes when untagged, otherwise number_of_lanes=1 would mean both untagged lanes and one lane cases.

chaupow · 2018-01-22T20:08:46Z

I compared RAM peak usage during osrm-extract of this PR and current master when extracting california:

master uses 4.59GB
this PR uses 4.57GB

… speed to the lua profile turn function

…duration instead

…e_edge

oxidase

Looks good!

chaupow self-assigned this Jan 11, 2018

chaupow added the Work In Progress label Jan 11, 2018

chaupow mentioned this pull request Jan 11, 2018

Implement more flexible turn weight function #4775

Closed

15 tasks

chaupow force-pushed the feature/flexible_turn_func branch from 6b7dcf3 to a09d6ad Compare January 15, 2018 09:49

emiltin reviewed Jan 16, 2018

View reviewed changes

chaupow force-pushed the feature/flexible_turn_func branch from 48e71da to 8778c86 Compare January 17, 2018 15:17

chaupow added Review and removed Work In Progress labels Jan 17, 2018

chaupow requested a review from oxidase January 17, 2018 16:36

oxidase requested changes Jan 18, 2018

View reviewed changes

Chau Nguyen and others added 19 commits January 23, 2018 15:12

init commit

9792493

prevent breaking API change

9450f13

set and store highway and access classification for the turn function

6775d86

expose highway turn classification and access turn classification and…

2a29c64

… speed to the lua profile turn function

expose whether connection road at turn is incoming or outgoing

329187c

small cleanup

2fd9442

add lua tests for exposed information to turn function

76224ef

clang-format

096f86d

remove mode in connected roads

9208fd7

check lane numbers in tests

69758c7

update docs about attributes in process_turn

5767da9

add turn_classification info to docs

ac2846d

small nitpicks and cleanup

3651f6b

review adjustments

0ad7169

remove speed from node based edge and calculate from edge length and …

09ff6ef

…duration instead

calculate speed by edge length and duration

73c72ff

split roads on the left and roads on the right before running generat…

62c089e

…e_edge

comment on u turns and empty roads_on_the_left

d1f3146

move profile to background

013d951

chaupow added 13 commits January 23, 2018 15:12

move impl or setting info for connected edges

0611e55

clang-format

e51abc8

dirty commit! checking failing assertion

3ee0d22

remove assertions for edge cases

d339178

less assertions

430859c

strange clang-format glitch

edc30b0

adding warning if uturn and intersection dont match

7898ccb

handle u turns that do not turn into intersection[0]

56754ea

attempt to make travis happy

939ac0f

try again

989f633

fix mismatch of arguments

106874d

add changelog

98ca829

add changelog

271915e

chaupow force-pushed the feature/flexible_turn_func branch 2 times, most recently from d0354d4 to d239814 Compare January 24, 2018 16:11

chaupow added 5 commits January 24, 2018 13:57

fix changelog

12629cd

clang-format

dd61ce8

fix typo and used std::transform instead of loop

a3de95c

split OSM link generation in an accessible coordinate function

9eb80ca

guard transform when begin iterator is bigger than end iterator

5550f76

chaupow force-pushed the feature/flexible_turn_func branch from 49efaeb to 5550f76 Compare January 24, 2018 18:57

oxidase approved these changes Jan 24, 2018

View reviewed changes

chaupow merged commit 61e06fc into master Jan 24, 2018

chaupow deleted the feature/flexible_turn_func branch January 24, 2018 20:39

chaupow mentioned this pull request Feb 6, 2018

Add road priority class to turn function #4858

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making the turn function more flexible #4789

Making the turn function more flexible #4789

chaupow commented Jan 11, 2018 •

edited

Loading

emiltin Jan 16, 2018

chaupow Jan 16, 2018

oxidase left a comment

oxidase Jan 18, 2018

oxidase Jan 18, 2018 •

edited by chaupow

Loading

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018 •

edited

Loading

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018

chaupow Jan 18, 2018 •

edited

Loading

oxidase Jan 18, 2018

chaupow Jan 18, 2018

oxidase Jan 18, 2018

oxidase Jan 18, 2018

chaupow commented Jan 22, 2018

oxidase left a comment

		@@ -0,0 +1,483 @@
		-- Adjusted car profile with print output for testing

	const auto turn =
	std::find_if(intersection.begin(),
	intersection.end(),
	[edge = outgoing_edge.edge](const auto &road) {
	return road.eid == edge;
	});

Making the turn function more flexible #4789

Making the turn function more flexible #4789

Conversation

chaupow commented Jan 11, 2018 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase Jan 18, 2018 • edited by chaupow Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow commented Jan 22, 2018

oxidase left a comment

Choose a reason for hiding this comment

chaupow commented Jan 11, 2018 •

edited

Loading

oxidase Jan 18, 2018 •

edited by chaupow

Loading

oxidase Jan 18, 2018 •

edited

Loading

chaupow Jan 18, 2018 •

edited

Loading