search

gis-ops / docker-valhalla

This is our flexible Docker repository for the Valhalla routing engine
MIT License
249 stars 71 forks source link

fixed a bug which caused build_elevation not working properly #79

Closed alisoufali closed 1 year ago

alisoufali commented 1 year ago

It seems that line 27 in configure_valhalla.sh sets the do_elevation variable to be "True" whenever all values of max_x, min_x, max_y and min_y are defined. Now it's been corrected in a way that do_elevation will be set to True if and only if all 4 values of mentioned variables are set and build_elevation was set to True or Force

nilsnolde commented 1 year ago

Before introducing the build_elevation env var, you could only build elevation by setting those coord vars which would download all tiles intersecting that bbox. Then we implemented the --from-tiles option in upstream Valhalla and I changed our image's logic to only pull tiles which are covering the graph and for backwards-compatibility I also respect the previous env vars.

Bottom line: I think you're right, we should set do_elevation = True when only one of the coord vars has a value, not all of them. That'd also better reflect backwards-compatibility, since all vars defaulted to their respective maxima. But I disagree that they need all to be present. The coords are not even used anywhere (currently), so that wouldn't make sense IMO.

Also it brings up another lack of backwards-compatibility: while it's fine for only routing to just have the elevation tiles the road graph covers, one might also want to use /height for the whole planet. That's currently not possible. We could re-introduce the coord vars and change build_elevation to take graph, bbox (I think we could skip geojson for now) and if it's bbox we require the coord vars as well and take them as input to valhalla_build_elevation.

nilsnolde commented 1 year ago

closing as stale, feel free to re-open if the need arises again