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

GeoTIFF serialization for isochrones #4594

Merged
merged 29 commits into from
Mar 4, 2024

Conversation

chrstnbwnkl
Copy link
Contributor

@chrstnbwnkl chrstnbwnkl commented Feb 20, 2024

Issue

Adds GeoTIFF as an output format to the isochrones endpoint, which returns the 2-D grid as a raster instead of contours.

image

The rough idea is this: when a user requests a raster output by specifing "format": "geotiff" in the request JSON, once the 2-D expansion grid is calculated, the grid is serialized as a GeoTIFF dataset and returned as raw bytes, instead of running the contouring algorithm.

Implementation Summary

  • uses GDAL for writing the raster dataset; when building Valhalla, one needs to actively opt in to this feature by setting the cmake flag ENABLE_GDAL (OFF by default)
  • raster data type: using uint16_t gives plenty range for distance and time metrics, so I decided to scale the values to seconds (which maxes out at ~18 hours) and 10 meter steps (~650 kms)
  • each metric gets its own band
  • the data grid has an immense padding, which leaves more than 50% of cells never reached by the expansion (in most cases probably more than 80% though), so the final raster is truncated to the minimum extent that actually contains values not equal to the set max value, plus just a little padding that reassures users we didn't accidentally cut off part of the grid 😅

For more info, see the initial discussion: #4581.

TODO

There are still one or two things left to do:

  • figure out a way to return the file buffer as a string without copying it
  • find out why raster values in the added tests are unexpectedly high (probably related to type casting)

Future steps

This is a basic first step, but it doesn't have to be the end of it. One could

  • add color information for renderers by making use of color tables (I have fiddled around with it but haven't gotten far 😅)
  • add some more metadata that can be useful for the ouput interpretation, e.g. about the snapped locations (analogous to show_locations)

Tasklist

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

@@ -357,6 +357,12 @@ loki_worker_t::work(const std::list<zmq::message_t>& job,
result.messages.emplace_back(request.SerializeAsString());
break;
case Options::isochrone:
// return early if geotiff was requested but GDAL isn't enabled
#ifndef ENABLE_GDAL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

be prepared to see plenty of these... I'm open to any suggestions that reduce these wherever possible

#ifdef ENABLE_GDAL
#include <gdal_priv.h>
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tyr has no way to maintain state, so thor_worker_t will hold the geotiff driver and pass an opaque pointer to it to the serializer function (note that GDAL is not thread-safe)

@chrstnbwnkl chrstnbwnkl marked this pull request as ready for review February 20, 2024 17:21
Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

great stuff, thanks!

not sure I'll join tmrw for the meeting so I left some comments here. maybe there's some way to hide the gdal headers in the implementation instead of exposing them in our headers. anyways, kevin is the master pimpl'er here :D

src/tyr/isochrone_serializer.cc Outdated Show resolved Hide resolved
valhalla/thor/worker.h Outdated Show resolved Hide resolved
src/valhalla_run_isochrone.cc Show resolved Hide resolved
src/worker.cc Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/ValhallaPkgConfig.cmake Outdated Show resolved Hide resolved
docs/docs/api/turn-by-turn/api-reference.md Show resolved Hide resolved
@nilsnolde
Copy link
Member

maybe @dnesbitt61 you want to look at this too, kevin mentioned you had this idea before too:)

src/proto_conversions.cc Outdated Show resolved Hide resolved
This reverts commit 657aa59.
@nilsnolde
Copy link
Member

so it seems debian & fedora based distros install gdal headers to /usr/include/gdal/ while pacman/arch/pkgbuild, brew & vcpkg install it to /usr/include/ directly. while I'd more agree with the former pattern (gdal has tons of installed headers), the latter one is coming from gdal's own cmake. I wouldn't really like to hack the cmake cxx/c flags for gdal. but maybe I'm not quite at full cognitive height anymore.. I asked upstream: OSGeo/gdal#9276

@nilsnolde
Copy link
Member

nilsnolde commented Feb 21, 2024

yeah, ubuntu packaging messes up the .pc file: OSGeo/gdal#9276 (comment) nope, got fixed in gdal 3.8.0. anyways, let's then use cmake's find_package

@nilsnolde
Copy link
Member

uh oh, windows is failing with some msvc specific stuff 😨

@chrstnbwnkl
Copy link
Contributor Author

Is tidy hallucinating or am I?

nilsnolde
nilsnolde previously approved these changes Feb 22, 2024
@kevinkreiser kevinkreiser merged commit 3794239 into valhalla:master Mar 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants