Parameter for keep waypoints on the left/right #4039

fijemax · 2017-05-12T14:59:52Z

Description :

This PR adds a parameter for each point to chose the start/stop side.
Three option can be used :

DEFAULT to start/stop at the drive side
OPPOSITE to start/stop at the opposite of default (cross the road)
BOTH each side can be used (old behavior).

The default depending at the country.

Issue

This pull request is linking to this issue :
#2201

Tasklist

Adding sides parameter into base parameters, it can take the values DEFAULT, OPPOSITE or BOTH.
Adding url parser for "sides" parameter, url values are "d" for DEFAULT, "o" for OPPOSITE and "b" for BOTH, example : "sides=s;s".
Checking parameters, if "sides" parameter is used, the number of parameter is the same as number of location.
Create a phantom to start/stop at side drive or opposite drive side.
Retrieve the country's drive side for apply the DEFAULT.

For the moment the country's drive side is set to right by default, we can find this parameter in the lua file.
How can access this parameter in the osrm-routed ?

Example for DEFAULT on start and stop route point value :
Here start and stop

Another example :

For the one-way roads :
The algorythm do not use the "side" parameter for the phantom node option because the driver hasn't the choice.

fijemax · 2017-05-12T15:04:42Z

@frodrigo

TheMarex · 2017-05-15T09:47:40Z

Thank you for taking a stab at this! 🎉 I just skimmed the diff quickly some things that I noticed:

No test coverage, this should have a cucumber test
No URL parser test, this should have a unit-test
NodeJS bindings are not extended yet.

I can put a real review on my TODO for later today, I need to think a little bit about parameter naming and such.

fijemax · 2017-05-17T09:57:42Z

Hi @TheMarex
I extended the NodeJS for the sides parameter.
I added the cucumber test.

oxidase

Looks good! AFAIU, DEFAULT side should be used as a parameter for re-routing

oxidase · 2017-05-17T12:59:16Z

include/server/api/base_parameters_grammar.hpp

@@ -149,10 +159,13 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
            qi::lit("bearings=") >
            (-(qi::short_ > ',' > qi::short_))[ph::bind(add_bearing, qi::_r1, qi::_1)] % ';';

+        sides_rule = qi::lit("sides=") >


sides_rule can be qi::symbols, so add_side is not needed.
Also BOTH can be used for empty boost::optional values with std::vector<Side> sides

I tryed to change by this code :

side_default = qi::char_("a-zA-Z")[qi::_val = engine::Side{engine::BOTH}]; struct StartSideValues: qi::symbols<char, engine::Side> { StartSideValues() {add("b", engine::Side{engine::BOTH})("d", engine::Side{engine::DEFAULT})("o", engine::Side{engine::OPPOSITE})("", engine::Side{engine::OPPOSITE}); } }; StartSideValues values; sides_rule = qi::lit("sides=") > (-(values | side_default) % ';')[ph::bind(&engine::api::BaseParameters::sides, qi::_r1) = qi::_1];

But it provokes segfault.
I am trying to analyse with gdb but it's a really complicated backtrace.

@fijemax it crashes because grammars are local variables in the constructor.

What i suggest is to use in the way as in commit oxidase@301a1cc It contains more restrictive grammar [bod|eps]

Please add also some unit tests for parsing incorrect URLs as suggested by @TheMarex

oxidase · 2017-05-17T13:08:11Z

features/testbot/side_param.feature

+
+        When I route I should get
+            | from | to | sides | route    |
+            | s    | e  | b o   | ab,bc,bc |


is it possible to add a turn instructions check? to see explicitly a u-turn

oxidase · 2017-05-17T13:14:48Z

include/engine/geospatial_query.hpp

+                    //                       |
+                    //                 input_coordinate
+
+                    bool input_coordinate_is_at_right = !util::coordinate_calculation::isCCW(


util::coordinate_calculation::isCCW(coordinates[segment.data.u], input_coordinate, coordinates[segment.data.v]); to correspond the figure above

fijemax · 2017-05-17T13:52:03Z

I just added the nodeJS binding and url tests.

fijemax · 2017-05-19T08:56:12Z

Hi @oxidase, thanks for parser example.
I refactored the code

Simple enumerator for Side
Change gi::lit to qi::symbol for side parser
I added URL tests in the unit_tests

oxidase

LGTM, some small cleanup comments

oxidase · 2017-05-19T09:05:13Z

unit_tests/server/parameters_parser.cpp

@@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(invalid_route_urls)
    BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("1,2;3,4?overview=false&radiuses=foo"),
                      32UL);
    BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("1,2;3,4?overview=false&sides=foo"),
