Location-dependent left hand driving flag #4415

oxidase · 2017-08-16T11:44:36Z

Issue

PR adds location-dependent tags and adds parsing driving_side tag.
Location-dependent data is taken from a GeoJSON file (file name is an argument of a new extractor option --location-dependent-data) and provided as an optional argument location_data of function process_way(profile, way, result, location_data). At the moment to use location-dependent data OSM files must be preprocessed with osmium add-locations-to-ways.

Left-hand driving flag is added to node-based-graph edges, edge-based-graph nodes and used in guidance pre-processing and in geospatial queries.

Some points to be discussed:

MultiPolygon support?
Use a nodes locations cache. This will require an implementation equivalent to add-locations-to-ways before parsing ways Country-aware way function #4167
Merging conflicting tags at different locations of a way or multiple-defined at one node. At the moment a way is represented by a single node node and no dedicated merging is implemented.
is_left_hand_driving flag is indexed by EBG node ids, but it can be indexed by geometry ids. This will require an additional file that is indexed by geometry ids, but contains geometry non-segments data.
vector<bool> is stored as 8-bit per 1-bit value, is it a new issue?

Tasklist

update relevant Wiki pages
add regression / cucumber cases (see docs/testing.md)
check with real OSM data
check edge cases near signs similar https://wiki.openstreetmap.org/wiki/File:Fari-Wechsel.jpg
review
adjust for comments

Requirements / Relations

Partially supersedes #4167

emiltin · 2017-08-16T12:20:35Z

do i understand it correctly, that this adds a general ability to use location-based data when preprocessing osm data, not just data about left/right side driving?

can the features in the geojson file overlap?

oxidase · 2017-08-16T13:06:58Z

@emiltin yes, it is not limited to left-hand driving tags and GeoJSON can have any data that will be forwarded into parse_way function, but I did not checked yet other tags. The next step will be to check driving_side together with some other tags.

Polygons with non-intersecting tags sets can overlap. If some overlapping polygons have the same tag then last-value-wins merging in the rtree indexing order will be used. Basically it just a

osrm-backend/src/extractor/location_dependent_data.cpp

Line 133 in de0a73d

boost::apply_visitor(table_setter(table, key_value.first), key_value.second);

so no dedicated conflict resolution yet.

emiltin · 2017-08-16T13:10:50Z

ok, thanks for clarifying. this seems very useful. it will cover a different set of use-cases related to location data than the raster-based approach already available.

TheMarex · 2017-08-17T09:16:29Z

vector is stored as 8-bit per 1-bit value, is it a new issue?

You mean stored like that on disk? As far as I remember there were some issues around using out default memory-dump based serialization. If you see a fix for that, go for it. 👍

Merging conflicting tags at different locations of a way or multiple-defined at one node.

This is mainly an issue while crossing country polygon borders?

Location-dependent data is taken from a GeoJSON file (file name is an argument of a new extractor option --location-dependent-data)

Do you think it would be possible to specify this from within the profile? Do we want to allow for multiple of those sources? This could work similar to how we use raster-data, in this case it would just be polygon based data.

oxidase · 2017-08-17T15:03:10Z

@TheMarex i added a vector serializer

This is mainly an issue while crossing country polygon borders?

