Skip to content

Fix annotations=true handling in NodeJS bindings & libosrm #6415

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 19, 2022
Merged

Fix annotations=true handling in NodeJS bindings & libosrm #6415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94f0d53
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
c7bf2a5
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
1ffb676
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
67b80e6
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
567be2e
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
7d5954f
Fix annotations=true handling in NodeJS bindings
SiarheiFedartsou Oct 18, 2022
4a70ac2
Merge branch 'master' into sf-fix-annotations-node
SiarheiFedartsou Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/nodejs/node_osrm_support.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,9 @@ inline bool parseCommonParameters(const v8::Local<v8::Object> &obj, ParamType &p
if (annotations->IsBoolean())
{
params->annotations = Nan::To<bool>(annotations).FromJust();
params->annotations_type = params->annotations
? osrm::RouteParameters::AnnotationsType::All
: osrm::RouteParameters::AnnotationsType::None;
}
else if (annotations->IsArray())
{
Expand Down
14 changes: 10 additions & 4 deletions test/nodejs/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,9 @@ test('route: routes Monaco with several (duration, distance, nodes) annotations
assert.ok(first.routes[0].legs.every(l => { return l.annotation.distance;}), 'every leg has annotations for distance');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.duration;}), 'every leg has annotations for durations');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.nodes;}), 'every leg has annotations for nodes');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'has no annotations for weight')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'has no annotations for datasources')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'has no annotations for speed')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'has no annotations for weight');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'has no annotations for datasources');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'has no annotations for speed');

options.overview = 'full';
osrm.route(options, function(err, full) {
Expand All @@ -303,7 +303,7 @@ test('route: routes Monaco with several (duration, distance, nodes) annotations
});

test('route: routes Monaco with options', function(assert) {
assert.plan(11);
assert.plan(17);
var osrm = new OSRM(monaco_path);
var options = {
coordinates: two_test_coordinates,
Expand All @@ -322,6 +322,12 @@ test('route: routes Monaco with options', function(assert) {
assert.ok(first.routes[0].legs[0]);
assert.ok(first.routes[0].legs.every(l => { return l.steps.length > 0; }), 'every leg has steps');
assert.ok(first.routes[0].legs.every(l => { return l.annotation;}), 'every leg has annotations');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.distance;}), 'every leg has annotations for distance');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.duration;}), 'every leg has annotations for durations');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.nodes;}), 'every leg has annotations for nodes');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'every leg has annotations for weight');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'every leg has annotations for datasources');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'every leg has annotations for speed');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw problem was happening only for speed, because we have these checks here which it seems were added for backward compatibility, but I am not sure if we should use parameters.annotations_type(but not requested_annotations) when checking if speed annotations were requested.

osrm-backend/include/engine/api/route_api.hpp

Line 770 in 5e5f1f4

requested_annotations = RouteParameters::AnnotationsType::All;

Screenshot 2022-10-18 at 18 35 51

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I am not sure if we should use parameters.annotations_type(but not requested_annotations) when checking if speed annotations were requested.

It looks as a bug, because it is different from other annotations, so I changed it to use requested_annotations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks like it.


options.overview = 'full';
osrm.route(options, function(err, full) {
Expand Down