-                      30UL);
+                      29UL);


oxidase · 2017-05-19T09:11:57Z

include/engine/side.hpp

+#ifndef OSRM_ENGINE_SIDE_HPP
+#define OSRM_ENGINE_SIDE_HPP
+
+#include <string>


It is not needed

oxidase · 2017-05-19T09:12:22Z

include/server/api/base_parameters_grammar.hpp

+        sides_rule = qi::lit("sides=") >
+                        (-(side_type | qi::attr(engine::Side::BOTH)) %
+                         ';')[ph::bind(&engine::api::BaseParameters::sides, qi::_r1) = qi::_1];
+                     //(-qi::symbols[qi::char_("aA-zZ")])[ph::bind(add_side, qi::_r1, qi::_1)] % ';';


oxidase · 2017-05-19T09:20:57Z

include/server/api/base_parameters_grammar.hpp

@@ -149,10 +149,17 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
            qi::lit("bearings=") >
            (-(qi::short_ > ',' > qi::short_))[ph::bind(add_bearing, qi::_r1, qi::_1)] % ';';

+        side_type.add("d", engine::Side::DEFAULT)("o", engine::Side::OPPOSITE)("b", engine::Side::BOTH);
+        sides_rule = qi::lit("sides=") >
+                        (-(side_type | qi::attr(engine::Side::BOTH)) %


Here must be either - for vector<optional<Side>> or qi::attr(engine::Side::BOTH) for vector<Side>
- will place empty optionals for empty values, but attr(BOTH) will place BOTH.

(-side_type %';')[...] is enough for vector of optionals

TheMarex · 2017-05-19T09:47:06Z

Thanks for adding the tests! Some API features need a quick discussion:

Is there a real use-case for opposite? I feel the actual feature here is the ability to force the curb approach.
Parameter name. I feel something like approach={curb,unrestriced} might be more descriptive.
Do we need the ability to configure this on per-waypoint basis? The scenarios I could come up with (taxi, garbage can pickup) would only need this to be a global flag that applies to all waypoints.

frodrigo · 2017-05-19T12:17:55Z

Is there a real use-case for opposite? I feel the actual feature here is the ability to force the curb approach.

Yes we build it for a use case where we need the curb approach. We do not have use case the opposite. But why implement half an option as we can easily make it for both sides? Nevertheless we can remove opposite if you want.

Do we need the ability to configure this on per-waypoint basis? The scenarios I could come up with (taxi, garbage can pickup) would only need this to be a global flag that applies to all waypoints.

We need it per waypoint. E.g. on garbage pickup, the pickup point must be done with the curb approach, but at the depot the side does not matter.

We also plan to add to this PR the support of the approach to other plugins. Especially on the table plugin (approach per waypoint make even more sens on it).

TheMarex · 2017-05-20T14:00:19Z

But why implement half an option as we can easily make it for both sides?

One of the main concerns with adding stuff is usually API-surface area. The fewer options we have, the less likely it is that we need to do a breaking API change. Would be cool if we could limit this to only unrestricted and curb.

We need it per waypoint. E.g. on garbage pickup, the pickup point must be done with the curb approach, but at the depot the side does not matter.

Okay thanks for the explanation. /cc @daniel-j-h because he has been doing a lot of VRP stuff lately and this might be useful for him too?

We also plan to add to this PR the support of the approach to other plugins.

🤔 I think that should already work if the right method was extended, all plugins rely on GetPhantomNodes() which should operate on the BaseParameter (hence works for all plugins, similar to how we handle bearings and radiuses)

I would like to propose the following API:

HTTP

approaches={approach}[;{approach}; ...] where approach is a String of either curb or unrestricted (default: unrestricted). Needs to have the same length as the number of coordinates given.

Example: ?approaches=curb;unrestricted;curb;curb;curb;unrestricted or?approaches=curb;;curb;curb;curb;

Node JS

approaches Array of Strings that can either be curb or unrestricted. Needs to have the same length as the number of coordinates given. (default: null, which means unrestricted)

Example: {approaches: ["curb", "unrestricted", "curb", "curb"]} or {approaches: ["curb", null, "curb", "curb"]}

fijemax · 2017-05-22T07:59:47Z

Hi @TheMarex ,
Thank you for the review, I going to make the changes as respecting your API descriptions.
Also, I will don't know how to retrieve the country's drive side for apply the curb option, is this actually possible ?
For the moment I apply right side by default for curb approach.

TheMarex · 2017-05-22T09:39:13Z

Also, I will don't know how to retrieve the country's drive side for apply the curb option, is this actually possible?

Ah this is part of ProfileProperties::left_hand_driving. To use this you would need to extend ContiguousInternalMemoryDataFacadeBase and BaseDataFacade with a new function like bool IsLeftHandDriving() { return m_profile_properties.left_hand_driving; }.

It can be configured in the profile via properties.left_hand_driving = true.

daniel-j-h · 2017-05-22T10:39:31Z

We need it per waypoint. E.g. on garbage pickup, the pickup point must be done with the curb approach, but at the depot the side does not matter.

Okay thanks for the explanation. /cc @daniel-j-h because he has been doing a lot of VRP stuff lately and this might be useful for him too?

Yeah I can totally see how this is useful. You can somewhat fake it already setting bearings - but then you need to do some pre-processing / set the bearing constraints manually.

emiltin · 2017-05-22T10:52:57Z

note that ProfileProperties::left_hand_driving is a global property set once by the profile. it doesn't take into account what country the current request (and and waypoint) is located in. so if you're working with a g planet dataset, the api will be wrong in some countries, and right in others.

- Adding sides parameter into base parameters, it can take the values SIDE, OPPOSITE or DEFAULT. - Adding url parser for "sides" parameter, url values are "s" for SIDE, "o" for OPPOSITE and "d" for DEFAULT, example : "sides=s;s". - Checking parameters, if "sides" parameter is used, the number of parameter is the same as number of location. - Create a phantom to start at side driving or Opposite side driving. Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Propagating approach parameter for plugins : - tabler - nearest - trip Signed-off-by: FILLAU Jean-Maxime <[email protected]>

fijemax · 2017-05-23T12:11:55Z

@oxidase
I refactored the API and propagate the param for every phantom nodes search function.
The support for left hand driving is ok.
Cucumber tests are ok and url unit tests too.

TheMarex · 2017-05-29T12:27:32Z

@fijemax looks like the only thing that is failing right now is the formating check. Can you try:

./scripts/format.sh

That requires clang-format 3.8.

TheMarex

Some minor things need to be fixed. A test for the node bindings would be great too, there seems to be a bug right now.

Thanks for this! Looking forward to shipping this in 5.8.

TheMarex · 2017-05-29T12:29:56Z

include/engine/approach.hpp

+namespace engine
+{
+
+enum Approach


enum class Approach : std::uint8_t would be better here, for one to remove implicit conversions and to limit the storage requirements to one byte per coordinate.

TheMarex · 2017-05-29T12:30:29Z

include/engine/datafacade/contiguous_internalmem_datafacade.hpp

    }

    std::vector<PhantomNodeWithDistance>
    NearestPhantomNodesInRange(const util::Coordinate input_coordinate,
                               const float max_distance,
                               const int bearing,
-                               const int bearing_range) const override final
+                               const int bearing_range,
+                               const engine::Approach approach) const override final


