add shape to matrix response #4432

nilsnolde · 2023-12-04T20:16:06Z

based on #4372

implements a new shape_format request field for /sources_to_targets with all available formats (polyline5/6, geojson and by default "none"), but only for CostMatrix. shape_format will dictate what the shape response field contains, if not "none" in which case there is no shape response field.

note: trivial routes have a bug in master (and the PR this is based on), where it finds the connection on the wrong directed edge, consistently. I guess this went unnoticed for so long because it hardly matters for the matrix. the distance will always be the same, thought the duration can of course vary. I'll band-aid that for now.

…es:)

…elf doesn't segfault

…ansion with deadend logic; still could/should be based on max cost of (which?) adjacency set

kevinkreiser · 2023-12-08T13:48:17Z

valhalla/midgard/util.h

@@ -15,6 +15,7 @@
 #include <utility>
 #include <vector>

+#include <valhalla/baldr/json.h>


this is not a good idea. it works for now because json.h is header only so you dont have to link the bit of the library that is baldr but id really really rather not mix baldr into midgard.

anyway i think you dont need this? nothing is using it here anyway right?

oops, must be a relict from before. I moved the function which needed it to tyr/serializers.h

kevinkreiser · 2023-12-08T13:48:37Z

valhalla/thor/matrix_common.h

@@ -126,6 +126,7 @@ inline void reserve_pbf_arrays(valhalla::Matrix& matrix, size_t size) {
  matrix.mutable_distances()->Resize(size, 0U);
  matrix.mutable_times()->Resize(size, 0U);
  matrix.mutable_date_times()->Reserve(size);
+  matrix.mutable_shapes()->Reserve(size);


kevinkreiser · 2023-12-08T13:49:37Z

src/tyr/serializers.cc

+        baldr::json::array({baldr::json::fixed_t{p.lng(), 6}, baldr::json::fixed_t{p.lat(), 6}}));
+  }
+  geojson->emplace("type", std::string("LineString"));
+  geojson->emplace("coordinates", std::move(coords));


its all shared pointers so move isnt really needed, just an fyi

right, I'll change that then

kevinkreiser · 2023-12-08T13:50:05Z

some tiny things to change otherwise looks done

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

kevinkreiser · 2023-12-09T17:09:02Z

just a little clang tidy and then done:

nilsnolde · 2023-12-09T17:20:06Z

oops done

kevinkreiser · 2023-12-09T17:32:12Z

merge master and update changelog yet

kevinkreiser · 2023-12-09T17:36:26Z

src/worker.cc

