Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSRM 5.0 API #1935

Merged
merged 359 commits into from Apr 8, 2016

Conversation

Projects
None yet
@TheMarex
Copy link
Member

commented Jan 28, 2016

This PR is meant to track the implementation of the new OSRM 5.0 API.

TODO

  • Fix api_parameter parser for new query URL
  • Port plugins to new response format:
    • viaroute
    • trip
    • table
    • match
    • nearest
  • Unit Tests #2088 @daniel-j-h
  • Adapt cucumber tests to new API:
    • Fix the support code
    • Remove code that tests for old API behavior

bug fixes

  • Return simplified overview geometry display (too granular at the moment, only returns origin/destination)
  • Investigate dead-end snapping bearings bug #1935 (comment)
  • Refactor uturns parameter functionality https://github.com/mapbox/api-directions/issues/453
  • Fix geometries parameter handling - step geometries are only returned as polyline even with geojson geometry requests
  • Return true NoMatch response in match service #1935 (comment)
  • Rename no match response code in match service to NoMatch from NoMatchings (parity with public api)
  • Fix max integer values in distance table to return null rather than 2147483647
  • Fix the polyline numeric problems
  • investigate mis-numbered roundabout exits (#2007 (comment))
  • Rename alternative parameter to alternatives

For reference the specification that we want to implement: https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api

/cc @daniel-j-h @MoKob would be interested about your opinion of the code structure.

double heading_before;
double heading_after;
extractor::TurnInstruction instruction;
};

This comment has been minimized.

Copy link
@MoKob

MoKob Jan 28, 2016

Would love to have the opportunity to add something like a hint to every maneuver. Essentially something like Turn right at name, or turn right onto ramp

This comment has been minimized.

Copy link
@TheMarex

TheMarex Jan 28, 2016

Author Member

Oh this is captured by the RouteStep by the way_name. Sorry should document the relation better here. StepManeuver is the maneuver used to navigate on the given RouteStep

This comment has been minimized.

Copy link
@MoKob

MoKob Jan 29, 2016

This only allows a single name, does it? I was thinking about something like take exit **number** towards **direction**. Is there a way do give these additional hints or am I supposed to encode these into some of the fields? I guess way_name would be the exit number (as a name for the ramp?), but where would I add the additional hints for the direction name?

Using the name could be a possibility, but it would somewhat break the expected result if such hints are encoded into the name field.

@emiltin

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2016

what about simplification of instructions? for example, crossing a big intersection on bike often results in multiple 'miniature' instructions because of the way the bike paths is modelled. but all you want to know as a user is a simple 'continue on ...'

std::vector<util::FixedPointCoordinate>
douglasPeucker(std::vector<util::FixedPointCoordinate>::const_iterator begin,
std::vector<util::FixedPointCoordinate>::const_iterator end,
const unsigned zoom_level);

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Why the change in the Douglas Peucker interface here? (Not saying it's bad, it looks better, just curious)

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

To circumvent creating a vector for each route step, the whole leg geometry is saved in one large vector and each step only saves indices into that. Technically we decided now that per-step simplification is probably not a good idea, but this is a left-over from the previous design.

@@ -47,7 +48,7 @@ class Engine final
public:
Engine(EngineConfig &config_);
Engine(const Engine &) = delete;
int RunQuery(const RouteParameters &route_parameters, util::json::Object &json_result);
Status RunQuery(const RouteParameters &route_parameters, util::json::Object &json_result);

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

tl;dr: returning an incomplete type is UB

3.9/5 http://eel.is/c++draft/basic.types#5

A class that has been declared but not defined, an enumeration type in certain contexts ([dcl.enum]), or an array of unknown size or of incomplete element type, is an incompletely-defined object type.45 Incompletely-defined object types and cv void are incomplete types ([basic.fundamental]).

7.2/6 http://eel.is/c++draft/dcl.enum#6

An enumeration whose underlying type is fixed is an incomplete type from its point of declaration ([basic.scope.pdecl]) to immediately after its enum-base (if any), at which point it becomes a complete type. An enumeration whose underlying type is not fixed is an incomplete type from its point of declaration to immediately after the closing } of its enum-specifier, at which point it becomes a complete type.