engine:: is unnecessary.

TheMarex · 2017-05-29T12:30:42Z

include/engine/datafacade/contiguous_internalmem_datafacade.hpp

    }

    std::vector<PhantomNodeWithDistance>
    NearestPhantomNodes(const util::Coordinate input_coordinate,
-                        const unsigned max_results) const override final
+                        const unsigned max_results,
+                        const engine::Approach approach) const override final


TheMarex · 2017-05-29T12:30:51Z

include/engine/datafacade/contiguous_internalmem_datafacade.hpp

    }

    std::vector<PhantomNodeWithDistance>
    NearestPhantomNodes(const util::Coordinate input_coordinate,
                        const unsigned max_results,
-                        const double max_distance) const override final
+                        const double max_distance,
+                        const engine::Approach approach) const override final


TheMarex · 2017-05-29T12:30:57Z

include/engine/datafacade/contiguous_internalmem_datafacade.hpp

    }

    std::vector<PhantomNodeWithDistance>
    NearestPhantomNodes(const util::Coordinate input_coordinate,
                        const unsigned max_results,
                        const int bearing,
-                        const int bearing_range) const override final
+                        const int bearing_range,
+                        const engine::Approach approach) const override final


TheMarex · 2017-05-29T12:34:21Z

include/engine/geospatial_query.hpp