@@ -806,10 +806,17 @@ void from_json(rapidjson::Document& doc, Options::Action action, Api& api) {
      options.set_shape_format(polyline5);
    } else if (*shape_format == "geojson") {
      options.set_shape_format(geojson);
+    } else if (*shape_format == "none") {


oh i didnt notice this in the json api it is still called none

hm that was the case since the beginning. but you're right, it's unintuitive to have them differ in PBF/JSON formats, I'll change it

kevinkreiser · 2023-12-09T17:37:36Z

src/worker.cc

+      if (action == Options::height) {
+        throw valhalla_exception_t{164};
+      }


its not clear to me why we need this. the height action will never look at options.shape_format() right? i think we shouldnt throw errors for superfluous parameters. or what am i missing?

the height action will never look at options.shape_format() right?

yes it does actually, to return either polyline5 or 6. in fact it's the only documented use of shape_format :)

not to return though right! its the input format in that case hahaha, i had forgotten about that!

we probably have to block this then for mapmatching too right?

i should also say i didnt check this but we should have a look quick while we are here

valhalla/src/worker.cc

Lines 839 to 852 in 253443e

// parse map matching location input and encoded_polyline for height actions

auto encoded_polyline = rapidjson::get_optional<std::string>(doc, "/encoded_polyline");

if (encoded_polyline) {

options.set_encoded_polyline(*encoded_polyline);

}

if (options.has_encoded_polyline_case()) {

// Set the precision to use when decoding the polyline. For height actions (only)

// either polyline6 (default) or polyline5 are supported. All other actions only

// support polyline6 inputs at this time.

double precision = 1e-6;

if (options.action() == Options::height) {

precision = options.shape_format() == valhalla::polyline5 ? 1e-5 : 1e-6;

}

looks like mapmatching forces to polyline6. kind of dumb, we should allow it but its a problem for another day

for height as input, right.. for map matching not relevant, it's hard-coded to polyline6 it seems in worker.cc

kevinkreiser

3 small things then merge:

changelog entry at bottom of list
merge master into branch
check if we can remove height action error response

nilsnolde · 2023-12-09T19:26:04Z

last things done, sorry for the hassle..

nilsnolde · 2023-12-09T20:09:15Z

hmpf.. tidy again and a test was broken after the recent changes.. I also remembered I'd like to add a warning when TDMatrix is used but shape_format was requested. since the approve is dismissed anyways, I'll add now.

don't hurry with this really, let's take it on monday real quick.

nilsnolde and others added 30 commits November 11, 2023 16:54

save my work on the failing laptop

fddc888

Merge branch 'master' into nn-matrix-uturns

bf1b3f8

refactor successful, all tests pass and the deadend bug is fixed..

b695688

put the expansion direction into the template arguments

ba04dc3

rename vars

ebfaf0b

do the memory retaining stuff as good as possible

469748c

clean up other algos

c7aba13

fix #4360 and the matrix-off-by-1-second issue

f424dce

put the dependencies back in place in cmakelists

4f2a421

changelog

a151e29

format

c08774c

matrix expansion test also needed an update; good: we expand less edg…

1861861

…es:)

Merge branch 'master' into nn-matrix-uturns

a00560f

oops, edgecost & transition cost were the same for both trees

70788a8

small update

a8f18cd

ha, what a shitshow.. tiny mis-ordering of statements messed it all up..

65759da

last remaining bugs fixed

0c48d72

Merge branch 'master' into nn-matrix-uturns

43a0552

cleanup

a0d0064

initialize interrupt function with nullptr so using costmatrix by its…

329cbd0

…elf doesn't segfault

re-add the additional testing scripts

a2c8d2e

format

9c4912b

increase the maximum threshold by 3% to account for slightly more exp…

d91afb6

…ansion with deadend logic; still could/should be based on max cost of (which?) adjacency set

after fixing the bugs, the expansion test has no changes

ecb8f9f

revert

11d6751

Merge branch 'master' into nn-matrix-uturns

ebb8e82

Merge branch 'master' into nn-matrix-uturns

79b133a

make the PR files warning-free (#4410)

7449ae4

Merge branch 'master' into nn-matrix-uturns

6dcf710

Merge branch 'master' into nn-matrix-uturns

98b6d1c

kevinkreiser reviewed Dec 8, 2023

View reviewed changes

nilsnolde and others added 3 commits December 9, 2023 15:37

Apply suggestions from code review

51f5c1a

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

addressed review

1a01879

address bug for tdmatrix when shape_format -ne 'none'

7580c8d

nilsnolde force-pushed the nn-matrix-geometry branch from 89ec5a8 to 7580c8d Compare December 9, 2023 15:45

tidy

2427e12

nilsnolde requested a review from kevinkreiser December 9, 2023 17:20

kevinkreiser reviewed Dec 9, 2023

View reviewed changes

kevinkreiser previously approved these changes Dec 9, 2023

View reviewed changes

nilsnolde added 2 commits December 9, 2023 20:21

Merge branch 'master' into nn-matrix-geometry

93c3f41

rename 'none' to 'no_shape' in JSON API; fix changelog; merge master

8a4c069

nilsnolde dismissed kevinkreiser’s stale review via 8a4c069 December 9, 2023 19:23

nilsnolde requested a review from kevinkreiser December 9, 2023 19:26

kevinkreiser previously approved these changes Dec 9, 2023

View reviewed changes

nilsnolde added 2 commits December 9, 2023 21:01

again tidy

7c0b6b8

test broke after renaming the JSON API

b3bc2c8

nilsnolde dismissed kevinkreiser’s stale review via b3bc2c8 December 9, 2023 20:06

added warning when tdmatrix was used and shape_format was requested

497507f

kevinkreiser approved these changes Dec 10, 2023

View reviewed changes

nilsnolde merged commit 89b0e2f into master Dec 10, 2023
8 of 9 checks passed

nilsnolde deleted the nn-matrix-geometry branch December 10, 2023 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add shape to matrix response #4432

add shape to matrix response #4432

nilsnolde commented Dec 4, 2023 •

edited

kevinkreiser Dec 8, 2023

nilsnolde Dec 9, 2023

kevinkreiser Dec 8, 2023

kevinkreiser Dec 8, 2023

nilsnolde Dec 9, 2023

kevinkreiser commented Dec 8, 2023

kevinkreiser commented Dec 9, 2023

nilsnolde commented Dec 9, 2023

kevinkreiser commented Dec 9, 2023

kevinkreiser Dec 9, 2023

nilsnolde Dec 9, 2023

kevinkreiser Dec 9, 2023

nilsnolde Dec 9, 2023

kevinkreiser Dec 9, 2023

kevinkreiser Dec 9, 2023

kevinkreiser Dec 9, 2023 •

edited

kevinkreiser Dec 9, 2023

kevinkreiser Dec 9, 2023

nilsnolde Dec 9, 2023

kevinkreiser left a comment

nilsnolde commented Dec 9, 2023

nilsnolde commented Dec 9, 2023

	// parse map matching location input and encoded_polyline for height actions
	auto encoded_polyline = rapidjson::get_optional<std::string>(doc, "/encoded_polyline");
	if (encoded_polyline) {
	options.set_encoded_polyline(*encoded_polyline);
	}
	if (options.has_encoded_polyline_case()) {

	// Set the precision to use when decoding the polyline. For height actions (only)
	// either polyline6 (default) or polyline5 are supported. All other actions only
	// support polyline6 inputs at this time.
	double precision = 1e-6;
	if (options.action() == Options::height) {
	precision = options.shape_format() == valhalla::polyline5 ? 1e-5 : 1e-6;
	}

add shape to matrix response #4432

add shape to matrix response #4432

Conversation

nilsnolde commented Dec 4, 2023 • edited

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

kevinkreiser commented Dec 8, 2023

kevinkreiser commented Dec 9, 2023

nilsnolde commented Dec 9, 2023

kevinkreiser commented Dec 9, 2023

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

kevinkreiser Dec 9, 2023 • edited

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

kevinkreiser left a comment

Choose a reason for hiding this comment

nilsnolde commented Dec 9, 2023

nilsnolde commented Dec 9, 2023

nilsnolde commented Dec 4, 2023 •

edited

kevinkreiser Dec 9, 2023 •

edited