Work on support relations in lua profiles #4438

deniskoronchik · 2017-08-24T13:07:14Z

Issue

#482

Tasklist

Add process_relation function into lua
Add parameter relation_data to process_way and process_node
Process relations logic (C++)
Change lua API to 3, an support this
Automation tests of new LUA features
Update documentation

emiltin · 2017-08-24T13:58:27Z

very nice to see work on relation support :-)

TheMarex · 2017-08-30T18:21:43Z

src/extractor/scripting_environment_lua.cpp

@@ -284,7 +361,7 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
                                             "id",
                                             &osmium::Node::id,
                                             "version",
-                                             &osmium::Way::version);
+                                             &osmium::Node::version);


Good catch!

deniskoronchik · 2017-08-31T12:02:02Z

This version have some problems:

old restriction mechanism still using (need to replace it in future);
can pass just string values to process_relation result (maybe can be changed with boost::any)

daniel-j-h · 2017-08-31T21:32:57Z

Great to see this moving forward! As a first use-case we should pass on cardinal directions from the route relation onto it's containing way members.

For now we're good with concatenating the route's cardinal direction (North, South, ..) onto the way's ref property.

In future versions (probably v6) we might want to reconsider this and properly split it off into own properties: #3304.

oxidase

LGTM! Please add some feature tests with test profiles to show how data will look on Lua side.

oxidase · 2017-09-01T12:17:41Z

docs/profiles.md

+Argument | Description
+---------|-------------------------------------------------------
+profile  | The configuration table you returned in `setup`.
+node     | The input relation to process (read-only).


node -> relation

oxidase · 2017-09-01T12:19:44Z

include/extractor/extraction_relation.hpp