yes, maybe for driving side flag this would make no sense (an edge case https://www.openstreetmap.org/export#map=17/22.52030/114.07047), but in many cases ways can cross polygons with the same tags, but different values.

Do you think it would be possible to specify this from within the profile? Do we want to allow for multiple of those sources? This could work similar to how we use raster-data, in this case it would just be polygon based data.

yes, it is possible to specify in profiles, but data must loaded before creating contexts, otherwise many copies of rtrees will be multiplied for every thread-local context.

Multiple of sources are also possible, in principle we can index data in a single rtree, but it will bring the merging question to discussion, i would go first towards a single GeoJSON file with MultiPolygon support and Sol2ScriptingEnvironment as the sole owner of a constant single copy of data.

TheMarex · 2017-08-18T16:13:51Z

yes, it is possible to specify in profiles, but data must loaded before creating contexts,

We could call the initialization in the setup functions. In setup we would call a function in C++ land that would check if the specific GeoJSON file has already been loaded. Data would be owned by SolScriptingEnvironment (not SolScriptingContext) so one global R-Tree per file and many threads access. I take it lookups in the RTree would be thread-safe?

TheMarex

Reading the code, I think it might make sense to have a CLI parameter for the location data after all, since it is optional. I will leave this up to you to decide.

What is the plan with relation to the node-location? Do we want to wait for that PR or should be tell users to use the osmium re-writer?

Thanks for pushing this forward. 👍

TheMarex · 2017-08-18T16:15:00Z

CHANGELOG.md

@@ -18,6 +19,10 @@
      - Fix a pre-processing bug where incorrect directions could be issued when two turns would have similar instructions and we tried to give them distinct values (https://github.com/Project-OSRM/osrm-backend/pull/4375)
      - The entry bearing for correct the cardinality of a direction value (https://github.com/Project-OSRM/osrm-backend/pull/4353
      - Change timezones in West Africa to the WAT zone so they're recognized on the Windows platform
+  - Profiles


Changelog entries need to be moved to 5.12

TheMarex · 2017-08-18T16:16:27Z

CHANGELOG.md

@@ -6,6 +6,7 @@
      - BREAKING: Traffic signals will no longer be represented as turns internally. This requires re-processing of data but enables via-way turn restrictions across highway=traffic_signals
      - Additional checks for empty segments when loading traffic data files
      - Tunes the constants for turns in sharp curves just a tiny bit to circumvent a mix-up in fork directions at a specific intersection (https://github.com/Project-OSRM/osrm-backend/issues/4331)
+      - BREAKING: added `is_left_hand_driving` vector to `.ebg_nodes` file


I think we might want to just say something like File format for .ebg_nodes not compatible with 5.11.

TheMarex · 2017-08-18T16:21:18Z

features/car/side_bias.feature

+            """
+        And the ways with locations
+            | nodes | driving_side |
+            | ab    | right        |


This tests the driving_side tag on ways in additional to the tag coming from outside data? Do you think in the future we might want to parse this data automatically from the OSM relations?

there some residential roads, service roads, private ways or u-turns. Yes, we can add in future such handling, atm tags on ways have higher priority than default values.

TheMarex · 2017-08-18T16:35:24Z

include/extractor/extraction_way.hpp

@@ -114,6 +115,7 @@ struct ExtractionWay
    bool is_startpoint : 1;
    bool forward_restricted : 1;
    bool backward_restricted : 1;
+    bool is_left_hand_driving : 1;


Just a thought: Maybe we should add some bool unused : 2 to make it explicit how many bits are still free.

Unrelated to your change but looking at this again, didn't VSC have problem with packing bools? We might want to change this to std::uint8_t.

VCS have other rules for padding, but here it should be similar to gcc packing

after some hours of debugging i think i will keep bool type here, because of some strange gcc behavior here: with std::uint8_t setting is_startpoint also sets forward_restricted and backward_restricted. it can be either ub or gcc optimization issue

TheMarex · 2017-08-18T16:38:21Z

include/extractor/profile_properties.hpp

@@ -103,7 +103,7 @@ struct ProfileProperties
    bool continue_straight_at_waypoint;
    //! flag used for restriction parser (e.g. used for the walk profile)
    bool use_turn_restrictions;
-    bool left_hand_driving;
+    bool left_hand_driving; // DEPRECATED: property value is local to edges from API version 2


What will happen if a user sets this value? Will it be used as the default for per-edge values?

TheMarex · 2017-08-18T16:45:36Z

include/storage/serialization.hpp

@@ -114,44 +114,64 @@ template <typename T> void write(io::FileWriter &writer, const util::vector_view
    writer.WriteFrom(data.data(), count);
 }

+template <typename T> inline unsigned char packBits(T &data, std::size_t index, std::size_t count)


data can be a const-ref.

TheMarex · 2017-08-18T16:58:44Z

src/extractor/location_dependent_data.cpp

+};
+}
+
+sol::table LocationDependentData::operator()(sol::state &state, const osmium::Way &way) const


Would it be possible to return a non-sol object, so we can keep this interface lua-agnostic? I think sol should be able to wrap an unordered_map automatically.

i would like to keep it as a shortcut to avoid creating unordered_map. It can be easily adjusted when it will be clear how conflicting tags must be merged

TheMarex · 2017-08-18T17:00:19Z

src/extractor/scripting_environment_lua.cpp

@@ -224,7 +225,7 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
                                                "interpolate",
                                                &RasterContainer::GetRasterInterpolateFromSource);

-    context.state.new_usertype<ProfileProperties>(
+    auto registration_ProfileProperties = context.state.new_usertype<ProfileProperties>(


Why do we need to save the return type of new_usertype here?

TheMarex · 2017-08-18T17:01:35Z

unit_tests/util/serialization.cpp

@@ -0,0 +1,38 @@
+#include "storage/serialization.hpp"


Good idea. We should add tests for the other serialization methods too at some point.

oxidase · 2017-08-21T17:23:23Z

@TheMarex please can you check the last changes?

I have tried to use real data with OSM administrative boundaries and results are very disappointing:
265 boost::geometry::within(point, polygons[v.second].first) calls increase OSM parsing from 30ms to 835 with just Thailand single border line.

EDIT: Before using location-dependent data, i think we need benchmarks of desired features and make an optimization of within or make better use of the r-tree data structure. ATM it is possible to use very simplified geometries without performance penalties.

oxidase · 2017-08-31T13:27:00Z

@TheMarex i have ported point-in-polygon check from osmium and added some unit tests to check correctness. The performance results are better than using boost:geometry::within but the price is additional memory usage to store segment bands. For berlin-latest with 690932 ways anf the Germany border parsing time changed from 5.2 to 40 seconds, that is ~ 100-200 microseconds per lookup that is still bad.

There are still some space for improvement because "bands" solution is a naive implementation of an interval tree with fixed-size bands. For OSM Germany border with latitudes from 47.2701 to 55.0992 with a band width 0.00056389 degrees the histogram of segments is

so the check performance will depend on node locations and at the southern border it will be 1000 times slower than at the northern one. I think it is ok to merge under assumption of simple polygons for the left-hand side driving checks, and performance can be improved later by using similar check with an interval tree.

Another huge performance improvement is to avoid using unordered_map merging because 54% out 59% of LocationDependentData::operator() time for berlin-latest extraction is spent at

osrm-backend/src/extractor/location_dependent_data.cpp

Line 204 in 116d7d2

result.insert(polygon_properties.begin(), polygon_properties.end());

But i don't know atm the best way how to deal with the issue: may be create Lua tables in every context in advance, don't merge tables in c++ and provide an optional table of tags tables (it is possible to add also relation tags or any other "extension" tags into that table). So it is mainly a Lua interface question.

TheMarex

I'm unclear on how the multi-file handling works, otherwise the code looks good.

However there is a big speedup potential for the point-in-polygon checks using quad-tree approximation (actually just removing most of the point-in-polygon checks).

TheMarex · 2017-08-31T20:41:32Z

src/extractor/location_dependent_data.cpp

+    }
+
+    // Create R-tree for bounding boxes of collected polygons
+    rtree = rtree_t(bounding_boxes);


Hrm I don't quite understand how this works with multiple input files: It seems to me on every call of this functions the rtree gets overwritten and all previous data is lost?

🤦‍♂️ will be fixed

TheMarex · 2017-08-31T20:48:26Z

src/extractor/scripting_environment_lua.cpp

+        }
+        else
+        {
+            way_function(profile_table, way, result, toLua(state, location_dependent_data(way)));


Does this add any overhead when not giving any location dependent data as input (e.g. no geojson file)?

empty data should handled in then branch when rtree is empty, and no optional arguments will be passed to the lua function

TheMarex · 2017-08-31T20:55:36Z

src/extractor/location_dependent_data.cpp

+    };
+
+    // Search the R-tree and collect a Lua table of tags that correspond to the location
+    rtree.query(boost::geometry::index::intersects(point) &&


Instead of saving each polygon in one bounding box you could try a quad tree approximation:

Divide the bounding box into 4 equal parts

For every new box check if its fully container, fully outside or intersects the boundary

If a box intersects the boundary: Continue dividing that box.

Stop when all boxes are either fully in or fully out, or you reached a minimum box size (like 10x10m)

Now your query can terminate immediately when it hits a fully in/fully out bounding box. You only need to do the expensive point-in-polygon check on the points that hit the boundary boxes.

daniel-j-h

I just stumbled upon some guidance profile decisions which depend on driving side here. I think it is missing from this changeset.

For some reason envelop = make_inverse<box_t>(); boost::geometry::expand(envelop, next); normalizes longitude to [-180,180]

oxidase · 2017-10-03T12:41:37Z

@TheMarex just checked the mean difference is 0.3 seconds or 5%

> t.test(d[d$V2=='no',1],d[d$V2=='yes',1])

	Welch Two Sample t-test

data:  d[d$V2 == "no", 1] and d[d$V2 == "yes", 1]
t = -4.4597, df = 197.97, p-value = 1.374e-05
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
 -0.4449259 -0.1720891
sample estimates:
mean of x mean of y 
 6.723882  7.032389

oxidase added the Review label Aug 16, 2017

oxidase force-pushed the feature/left-hand-driving branch from cdcc6a5 to de0a73d Compare August 16, 2017 12:50

oxidase force-pushed the feature/left-hand-driving branch from de0a73d to 1030510 Compare August 16, 2017 13:37

emiltin mentioned this pull request Aug 18, 2017

suffix/prefix in other languages #4423

Open

TheMarex approved these changes Aug 18, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Review labels Aug 18, 2017

oxidase added the Work In Progress label Aug 18, 2017

oxidase force-pushed the feature/left-hand-driving branch 3 times, most recently from d4f5c92 to be68c57 Compare August 21, 2017 14:46

oxidase removed the Work In Progress label Aug 21, 2017

TheMarex mentioned this pull request Aug 23, 2017

support relations in lua profiles #482

Closed

oxidase added the Work In Progress label Aug 30, 2017

oxidase force-pushed the feature/left-hand-driving branch 2 times, most recently from 5550b63 to 57e7819 Compare August 31, 2017 11:14

oxidase removed the Work In Progress label Aug 31, 2017

TheMarex requested changes Aug 31, 2017

View reviewed changes

1ec5 mentioned this pull request Sep 1, 2017

Roundabout icon assumes right-hand driving mapbox/mapbox-navigation-ios#575

Closed

daniel-j-h reviewed Sep 4, 2017

View reviewed changes

oxidase force-pushed the feature/left-hand-driving branch 2 times, most recently from 4dad3c3 to 480bc4a Compare September 21, 2017 13:21

oxidase added 16 commits October 3, 2017 13:16

Make class_names default initialized

9b9adac

Added bit packing for serialization of vector<bool>

08b2a6d

Allow multiple GeoJSON files

9d594e8

Add MultiPolygon support

8d5da52

Left-hand driving flag review updates

c023bf5

Add location_dependent_data unit tests

b646742

Port osmium point-in-polygon function

743c80b

Remove polygon copying overhead

74d61e5

Use correct bounding box

e8783c5

For some reason envelop = make_inverse<box_t>(); boost::geometry::expand(envelop, next); normalizes longitude to [-180,180]

Bump api_version to 3 in car.lua

ea05622

Add osmium locations cache

866d8c8

Restructure ParseOSMData method

8efcd89

Access to location dependent data in Lua via way:get_location_tags()

bf4a777

Change location data method to way:get_location_tags(key)

b2a99b7

Allow multiple GeoJSON files with locations data

b29445d

Don't use location cache if not needed

686588b

oxidase force-pushed the feature/left-hand-driving branch from e407aa3 to e59f95d Compare October 3, 2017 11:17

Add last location memoization in Lua context

d644586

oxidase force-pushed the feature/left-hand-driving branch from e59f95d to d644586 Compare October 3, 2017 12:18

oxidase added Ready To Merge and removed Review - In feedback labels Oct 3, 2017

miccolis mentioned this pull request Oct 3, 2017

Support Left-Sided Driving #2269

Closed

oxidase merged commit 11e7b6e into master Oct 4, 2017

oxidase deleted the feature/left-hand-driving branch October 4, 2017 08:03

This was referenced Oct 4, 2017

Update CHANGELOG.md for location-dependent data #4570

Merged

Country-aware way function #4167

Closed

Driving-side aware sliproads handler #4587

Closed

daniel-j-h mentioned this pull request Oct 27, 2017

use different profiles for different areas #333

Closed

1ec5 mentioned this pull request Dec 4, 2017

Add roadSide to RouteStep mapbox/mapbox-directions-swift#219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Location-dependent left hand driving flag #4415

Location-dependent left hand driving flag #4415

oxidase commented Aug 16, 2017 •

edited

Loading

emiltin commented Aug 16, 2017

oxidase commented Aug 16, 2017

emiltin commented Aug 16, 2017 •

edited

Loading

TheMarex commented Aug 17, 2017 •

edited

Loading

oxidase commented Aug 17, 2017

TheMarex commented Aug 18, 2017

TheMarex left a comment

TheMarex Aug 18, 2017

TheMarex Aug 18, 2017

TheMarex Aug 18, 2017

oxidase Aug 21, 2017

TheMarex Aug 18, 2017

oxidase Aug 21, 2017

oxidase Aug 21, 2017

TheMarex Aug 18, 2017

oxidase Aug 21, 2017

TheMarex Aug 18, 2017

TheMarex Aug 18, 2017

oxidase Aug 21, 2017

TheMarex Aug 18, 2017

TheMarex Aug 18, 2017

oxidase commented Aug 21, 2017 •

edited

Loading

oxidase commented Aug 31, 2017 •

edited

Loading

TheMarex left a comment

TheMarex Aug 31, 2017

oxidase Aug 31, 2017

TheMarex Aug 31, 2017

oxidase Aug 31, 2017

TheMarex Aug 31, 2017

daniel-j-h left a comment

oxidase commented Oct 3, 2017

Location-dependent left hand driving flag #4415

Location-dependent left hand driving flag #4415

Conversation

oxidase commented Aug 16, 2017 • edited Loading

Issue

Tasklist

Requirements / Relations

emiltin commented Aug 16, 2017

oxidase commented Aug 16, 2017

emiltin commented Aug 16, 2017 • edited Loading

TheMarex commented Aug 17, 2017 • edited Loading

oxidase commented Aug 17, 2017

TheMarex commented Aug 18, 2017

TheMarex left a 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

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

oxidase commented Aug 21, 2017 • edited Loading

oxidase commented Aug 31, 2017 • edited Loading

TheMarex left a 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

daniel-j-h left a comment

Choose a reason for hiding this comment

oxidase commented Oct 3, 2017

oxidase commented Aug 16, 2017 •

edited

Loading

emiltin commented Aug 16, 2017 •

edited

Loading

TheMarex commented Aug 17, 2017 •

edited

Loading

oxidase commented Aug 21, 2017 •

edited

Loading

oxidase commented Aug 31, 2017 •

edited

Loading