Isochrone PBF output and multipolygon support #4575

chrstnbwnkl · 2024-02-08T16:21:47Z

Issue

This PR builds on #4543 but takes it step further by grouping contour rings into multipolygons by means of pont in polygon checks.

Tasklist

Add tests
Update the docs with any new request parameters or changes to behavior described
Update the changelog

…iso-pbf

chrstnbwnkl · 2024-02-08T16:25:24Z

proto/isochrone.proto

+    repeated Geometry geometries = 2; // if polygon first one is outer rest are inners, though this is a problem when we allow multi location isochrones
+  }
+
+  message Interval {


Rather than just having a repeated Contour field that each holds a ring (or linestring), there are multiple intervals (one for each requested contour), each of which holds several contours (i.e. linestrings or polygons). If linestrings were requested, there'll be just one geometry; if polygons were requested the geometry field holds a number of rings, where the first is the exterior ring and the remaning are inner rings.

Especially useful for multi-location, but also for separated contours (e.g. when a ferry edge is part of the expansion)

src/tyr/isochrone_serializer.cc

chrstnbwnkl · 2024-02-08T16:31:07Z

src/tyr/isochrone_serializer.cc

+    case Options_Format_pbf:
+      return serializeIsochronePbf(request, intervals, contours);
+    case Options_Format_json:
+      if (polygons)


This still needs to change, probably should keep the current serialization as the default and only use the new method when explicitly requested by the user.

chrstnbwnkl · 2024-02-08T16:31:54Z

test/isochrone.cc

+        ASSERT_EQ(actual_poly_size, expected_poly_size);
+        for (uint32_t j = 0; j < actual_poly_size; ++j) {
+
+          // we only compare exterior rings


I noticed we never compare full geometry, as we leave out the holes. maybe check those as well?

Let's do that

chrstnbwnkl · 2024-02-12T12:22:38Z

src/tyr/isochrone_serializer.cc

+  // iterate over outer rings and for each inner ring check if the inner ring is within the outer ring
+  for (const auto inner : inner_ptrs) {
+    // construct bbox
+    AABB2<PointLL> inner_bbox(std::vector(inner->rbegin(), inner->rend()));


AABB2 constructor expects vector, not list. there's probably a better way to solve this 😄

nilsnolde · 2024-02-12T12:27:48Z

One bigger question to be discussed: would this warrant a request param to enable proper multipolygons and keep the "legacy"/current behavior by default?

IMO our current GeoJSON polygon support is broken and this is actually fixing that bug. Apart from the fact that it's contours and not polygons and it's a crutch to begin with. But if we offer this we should do it as correctly as we can.

Most libs seem to be forgiving enough to render them half ok-ish even now (with multiple outer rings per polygon feature..). Also I'd guess 99% of consumers use some library's GeoJSON implementation, feeding in just the full thing and getting lucky rendering it properly, so nothing would change for them. As @chrstnbwnkl found out, our own web app at https://github.com/gis-ops/valhalla-app needs a bit of work to handle this, but that's just because we weren't doing using leaflet properly for GeoJSON in the first place (using L.polygon() instead of L.GeoJSON()).

kevinkreiser · 2024-02-12T15:50:50Z

src/tyr/isochrone_serializer.cc

+    return results;
+  }
+
+  // iterate over outer rings and for each inner ring check if the inner ring is within the exterior


the check can simply take a single point from the inner and do point in polygon on a given outer. i would consider using the simplified winding number algorithm as proposed in the graphics gems book under "An Incremental Angle Point in Polygon Test". here's a sample implementation of that: https://jeremytammik.github.io/tbc/a/0491_point_in_poly.htm

Thanks for the resource! I gave it a shot, and placed the implentation in midgard/util.cc, maybe there's a better place to put it though

kevinkreiser

looks excellent pbf support and multipolygon with corrected inner outer relationships is awesome

…rone response polygons

…la into cb-isochrone-polygons

…quest with a hole

chrstnbwnkl · 2024-02-15T09:39:57Z

So there's probably some floating point issue in the ARM build again that impacts the contouring. Shape tolerance in the isochrone test is exceeded by a bit, but I'm hesitant to increase it again, since I just did that not too long ago in #4463.

nilsnolde · 2024-02-15T10:10:12Z

I don’t think we can do too much here and have to deal with it as it comes

nilsnolde

good stuff:) just a few nits here and there from my side

nilsnolde · 2024-02-15T12:01:05Z

docs/docs/api/isochrone/api-reference.md

+* ~~Generating outer contours or contours with interior holes for regions that cannot be accessed within the specified time.~~
 * ~~Options to control the minimum size of interior holes.~~
-* Removing self intersections from polygonal contours.
+* ~~Removing self intersections from polygonal contours.~~


nilsnolde · 2024-02-15T12:03:57Z

src/tyr/isochrone_serializer.cc

+  // if the user requested linestrings, we'll give them linestrings
+  if (!polygons || contours.size() < 2) {
+    std::for_each(contours.begin(), contours.end(),
+                  [&results](const contour_t& c) { results.push_back({&c}); });


probably we can reserve here with results.reserve(contours.size()?

They're nested vectors, of which we don't know the size before runtime, so not sure this makes any difference

You’re right, didn’t see the nesting

nilsnolde · 2024-02-15T12:07:39Z

src/tyr/isochrone_serializer.cc

+
+  // iterate over outer rings and for each inner ring check if the inner ring is within the exterior
+  // ring
+  for (const auto inner : inner_ptrs) {


Suggested change

for (const auto inner : inner_ptrs) {

for (const auto* inner : inner_ptrs) {

it's easy to get confused if we hide stuff in auto

nilsnolde · 2024-02-15T12:08:39Z

src/tyr/isochrone_serializer.cc

+    bool found_exterior = false;
+    // go over exterior rings from smallest to largest
+    for (size_t i = results.size(); i > 0; --i) {
+      contour_t ext = *results[i - 1][0];


if you don't want the pointer, rather take a ref to the underlying value or you do a copy

Suggested change

contour_t ext = *results[i - 1][0];

const contour_t& ext = *results[i - 1][0];

nilsnolde · 2024-02-15T12:10:06Z

src/midgard/util.cc

+
+template bool point_in_poly<valhalla::midgard::PointLL, std::list<valhalla::midgard::PointLL>>(
+    const valhalla::midgard::PointLL&,
+    std::list<valhalla::midgard::PointLL>&);


the polygon is not const?

whoops, updated that

nilsnolde · 2024-02-15T12:12:11Z

src/tyr/isochrone_serializer.cc

+    if (!found_exterior) {
+      LOG_WARN("No exterior ring contour found for inner contour.");
+    }


did you observe this? probably could happen in the contouring algo?

Haven't observed this yet, even after excessive playing around (with different graph extracts even)

nilsnolde · 2024-02-15T12:16:25Z

src/tyr/isochrone_serializer.cc

+  for (const auto inner : inner_ptrs) {
+
+    // get the first point of the ring (could be any though)
+    PointLL inner_pt = inner->front();


could also take a (const) reference here

chrstnbwnkl and others added 9 commits February 2, 2024 11:22

first shot at pbf output for isochrone

e68f2f1

use sint32, clean up a little

e486dbb

changelog

8dee342

removed message remnant

47e7e48

Merge branch 'master' into cb-iso-pbf

9ebf63f

comments

6c37b1f

Merge branch 'cb-iso-pbf' of github.com:chrstnbwnkl/valhalla into cb-…

366e90c

…iso-pbf

first shot at multiple polygon output for isochrone

c93128e

Merge branch 'master' into cb-isochrone-polygons

026b7d7

chrstnbwnkl commented Feb 8, 2024

View reviewed changes

src/tyr/isochrone_serializer.cc Outdated Show resolved Hide resolved

chrstnbwnkl commented Feb 8, 2024

View reviewed changes

chrstnbwnkl requested a review from nilsnolde February 8, 2024 16:32

chrstnbwnkl added 2 commits February 8, 2024 18:25

clang tidy

1b2dba5

tidy, again

efb65fd

chrstnbwnkl mentioned this pull request Feb 9, 2024

Enable pbf output for isochrone endpoint #4543

Closed

5 tasks

forgot to pass show_locations

103e452

chrstnbwnkl commented Feb 12, 2024

View reviewed changes

chrstnbwnkl and others added 3 commits February 12, 2024 16:24

indicate packed varint field, use point in poly check for grouping

98aa615

update docs

0318df0

Merge branch 'master' into cb-isochrone-polygons

0a4494b

chrstnbwnkl marked this pull request as ready for review February 12, 2024 15:34

changelog

45ed7ca

chrstnbwnkl requested a review from kevinkreiser February 12, 2024 15:40

kevinkreiser reviewed Feb 12, 2024

View reviewed changes

chrstnbwnkl added 3 commits February 12, 2024 16:59

remove old geojson serialization logic

ecb95d1

implement simplified winding number algorithm

d45b193

typo

9eb7eb6

chrstnbwnkl requested a review from kevinkreiser February 13, 2024 11:44

chrstnbwnkl added 2 commits February 14, 2024 11:33

Merge branch 'master' into cb-isochrone-polygons

4694952

Merge branch 'master' into cb-isochrone-polygons

305f67f

kevinkreiser previously approved these changes Feb 14, 2024

View reviewed changes

chrstnbwnkl added 2 commits February 14, 2024 16:19

return hex string from getIntervalColor function, test holes in isoch…

a1cb8a6

…rone response polygons

Merge branch 'cb-isochrone-polygons' of github.com:chrstnbwnkl/valhal…

9b1e5f1

…la into cb-isochrone-polygons

chrstnbwnkl dismissed kevinkreiser’s stale review via 9b1e5f1 February 14, 2024 15:19

chrstnbwnkl added 3 commits February 14, 2024 17:12

forgot to remove unused polygon bool param in test code

f1606da

multipoint was not correctly tested for shape equality

8846e78

small oversight in the ring comparison, plus a more robust example re…

447e2ac

…quest with a hole

increase hausdorff distance tolerance again

8360058

nilsnolde reviewed Feb 15, 2024

View reviewed changes

chrstnbwnkl and others added 3 commits February 16, 2024 09:22

nils comments

9744cb7

Merge branch 'master' into cb-isochrone-polygons

ce5fce3

Merge branch 'master' into cb-isochrone-polygons

5383cb8

nilsnolde approved these changes Feb 19, 2024

View reviewed changes

kevinkreiser approved these changes Feb 19, 2024

View reviewed changes

kevinkreiser merged commit 812386f into valhalla:master Feb 19, 2024
7 checks passed

chrstnbwnkl mentioned this pull request Feb 20, 2024

Proper support for Polygons and MultiPolygons nilsnolde/valhalla-qgis-plugin#20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isochrone PBF output and multipolygon support #4575

Isochrone PBF output and multipolygon support #4575

chrstnbwnkl commented Feb 8, 2024 •

edited

Loading

chrstnbwnkl Feb 8, 2024 •

edited

Loading

chrstnbwnkl Feb 8, 2024

chrstnbwnkl Feb 8, 2024

chrstnbwnkl Feb 14, 2024

chrstnbwnkl Feb 12, 2024

nilsnolde commented Feb 12, 2024 •

edited

Loading

kevinkreiser Feb 12, 2024 •

edited

Loading

chrstnbwnkl Feb 13, 2024

kevinkreiser left a comment

chrstnbwnkl commented Feb 15, 2024 •

edited

Loading

nilsnolde commented Feb 15, 2024

nilsnolde left a comment

nilsnolde Feb 15, 2024

nilsnolde Feb 15, 2024

chrstnbwnkl Feb 15, 2024 •

edited

Loading

nilsnolde Feb 15, 2024

nilsnolde Feb 15, 2024

nilsnolde Feb 15, 2024

nilsnolde Feb 15, 2024

chrstnbwnkl Feb 16, 2024

nilsnolde Feb 15, 2024

chrstnbwnkl Feb 16, 2024

nilsnolde Feb 15, 2024

	for (const auto inner : inner_ptrs) {
	for (const auto* inner : inner_ptrs) {

	contour_t ext = *results[i - 1][0];
	const contour_t& ext = *results[i - 1][0];

Isochrone PBF output and multipolygon support #4575

Isochrone PBF output and multipolygon support #4575

Conversation

chrstnbwnkl commented Feb 8, 2024 • edited Loading

Issue

Tasklist

chrstnbwnkl Feb 8, 2024 • 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

Choose a reason for hiding this comment

nilsnolde commented Feb 12, 2024 • edited Loading

kevinkreiser Feb 12, 2024 • edited Loading

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

chrstnbwnkl commented Feb 15, 2024 • edited Loading

nilsnolde commented Feb 15, 2024

nilsnolde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrstnbwnkl Feb 15, 2024 • 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

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

chrstnbwnkl commented Feb 8, 2024 •

edited

Loading

chrstnbwnkl Feb 8, 2024 •

edited

Loading

nilsnolde commented Feb 12, 2024 •

edited

Loading

kevinkreiser Feb 12, 2024 •

edited

Loading

chrstnbwnkl commented Feb 15, 2024 •

edited

Loading

chrstnbwnkl Feb 15, 2024 •

edited

Loading