+namespace detail
+{
+
+inline const char * checkedString(const char * str)


is it used somewhere? pls remove if will not be used

oxidase · 2017-09-01T12:32:10Z

include/util/osm_id_typed.hpp

+namespace util
+{
+
+class OsmIDTyped


Just as an alternative you can use union as

union OsmIDTyped { OsmIDTyped(std::uint64_t id, std::uint8_t type) : detail{id, type} {} std::uint64_t hash; std::uint64_t GetID() const { return detail.id; } std::uint8_t GetType() const { return detail.type; } private: struct { std::uint64_t id : 56; std::uint8_t type; } detail; };

I've think about this. And decide that it's not normal for OSRM (haven't seen union in project). If it good, i can do that.

oxidase · 2017-09-01T12:57:51Z

src/extractor/extractor.cpp

            for (const auto &result : parsed_buffer->resulting_restrictions)
            {
                extractor_callbacks->ProcessRestriction(result);
            }
        });

+    /* Main trick that we can use the same pipeline. It just receive relation objects


Another trick is to use HOF that returns a function that captures a reader as

Reader reader1, reader2; auto make_buffer_reader = [](Reader& reader) { return [&reader]() { reader.bark(); }; }; make_buffer_reader(reader1)(); make_buffer_reader(reader2)();

and use on-stack allocated readers.

So pipelines will be

{ osmium::io::Reader reader(input_file, osmium::osm_entity_bits::relation,...); tbb::parallel_pipeline(tbb::task_scheduler_init::default_num_threads() * 1.5, make_buffer_reader(reader) & buffer_transform & buffer_storage_relation); } { osmium::io::Reader reader(input_file, osmium::osm_entity_bits::node | osmium::osm_entity_bits::way,,...); tbb::parallel_pipeline(tbb::task_scheduler_init::default_num_threads() * 1.5, make_buffer_reader(reader) & buffer_transform & buffer_storage); }

It also can be n

yeah, that also was an idea. But there are some code that request reader before: ed5e00f#diff-edf14d92e413123a2b940ab0b8491ffbR298

This approach is much cleaner for me. But it can be implemented if we will create another reader to get header info.

oxidase · 2017-09-01T13:04:35Z

src/extractor/scripting_environment_lua.cpp

+        osmium::item_type GetItemType() const { return item_type; }
+        osmium::object_id_type GetId() const { return id; }
+
+        util::OsmIDTyped ref() const { return util::OsmIDTyped(id, std::uint8_t(item_type)); }


conversion of osmium::item_type to std::uint8_t must be a hidden in OsmIDTyped

You talk about store std::uint8_t instead of osmium::item_type or something else?

oxidase · 2017-09-01T13:21:53Z

src/extractor/scripting_environment_lua.cpp

+            id = member.ref();
+        }
+
+        const char* GetRole() const { return role.c_str(); }


you don't need accessor function with pubic data members , it is better to pass references to sol and set r/o access as sol::readonly( &RelationMemberWrap::role )

Then it would be a problem:

relation:id() -- used for nodes and ways before member:role() -- will be similar -- instead of member:role

oxidase · 2017-09-01T13:27:38Z

src/extractor/scripting_environment_lua.cpp

+                                                 "members", [](const osmium::Relation &rel)
+    {
+        std::vector<RelationMemberWrap> members(rel.members().size());
+        size_t i = 0;


std::copy(...) of rel.members() to memeber?

oxidase · 2017-09-01T13:31:37Z

src/extractor/scripting_environment_lua.cpp

@@ -276,6 +288,71 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
        "version",
        &osmium::Way::version);

+    struct RelationMemberWrap


Why not to expose osmium::RelationMember directly to avoid copying role strings?

Because it has deleted a lot of constructors and operators. So it doesn't compiles with sol.
I've tried for example:

context.state.new_usertype<osmium::Relation>("Relation", "get_value_by_key", &get_value_by_key<osmium::Relation>, "id", &osmium::Relation::id, "version", &osmium::Relation::version, "members", [](const osmium::Relation &rel) { return sol::as_table(rel.members()); });

The wrapper can be removed return sol::as_table(rel.members()); should work well

oxidase · 2017-09-01T13:39:43Z

third_party/libosmium/test/t/io/data-cr.opl

@@ -1 +1 @@
-n1 v1 dV c1 t2014-01-01T00:00:00Z i1 utest T x1.02 y1.02


This must be reverted

Can't do that until will be fixed in master. It create this one automatically with git on any rebase. So moved it to a separate commit, that will be removed on fixed master rebase

oxidase · 2017-09-01T13:49:46Z

docs/profiles.md

@@ -130,7 +130,7 @@ Attribute       | Type    | Notes
 barrier         | Boolean | Is it an impassable barrier?
 traffic_lights  | Boolean | Is it a traffic light (incurs delay in `process_turn`)?

-## process_way(profile, way, result)
+### process_way(profile, way, result)


form the code process_way has the 4th relations argument but here it is not mentioned

Should it be may be optional ? I feel we need some sort of sync here with location-dependent tags, that I would like to see optional. So we can add an additional positional argument that will hold a table with optional data items.
This will save from bumping api version every time when a new piece of data will be added to lua callback functions.

deniskoronchik · 2017-09-01T14:56:14Z

@oxidase have test features of api_version 3: https://github.com/Project-OSRM/osrm-backend/pull/4438/files#diff-c86e1caf4e4a3c911f176b8f8b1c0d87

Or need more and if yes, then where to place them?

oxidase · 2017-09-01T16:07:53Z

@deniskoronchik test looks good, somehow i missed it during review

danpat

Basic approach LGTM - only one minor comment for the OSMIDTyped class.

@deniskoronchik If you think there's a good reason not to make the type field an enum, then it's OK to not change it, but please add a code comment describing what the type field represents.

@deniskoronchik Can you please add an entry to CHANGELOG.md, then I think this is good to merge.

danpat · 2017-09-08T05:03:39Z

include/util/osm_id_typed.hpp

+    inline HashType Hash() const { return (std::uint64_t(id) | std::uint64_t(type) << 56); }
+
+    std::uint64_t GetID() const { return id; }
+    std::uint8_t GetType() const { return type; }


I think it would be good to do what osmium does here and make type an enum. We should have an explicit mapping from the osmium::item_type enum values to our own internal values.

danpat · 2017-09-08T05:09:54Z

src/extractor/scripting_environment_lua.cpp

+        osmium::item_type GetItemType() const { return item_type; }
+        osmium::object_id_type GetId() const { return id; }
+
+        util::OsmIDTyped ref() const { return util::OsmIDTyped(id, std::uint8_t(item_type)); }


This is the other side to my comment on OsmIDTyped - I think we should be very explicit about what values we store here. Something like:

switch(item_type) { case osmium::item_type.node: return util::OsmIDTyped(id, osrm::extractor::OSMIDType.NODE); case osmium::item_type.way: return util::OsmIDTyped(id, osrm::extractor::OSMIDType.WAY); default: throw util::exception("some error message"); }

@danpat done there 5bd29dd

1ec5 · 2017-09-08T09:28:17Z

Great to see this moving forward! As a first use-case we should pass on cardinal directions from the route relation onto it's containing way members.

For now we're good with concatenating the route's cardinal direction (North, South, ..) onto the way's ref property.

In future versions (probably v6) we might want to reconsider this and properly split it off into own properties: #3304.

Note that simply appending onto the ways’ ref property will produce incorrect refs for many route concurrencies in the U.S.: Project-OSRM/osrm-text-instructions#119 (comment).

oxidase

LGTM! I still think that most of c++ wrappers can be removed, because sol2 is really great library and can create such wrappers invisible. Also it reduces copying data c++ -> lua -> c++

I can attach a patch or commit later a cleanup

oxidase · 2017-09-13T13:21:25Z

src/extractor/scripting_environment_lua.cpp

@@ -366,6 +499,45 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
        sol::property([](const ExtractionWay &way) { return way.backward_restricted; },
                      [](ExtractionWay &way, bool flag) { way.backward_restricted = flag; }));

+    struct ExtractionRelationData


The wrapper is not really needed. sol2 understands lambda return types:

context.state.new_usertype<ExtractionRelation>( "ExtractionRelation", sol::meta_function::new_index, [](ExtractionRelation &rel, const RelationMemberWrap &member) -> ExtractionRelation::AttributesMap& { return rel.GetMember(member.ref()); }, sol::meta_function::index, [](ExtractionRelation &rel, const RelationMemberWrap &member) -> ExtractionRelation::AttributesMap& { return rel.GetMember(member.ref()); }, "restriction", sol::property([](const ExtractionRelation &rel) { return rel.is_restriction; }, [](ExtractionRelation &rel, bool flag) { rel.is_restriction = flag; }));

below

oxidase · 2017-09-13T15:16:38Z

src/extractor/scripting_environment_lua.cpp

 }

 namespace osrm
 {
 namespace extractor
 {

+class RelationsContainerWrap


The wrapper is not needed and can be removed. Also context.state.new_usertype<RelationsContainerWrap>( ... can be removed. Relations can be forwarded to lua as

node_function(profile_table, node, result, sol::as_table(relations)); ... way_function(profile_table, way, result, sol::as_table(relations));sol::as_table(relations)

oxidase · 2017-09-13T15:19:58Z

src/extractor/scripting_environment_lua.cpp

+        ExtractionRelation::AttributesMap &attrs;
+    };
+
+    context.state.new_usertype<RelationsContainerWrap>(


oxidase · 2017-09-13T15:20:29Z

src/extractor/scripting_environment_lua.cpp

+        sol::meta_function::index,
+        [](const RelationsContainerWrap &rel, size_t index) { return rel.GetAttributes(index); });
+
+    context.state.new_usertype<ExtractionRelationData>(


oxidase · 2017-09-13T15:21:04Z

src/extractor/scripting_environment_lua.cpp

+    context.state.new_usertype<ExtractionRelation>(
+        "ExtractionRelation",
+        sol::meta_function::new_index,
+        [](ExtractionRelation &rel, const RelationMemberWrap &member) {


add -> ExtractionRelation::AttributesMap&

oxidase · 2017-09-13T15:21:14Z

src/extractor/scripting_environment_lua.cpp

+            return ExtractionRelationData(rel.GetMember(member.ref()));
+        },
+        sol::meta_function::index,
+        [](ExtractionRelation &rel, const RelationMemberWrap &member) {


add add -> ExtractionRelation::AttributesMap&``

oxidase · 2017-09-13T15:38:38Z

src/extractor/scripting_environment_lua.cpp

@@ -276,6 +288,71 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
        "version",
        &osmium::Way::version);

+    struct RelationMemberWrap


The wrapper can be removed return sol::as_table(rel.members()); should work well

oxidase · 2017-09-14T11:32:12Z

@deniskoronchik @TheMarex i have removed Lua 5.1 support because ThePhD/sol2#302 makes relations wrapping too complicated and requires copying of members for every node/way.

ATM windows build failed, because it requires Lua 5.1 -> 5.2 update.
I will try to build dependencies locally and update osrm-deps-win-x64-14.0.7z

Pool instance has been removed from Reader ctor parameters list in PR Project-OSRM#4438, presumably unintentionally. It is required to prevent potential deadlock during Pool shutdown as explained in PR Project-OSRM#4452.

Pool instance has been removed from Reader ctor parameters list in PR #4438, presumably unintentionally. It is required to prevent potential deadlock during Pool shutdown as explained in PR #4452.

deniskoronchik added the Work In Progress label Aug 24, 2017

TheMarex reviewed Aug 30, 2017

View reviewed changes

TheMarex added this to the 5.12.0 milestone Aug 30, 2017

deniskoronchik added the Review label Aug 31, 2017

TheMarex modified the milestones: 5.13, 5.12.0 Aug 31, 2017

oxidase requested changes Sep 1, 2017

View reviewed changes

deniskoronchik requested review from daniel-j-h and danpat September 4, 2017 13:28

deniskoronchik mentioned this pull request Sep 5, 2017

Process route relation in car.lua #4478

Closed

6 tasks

danpat reviewed Sep 8, 2017

View reviewed changes

oxidase approved these changes Sep 13, 2017

View reviewed changes

deniskoronchik and others added 12 commits September 14, 2017 08:04

Add ExtractionRelation class

3946316

LUA binding for a relation processing

505e082

Pass relation data to way and node functions

4c279c0

Update LUA profiles documentation

5f612ee

Fix some tests and add new ones

b53b638

Apply clang format

f6fd8e3

Review fixes

a2405d1

Fix different compilers support

e4c1b66

Review fixes

ee3251f

Remove ExtractionRelationData wraper

b6e0eda

Remove Lua 5.1 support

392d860

Remove RelationMemberWrap and avoid data copying

95a0350

oxidase added 2 commits September 14, 2017 13:24

Remove RelationsContainerWrap

3c16343

Small ProcessElements refactoring

8e274f9

Fix EOL value in libosmium opl script

03254ea

deniskoronchik added Ready To Merge and removed Work In Progress labels Sep 14, 2017

oxidase added 2 commits September 14, 2017 17:39

Bump CMake version to 3.9.2

65d380e

[skip ci] Moved support relations entry in change log

03582d8

oxidase merged commit c065335 into Project-OSRM:master Sep 14, 2017

mloskot mentioned this pull request Jan 5, 2018

Restore use of user-provided osmium::thread::Pool instance #4774

Merged

1 task

		@@ -1 +1 @@
		n1 v1 dV c1 t2014-01-01T00:00:00Z i1 utest T x1.02 y1.02

Work on support relations in lua profiles #4438

Work on support relations in lua profiles #4438

Conversation

deniskoronchik commented Aug 24, 2017 • edited Loading

Issue

Tasklist

emiltin commented Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

deniskoronchik commented Aug 31, 2017

daniel-j-h commented Aug 31, 2017

oxidase 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

deniskoronchik Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskoronchik Sep 1, 2017 • 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

deniskoronchik Sep 1, 2017 • 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

deniskoronchik Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskoronchik commented Sep 1, 2017

oxidase commented Sep 1, 2017

danpat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskoronchik Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Sep 8, 2017 • edited Loading

oxidase 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

oxidase commented Sep 14, 2017

deniskoronchik commented Aug 24, 2017 •

edited

Loading

emiltin commented Aug 24, 2017 •

edited

Loading

deniskoronchik Sep 1, 2017 •

edited

Loading

deniskoronchik Sep 1, 2017 •

edited

Loading

deniskoronchik Sep 1, 2017 •

edited

Loading

deniskoronchik Sep 1, 2017 •

edited

Loading

danpat left a comment •

edited

Loading

deniskoronchik Sep 8, 2017 •

edited

Loading

1ec5 commented Sep 8, 2017 •

edited

Loading