3.10/4 http://eel.is/c++draft/basic.lval#4

Unless otherwise indicated ([expr.call]), prvalues shall always have complete types or the void type; in addition to these types, glvalues can also have incomplete types. [ Note: class and array prvalues can have cv-qualified types; other prvalues always have cv-unqualified types. See Clause [expr]. — end note ]

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

Oh interesting. Thanks for the heads up. 👍

#include "extractor/turn_instructions.hpp"
#include "extractor/travel_mode.hpp"

#include <vector>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

include utility for move


#include <boost/range/irange.hpp>

#include <vector>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

cstdint for size_t, uint32_t
numeric for accumulate
algorithm for transform, sort, min
string for, well, string :)
iterator for next

#include "engine/guidance/leg_geometry.hpp"
#include "engine/internal_route_result.hpp"

#include <boost/range/irange.hpp>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

unused, also older Boost's irange (e.g. 1.55) show disturbing behavior #1642 (comment)

return lhs.position < rhs.position;
});

std::string summary;

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

boost::algorithm::join does the next 10 lines; take a look at rtd/include/util/geojson

join(segments | transformed(toName), ", ");
#include "util/integer_range.hpp"

#include <boost/assert.hpp>
#include <boost/range/iterator_range.hpp>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

unused, see comment above

#include "engine/guidance/leg_geometry.hpp"
#include "engine/douglas_peucker.hpp"

#include <boost/range/irange.hpp>

This comment has been minimized.

