fixed-start-and-end-trip #296

ghoshkaj · 2017-02-09T22:32:29Z

Addresses #294

ghoshkaj · 2017-02-13T13:24:41Z

@freenerd would you please review this? I especially want your opinion on the way I've handled the errors coming from osrm-backend. I ended up using a simple equals message, instead of an error assertion because I couldn't get any of the error assertions to work with this kind of osrm-backend error: https://github.com/Project-OSRM/node-osrm/pull/296/files#diff-f2f7b3e695a783f368903f577c8f8036R207

freenerd

Looking good to me

freenerd · 2017-02-13T13:28:07Z

docs/api.md


 **Parameters**

 -   `options` **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)** Object literal containing parameters for the trip query.
+    -   `options.roundtrip` **\[[Boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)]** Return route is a roundtrip. (optional, default `true`)
+    -   `options.source` **\[[String](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String)]** Return route starts at `any` or `first` coordinate. Can also be `first`. (optional, default `any`)


first is in here double

freenerd · 2017-02-13T13:28:19Z

docs/api.md


 **Parameters**

 -   `options` **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)** Object literal containing parameters for the trip query.
+    -   `options.roundtrip` **\[[Boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)]** Return route is a roundtrip. (optional, default `true`)
+    -   `options.source` **\[[String](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String)]** Return route starts at `any` or `first` coordinate. Can also be `first`. (optional, default `any`)
+    -   `options.destination` **\[[String](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String)]** Return route ends at `any` or `last` coordinate. Can also be `last`. (optional, default `any`)


last is in here double

daniel-j-h · 2017-02-13T14:34:04Z

docs/api.md

    -   `options.steps` **\[[Boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)]** Return route steps for each route. (optional, default `false`)
    -   `options.annotations` **\[[Boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)]** Return annotations for each route leg. (optional, default `false`)
    -   `options.geometries` **\[[String](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String)]** Returned route geometry format (influences overview
        and per step). Can also be `geojson`. (optional, default `polyline`)
    -   `options.overview` **\[[String](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String)]** Add overview geometry either `full`, `simplified` (optional, default `simplified`)
 -   `callback` **[Function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function)** 

+**Fixing Start and End Points**
+
+It is possible to explicitely set the start or end coordinate of the trip. When source is set to `first`, the first coordinate is used as start coordinate of the trip in the output. When destination is set to `last`, the last coordinate will be used as destination of the trip in the returned output. If you specify `any`, any of the coordinates can be used as the first or last coordinate in the output.


daniel-j-h · 2017-02-13T14:34:44Z

docs/api.md

+| false | first | last | **yes** |
+| false | first | any | no |
+| false | any | last | no |
+| false | any | any | no |


What are our plans wrt. supporting the currently unsupported combinations? Is this getting checked for in code?

@daniel-j-h it gets checked for in the code and a NotImplemented error is returned if it is an unsupported combination.

daniel-j-h · 2017-02-13T14:35:37Z

docs/api.md

+    [13.374481201171875, 52.506191342034576]
+  ],
+  source: first,
+  destination: last,


What is first and last? Does this work or should it be "first" and "last"?

Oh you're right. Changed these to strings.

Maybe run your example locally to check if it works? :)

daniel-j-h · 2017-02-13T14:38:51Z

src/node_osrm_support.hpp

@@ -733,6 +733,74 @@ argumentsToTripParameter(const Nan::FunctionCallbackInfo<v8::Value> &args,
        return trip_parameters_ptr();
    }

+    if (obj->Has(Nan::New("roundtrip").ToLocalChecked()))
+    {
+        auto roundtrip = obj->Get(Nan::New("roundtrip").ToLocalChecked());


Here and below: what you can do instead of Has()+Get() is the following:

auto maybeRoundtrip = Nan::Get(..); if (maybeRoundtrip.empty() || !maybeRoundtrip.ToLocalChecked()->IsBoolean) error();

Nan::Get returns you a Maybe type similar to boost::optional<T>.

I tried to change it to the Get pattern but can't figure it out right now. But I'm going to leave this pattern as is for now as it is the pattern is used by the rest of the file. In a different PR, I can go through and convert all of them in one go.

daniel-j-h · 2017-02-13T14:44:02Z

src/node_osrm_support.hpp

+            return trip_parameters_ptr();
+        }
+
+        std::string source_str = *v8::String::Utf8Value(source);


Can you check for the ptr not being a nullptr?

Also please use Nan's string functions which are portable:
https://github.com/nodejs/nan/blob/master/doc/v8_misc.md

const Nan::Utf8String utf8str(source); std::string s{*utf8str, *utf8str + utfstr.length()};

Again I tried to implement this but didn't manage to get it to work, but I'll leave this for now, as the pattern here is different from elsewhere in the rest of the file. This can be addressed in a refactor PR that changes the rest of the file as well.

springmeyer · 2017-02-14T18:13:38Z

Looks like circle is failing with:

In file included from /Users/distiller/node-osrm/deps/osrm-backend-Debug/src/extractor/extraction_containers.cpp:12:
In file included from /Users/distiller/node-osrm/deps/osrm-backend-Debug/include/util/name_table.hpp:4:
/Users/distiller/node-osrm/deps/osrm-backend-Debug/include/util/indexed_data.hpp:93:82: error: invalid use of non-static data member 'offset'
        BOOST_ASSERT(data_offset <= std::numeric_limits<decltype(BlockReference::offset)>::max());
                                                                 ~~~~~~~~~~~~~~~~^~~~~~
/Users/distiller/node-osrm/mason_packages/headers/boost/1.63.0/include/boost/assert.hpp:53:45: note: expanded from macro 'BOOST_ASSERT'
#define BOOST_ASSERT(expr) (BOOST_LIKELY(!!(expr))? ((void)0): ::boost::assertion_failed(#expr, BOOST_CURRENT_FUNCTION, __FILE__, __LINE__))
                                            ^~~~
/Users/distiller/node-osrm/mason_packages/headers/boost/1.63.0/include/boost/config/compiler/clang.hpp:62:42: note: expanded from macro 'BOOST_LIKELY'
#define BOOST_LIKELY(x) __builtin_expect(x, 1)
                                         ^
/Users/distiller/node-osrm/deps/osrm-backend-Debug/include/util/indexed_data.hpp:270:32: note: in instantiation of function template specialization 'osrm::util::VariableGroupBlock<16, boost::basic_string_ref<char, std::__1::char_traits<char> > >::WriteBlockReference<unsigned long long, stxxl::vector_iterator<unsigned int, stxxl::RC, unsigned long long, long long, 2097152, stxxl::lru_pager<8>, 4> >' requested here
            data_size += block.WriteBlockReference(out, data_size, curr, next);
                               ^
/Users/distiller/node-osrm/deps/osrm-backend-Debug/src/extractor/extraction_containers.cpp:182:18: note: in instantiation of function template specialization 'osrm::util::IndexedData<osrm::util::VariableGroupBlock<16, boost::basic_string_ref<char, std::__1::char_traits<char> > > >::write<stxxl::vector_iterator<unsigned int, stxxl::RC, unsigned long long, long long, 2097152, stxxl::lru_pager<8>, 4>, stxxl::vector_iterator<unsigned char, stxxl::RC, unsigned long long, long long, 2097152, stxxl::lru_pager<8>, 4> >' requested here
    indexed_data.write(file, name_offsets.begin(), name_offsets.end(), name_char_data.begin());
                 ^
In file included from /Users/distiller/node-osrm/deps/osrm-backend-Debug/src/extractor/extraction_containers.cpp:12:
In file included from /Users/distiller/node-osrm/deps/osrm-backend-Debug/include/util/name_table.hpp:4:
/Users/distiller/node-osrm/deps/osrm-backend-Debug/include/util/indexed_data.hpp:96:70: error: invalid use of non-static data member 'offset'
        BlockReference refernce{static_cast<decltype(BlockReference::offset)>(data_offset), 0};
                                                     ~~~~~~~~~~~~~~~~^~~~~~
2 errors generated.

Per chat we are discussing disabling circle since we are still using travis OS X, but we should still address ^^. I think this is happening because we still use xcode: 7.3 on circle. The error would likely go away if we used xcode: 8.2 on circle. I think xcode 8 is needed for the kind of C++14 support osrm-backend uses. The actionable thing would be to add a check in osrm-backend to assert on the minimum xcode/osx version (perhaps cmake has an easy way to do this).

springmeyer · 2017-02-15T21:23:17Z

The error would likely go away if we used xcode: 8.2 on circle.

It did. Let's discuss next actions at Project-OSRM/osrm-backend#3719

ghoshkaj force-pushed the trip-with-fixed-start-and-end-points branch from 02dec87 to d5069de Compare February 13, 2017 12:20

ghoshkaj requested review from freenerd and chaupow February 13, 2017 13:32

freenerd approved these changes Feb 13, 2017

View reviewed changes

daniel-j-h suggested changes Feb 13, 2017

View reviewed changes

ghoshkaj removed the request for review from chaupow February 13, 2017 15:05

ghoshkaj force-pushed the trip-with-fixed-start-and-end-points branch from 7ce81c2 to ed6c5c8 Compare February 13, 2017 16:15

wrapper for optimized trip with fixed start and end points

acd22ed

ghoshkaj force-pushed the trip-with-fixed-start-and-end-points branch from ed6c5c8 to acd22ed Compare February 14, 2017 15:42

updated xcode version

d36e587

ghoshkaj merged commit 2cb902c into master Feb 14, 2017

ghoshkaj deleted the trip-with-fixed-start-and-end-points branch February 14, 2017 21:29

springmeyer mentioned this pull request Feb 15, 2017

Invalid use of non-static data member 'offset' Project-OSRM/osrm-backend#3719

Closed

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

Wrap: Trip with Fixed Start and End points (TFSE) #294

Closed

4 tasks

daniel-j-h mentioned this pull request Apr 25, 2017

[Trip Plugin] Computing trips without returning to the start point Project-OSRM/osrm-backend#1644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed-start-and-end-trip #296

fixed-start-and-end-trip #296

ghoshkaj commented Feb 9, 2017 •

edited

Loading

ghoshkaj commented Feb 13, 2017

freenerd left a comment

freenerd Feb 13, 2017

freenerd Feb 13, 2017

daniel-j-h Feb 13, 2017

daniel-j-h Feb 13, 2017

ghoshkaj Feb 13, 2017

daniel-j-h Feb 13, 2017

ghoshkaj Feb 13, 2017

daniel-j-h Feb 13, 2017

daniel-j-h Feb 13, 2017

ghoshkaj Feb 13, 2017

daniel-j-h Feb 13, 2017

ghoshkaj Feb 13, 2017

springmeyer commented Feb 14, 2017

springmeyer commented Feb 15, 2017

fixed-start-and-end-trip #296

fixed-start-and-end-trip #296

Conversation

ghoshkaj commented Feb 9, 2017 • edited Loading

ghoshkaj commented Feb 13, 2017

freenerd 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

springmeyer commented Feb 14, 2017

springmeyer commented Feb 15, 2017

ghoshkaj commented Feb 9, 2017 •

edited

Loading