@@ -15,6 +16,8 @@
 #include <memory>
 #include <vector>

+#include <util/log.hpp>


not needed anymore?

TheMarex · 2017-05-29T12:36:41Z

include/engine/geospatial_query.hpp

@@ -545,6 +578,32 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
        return datafacade.GetComponentID(data.forward_segment_id.id).is_tiny;
    }

+    std::pair<bool, bool> approach_usage(const util::Coordinate &input_coordinate,


Rename to ApproachUsage for camel-case. A better name could be CheckApproach.

TheMarex · 2017-05-29T12:38:55Z

include/nodejs/node_osrm_support.hpp

+
+            if (approach_raw->IsNull())
+            {
+                params->bearings.emplace_back();


Copy&Paste error, should be params->approaches.

TheMarex · 2017-05-29T12:39:34Z

include/nodejs/node_osrm_support.hpp

+
+        if (!approaches->IsArray())
+        {
+            Nan::ThrowError("Approaches must be an array of arrays of numbers");


Error message is wrong: Should be arrays of strings

TheMarex · 2017-05-29T12:40:14Z

include/nodejs/node_osrm_support.hpp

+                }
+                else
+                {
+                    Nan::ThrowError("'approach' param must be one of [curb, unrestricted]");


approach -> approaches

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Suppress "engine::" Signed-off-by: FILLAU Jean-Maxime <[email protected]>

fijemax · 2017-05-29T16:07:50Z

Hi @TheMarex
Change was done.
I don't konw how testing the nodejs binding ?

TheMarex · 2017-05-29T17:34:37Z

@fijemax for some examples look at test/nodejs/route.js. What you basically want is to create a request that uses your parameter and try a few permutations that would create an error.

fijemax · 2017-05-30T07:44:58Z

@TheMarex When I run nodejs test/nodes/index.js or nodejs test/nodes/route.js I have the following error :
Error: Cannot find module './binding/node_osrm.node'

fijemax · 2017-05-30T08:33:23Z

It's ok, I hadn't activate ENABLE_NODE_BINDINGS during cmake compilation.

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

fijemax · 2017-05-30T09:30:51Z

@TheMarex the nodejs binding tests was done

TheMarex · 2017-05-30T10:19:36Z

@fijemax awesome, this looks good to merge for me. 🎉

daniel-j-h · 2017-05-30T10:43:05Z

Very cool - looking forward to having this in a stable release!

What I just now realized after this was merged is we need

a changelog entry (changelog.md)
docs for the http api (docs/http.md)
docs for the node bindings (docs/nodejs/api.md)

could you run this out or ticket and tag it for the next release @fijemax so we don't forget about it.

(also feel free to write a osm diary / blog post about this if you have the time for it, I can totally see people being interested in this feature :) I wrote a bit about bearings - this one would fit right in)

fijemax · 2017-05-30T13:53:31Z

@daniel-j-h I just created this PR #4104.

fijemax · 2017-06-19T08:43:06Z

Hi @daniel-j-h, i wrote this about the new feature.

daniel-j-h · 2017-06-19T12:15:22Z

Sweet, thank you!

asaveljevs · 2019-11-26T11:09:12Z

Is there a real use-case for opposite? I feel the actual feature here is the ability to force the curb approach.

We have tried to describe our use case for opposite in #5616, which is needed for solving a VRP problem.

geocept · 2024-02-10T17:18:00Z

Is there a reason why this great feature is not in the stable release of OSRM?? I mean 6,5 years have passed now and many many people are waiting for it. Would be great if someone would bring it to stable release.

frodrigo · 2024-02-10T17:22:38Z

Is there a reason why this great feature is not in the stable release of OSRM?? I mean 6,5 years have passed now and many many people are waiting for it. Would be great if someone would bring it to stable release.

It was released in 5.8.0
https://github.com/Project-OSRM/osrm-backend/blob/master/CHANGELOG.md#580

fijemax changed the title ~~New "sides" setting in base parameters for phantom nodes.~~ Parameter for keep waypoints on the left/right May 12, 2017

fijemax force-pushed the mt-start-side branch 4 times, most recently from 61548fd to 078b238 Compare May 15, 2017 09:17

TheMarex added the Review label May 15, 2017

TheMarex added this to the 5.8.0 milestone May 15, 2017

fijemax force-pushed the mt-start-side branch from 078b238 to ab3ea32 Compare May 17, 2017 09:57

oxidase requested changes May 17, 2017

View reviewed changes

fijemax force-pushed the mt-start-side branch 2 times, most recently from d2b3514 to 6e55692 Compare May 18, 2017 15:14

fijemax force-pushed the mt-start-side branch from 9ea1b2b to 1ace23f Compare May 19, 2017 09:17

oxidase approved these changes May 19, 2017

View reviewed changes

fijemax force-pushed the mt-start-side branch 2 times, most recently from 33b0389 to 1f83b36 Compare May 19, 2017 13:08

fijemax force-pushed the mt-start-side branch from 1f83b36 to a7c0847 Compare May 22, 2017 09:26

FILLAU Jean-Maxime added 2 commits May 22, 2017 14:56

Adding "sides" params for the sides option tests into java script

e0c1130

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

FILLAU Jean-Maxime added 2 commits May 22, 2017 16:07

Add NodeJS API for Approaches param.

0dd1fc7

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Adding support for left left hand driving.

863b23f

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

fijemax force-pushed the mt-start-side branch from a7c0847 to 863b23f Compare May 22, 2017 14:10

Propagating approach parameter to every phantom nodes search function.

79f8568

Propagating approach parameter for plugins : - tabler - nearest - trip Signed-off-by: FILLAU Jean-Maxime <[email protected]>

TheMarex requested changes May 29, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Review labels May 29, 2017

FILLAU Jean-Maxime added 2 commits May 29, 2017 14:46

Use formating script.

eb1cd2e

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Refactoring enum Approach in enum class.

4589366

Suppress "engine::" Signed-off-by: FILLAU Jean-Maxime <[email protected]>

Add nodejs binding test

673c55f

Signed-off-by: FILLAU Jean-Maxime <[email protected]>

TheMarex approved these changes May 30, 2017

View reviewed changes

TheMarex merged commit 15a1e10 into Project-OSRM:master May 30, 2017

fijemax mentioned this pull request May 30, 2017

Adds documentations on 'approaches' parameter. #4104

Merged

asaveljevs mentioned this pull request Nov 26, 2019

add approach on the opposite side of the road #5616

Closed

mjjbell mentioned this pull request Apr 2, 2024

Add support for opposite approach request parameter #6842

Merged

6 tasks

Parameter for keep waypoints on the left/right #4039

Parameter for keep waypoints on the left/right #4039

Conversation

fijemax commented May 12, 2017 • edited Loading

Description :

Issue

Tasklist

fijemax commented May 12, 2017

TheMarex commented May 15, 2017

fijemax commented May 17, 2017 • edited Loading

oxidase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fijemax May 18, 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

fijemax commented May 17, 2017 • edited Loading

fijemax commented May 19, 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

TheMarex commented May 19, 2017

frodrigo commented May 19, 2017

TheMarex commented May 20, 2017

HTTP

Node JS

fijemax commented May 22, 2017 • edited Loading

TheMarex commented May 22, 2017

daniel-j-h commented May 22, 2017

emiltin commented May 22, 2017

fijemax commented May 23, 2017

TheMarex commented May 29, 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

fijemax commented May 29, 2017

TheMarex commented May 29, 2017

fijemax commented May 30, 2017 • edited Loading

fijemax commented May 30, 2017

fijemax commented May 30, 2017

TheMarex commented May 30, 2017

daniel-j-h commented May 30, 2017

fijemax commented May 30, 2017

fijemax commented Jun 19, 2017

daniel-j-h commented Jun 19, 2017

asaveljevs commented Nov 26, 2019

geocept commented Feb 10, 2024

frodrigo commented Feb 10, 2024

fijemax commented May 12, 2017 •

edited

Loading

fijemax commented May 17, 2017 •

edited

Loading

fijemax May 18, 2017 •

edited

Loading

fijemax commented May 17, 2017 •

edited

Loading

fijemax commented May 22, 2017 •

edited

Loading

fijemax commented May 30, 2017 •

edited

Loading