Add bike elevation profiles #26
Conversation
src/elevation_storage.cc
Outdated
| {elevation_files::kDataName, elevation_files::kIndexName}) { | ||
| auto const full_path = path / filename; | ||
| if (!fs::exists(full_path)) { | ||
| std::cout << full_path << " does not exist\n"; |
There was a problem hiding this comment.
maybe std::clog so it shows up in data/logs/osr.txt?
There was a problem hiding this comment.
Sounds great. I will change it.
| } | ||
|
|
||
| struct elevation_storage { | ||
| struct elevation { |
There was a problem hiding this comment.
why another type? can't encoding be renamed to elevation and return elevation_monotonic_t with get functions?
There was a problem hiding this comment.
I'm not sure, if this will be too strict in the long run. But my code might be is too complicated indeed, as there are different kinds of elevations.
- directional elevation data, containing both up and down elevation (
elevation)- underlying type for each direction (
elevation_monotonic_t, i.e.uint16_t)
- underlying type for each direction (
- compressed data (
encoding) - data format specific elevation values (
int16_tfor HGT and BIL)
If we only care about elevations up, the elevation type could be removed. But I think, there could be some usage for elevations down in some profiles as well. Furthermore we can provide these for the resulting path.
| }; | ||
|
|
||
| struct dem_tile { | ||
| explicit dem_tile(fs::path const& filename); |
There was a problem hiding this comment.
I guess it's not the filename but a full absolute or relative path?
There was a problem hiding this comment.
You are right. Should have been path instead. I have updated it.
|
|
||
| hgt_driver() = default; | ||
| bool add_tile(fs::path const&); | ||
| elevation_meters_t get(point const&) const; |
There was a problem hiding this comment.
point fits in a register on 64bit architectures (=main target) - so it should be passed by value
| std::unique_ptr<impl> impl_; | ||
| }; | ||
|
|
||
| extern template struct hgt_tile<3601U>; |
There was a problem hiding this comment.
You are right. It doesn't seem necessary.
By the way, any comments about the separation? I couldn't find many examples for explicit template instantiation. So I created include/osr/preprocessing/elevation/hgt_tile_def.h and src/preprocessing/elevation/hgt_tile.cc. But I probably could have put everything into src/preprocessing/elevation/hgt_tile.cc instead.
|
|
||
| elevation_meters_t get(std::size_t const offset) const { | ||
| assert(offset < kBytesPerPixel * RasterSize * RasterSize); | ||
| auto const byte_ptr = file_.data() + offset; |
There was a problem hiding this comment.
utl::verify that this is not out of bounds?
maybe once in the constructor is sufficient?
There was a problem hiding this comment.
For HGT tiles checking the file size during the constructor should be sufficient, as one cannot pass arbitrary offsets. For BIL tiles I need to apply some more changes, but it should be doable as well.
| using elevation_meters_t = | ||
| cista::strong<std::int16_t, struct elevation_meters_>; | ||
|
|
||
| struct coord_box { |
There was a problem hiding this comment.
I will have a look. I think, there first was some reason, but it doesn't seem relevant any longer.
src/route.cc
Outdated
| if (target == to && cost == expected_cost) { | ||
| auto const is_loop = way != way_idx_t::invalid() && r.is_loop(way) && | ||
| static_cast<unsigned>(std::abs(a_idx - b_idx)) == | ||
| r.way_nodes_[way].size() - 2U; | ||
| conn = {way, a_idx, b_idx, is_loop, dist}; | ||
| conn = {way, a_idx, b_idx, is_loop, dist, std::move(elevation)}; |
There was a problem hiding this comment.
does move make sense here?
There was a problem hiding this comment.
I think, there are some instances, where std::move() isn't needed and the data type is quite small as well. I'll check them and change if possible.
| @@ -90,6 +90,8 @@ using multi_level_elevator_idx_t = | |||
| cista::strong<std::uint32_t, struct multi_level_elevator_idx_>; | |||
|
|
|||
| using distance_t = std::uint16_t; | |||
| using elevation_monotonic_t = | |||
There was a problem hiding this comment.
what does monotonic mean here?
There was a problem hiding this comment.
I intended to use store and down elevations along each way, without annihilating each other. Therefore each variable (up and down) should only increase, i.e. be monotonic. I didn't have a better idea for naming it though.
| auto const lat = p.lat(); | ||
| auto const lng = p.lng(); | ||
| auto const box = get_box(); | ||
| if (box.min_.lng_ <= lng && lng < box.max_.lng_ && // |
There was a problem hiding this comment.
You are right. I didn't implement it for my type. But now, with geo::box, I can use it …
| auto const min = decltype(rtree_)::coord_t{static_cast<float>(box.min_.lat_), | ||
| static_cast<float>(box.min_.lng_)}; | ||
| auto const max = decltype(rtree_)::coord_t{static_cast<float>(box.max_.lat_), | ||
| static_cast<float>(box.max_.lng_)}; |
There was a problem hiding this comment.
https://github.com/motis-project/geo/blob/master/include/geo/latlng.h#L29-L32
I always use lnglat float or lnglat (watch out.. it's inverted order)
There was a problem hiding this comment.
Thank you for suggesting this. This will make the code much clearer.
The swapped order isn't a problem here, as I can use lnglat_float for all instances where the R-tree is accessed.
Using geo::latlng will avoid recurring conversions from point to geo::latlng in multiple methods, making the code more readable.
This adds elevation profiles for bike routing.
When a bike elevation profile is used, routes with a lower elevation difference but longer distance will be preferred. This parameter is configurable.
For using this feature, additional elevation files are required.
Currently only elevation data in BIL file format are supported, e.g. SRTM data.