Copy link
@daniel-j-h
{
namespace guidance
{
class OverviewAssembler

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Why is this a class when the small helper types above are all structs?

{
public:
std::vector<util::FixedPointCoordinate>
operator()(const std::vector<LegGeometry> &leg_geometries, const unsigned zoom_level) const

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Also with a single operator() this could be a free standing function?

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

Yep, transforming these. Doesn't make much sense when you don't capture the Datafacade.

struct Route
{
double duration;
double distance;

This comment has been minimized.

Copy link
@daniel-j-h
class RouteAssembler
{
public:
Route operator()(const std::vector<RouteLeg> &route_legs) const

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Same as above, struct, operator()


#include <boost/range/irange.hpp>

#include <vector>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

numeric for accumulate

double duration;
double distance;
std::string summary;
boost::optional<std::vector<RouteStep>> steps;

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Does a optional<vector<T>> make sense here? A empty vector<T> already signals an abundance of values. Making use of the Free Monad and all that.

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

You are right, don't know why I wrapped this.

#include "engine/guidance/step_maneuver.hpp"

#include <string>
#include <vector>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

cstddef for size_t

unsigned name_id;
std::string way_name;
double duration;
double distance;

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

instead of duration and distance, why not use the Route type defined above that itself holds a duration and distance?

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

Because it would be semantically incorrect. Route contains RouteLegs which contain RouteSteps

#include "extractor/turn_instructions.hpp"
#include "extractor/travel_mode.hpp"

#include <boost/range/irange.hpp>

This comment has been minimized.

Copy link
@daniel-j-h

#include <boost/range/irange.hpp>

#include <vector>

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

utility for move
cstddef for size_t

{
auto name = facade->get_name_for_id(path_point.name_id);
const auto distance = leg_geometry.segment_distances[segment_index];
steps.push_back(RouteStep{path_point.name_id, std::move(name),

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

I think you can emplace_back here

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

But that would mean adding a custom constructor to RouteStep no?

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Oh, yes you are correct. Hmm push_back({args}) should work though.

}
}
const auto distance = leg_geometry.segment_distances[segment_index];
steps.push_back(RouteStep{

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

same here

// |-------------t target_duration
// x---*---*---*---z compressed edge
// |-------| duration
steps.push_back(RouteStep{

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

same here


BOOST_ASSERT(segment_index == number_of_segments - 1);
// This step has length zero, the only reason we need it is the target location
steps.push_back(RouteStep{

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

same here

return encoded;
}

static Hint FromBase64(std::string base64Hint)

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Take by const ref?

@@ -17,26 +17,15 @@ namespace engine

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

cstddef for size_t

@@ -2,6 +2,7 @@
#define BASE_PLUGIN_HPP

#include "engine/phantom_node.hpp"
#include "engine/status.hpp"

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

utility for pair
iterator for begin, end, back_inserter

json_result.values["code"] = code;
json_result.values["message"] = message;
return Status::Error;
}

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Take the strings by value and move them into the values --- if a copy needs to be made anyway, then is will make a copy. Otherwise allows the callsite to optimize for rvalues by moving objects in.

std::vector<guidance::LegGeometry> leg_geometries;
legs.reserve(raw_route.segment_end_coordinates.size());
leg_geometries.reserve(raw_route.segment_end_coordinates.size());
for (auto idx : boost::irange(0UL, raw_route.segment_end_coordinates.size()))

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

See comment and ref. above why we do not use irange. Dangerous.


// Decodes geometry from polyline format
// See: https://developers.google.com/maps/documentation/utilities/polylinealgorithm
std::vector<util::FixedPointCoordinate> polylineDecode(const std::string &polyline);
std::vector<util::FixedPointCoordinate> decodePolyline(const std::string &polyline);

This comment has been minimized.

Copy link
@daniel-j-h

daniel-j-h Feb 1, 2016

Member

Hm so global convention is verbNoun? I think that's my fault, I changed it when refactoring the code last week.

This comment has been minimized.

Copy link
@TheMarex

TheMarex Feb 1, 2016

Author Member

Yeah probably happened when you switched from the noun PolylineDecoder to polylineDecode. decodePolyline sounds better to my ear?

TheMarex and others added some commits Apr 7, 2016

Moritz Kobitzsch
Moritz Kobitzsch
Allow 4.json and 4.3.json format; needs -lit(".") >> -lit("json") hac…
…k^Wworkaround

Rainer Deyke's workaround without the need to do ugly backtracking.

References:
- http://lists.boost.org/boost-users/2016/03/85960.php
- #2173 (comment)
Fix #2173 with a no_trailing_dot_policy
no_trailing_dot_policy rejects parsing exp, exp_n, nan, inf
and rejects parsing a fractional part if detects ".Fmt".
For Fmt = 'j', 's', 'o', 'n':
 42.foo    rule parses 42.
 42.json   rule parses 42
 42..json  rule parses 42.

Reference:
- #2222 (comment)
Fix BOOST_FUSION_ADAPT_STRUCT parameters for ParsedURL
Fix build error: macro "BOOST_FUSION_ADAPT_STRUCT" passed 5 arguments, but takes just 2
https://travis-ci.org/Project-OSRM/osrm-backend/builds/121406444
Conform to v5 spec and support "unlimited" as radiuses value.
Supersedes: #2231

Thanks to Michael Krasnyk (@oxidase) for figuring out the reason for the
segfault earlier:

> #2231 (comment)
Adapts all grammars to use expectation parsers without backtracking.
Sequence parsers using `>>` allow for backtracking, expectation parsers
`>` do not. This allows us to properly report the position where parsing
failed, by catching the expectation_failure exception and adapting the
iterator ourselves.

References:
- #2188
- #2168
- http://www.boost.org/doc/libs/1_55_0/libs/spirit/doc/html/spirit/qi/reference/operator/expect.html

@TheMarex TheMarex merged commit cb18c1a into develop Apr 8, 2016

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on rewrite/new-api at 88.235%
Details

@TheMarex TheMarex changed the title [NOT READY] OSRM 5.0 API OSRM 5.0 API Apr 8, 2016

@TheMarex

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2016

All tests pass. Merged into deveop. 🎉 🎉

@MoKob MoKob deleted the rewrite/new-api branch May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.