Include 'rate' property (reciprocal of weight) on debug tile edges. #4162
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -428,6 +428,12 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| const auto &edge = edges[edge_index]; | ||
| const auto geometry_id = get_geometry_id(edge); | ||
|
|
||
| // Get coordinates for start/end nodes of segment (NodeIDs u and v) | ||
| const auto a = facade.GetCoordinateOfNode(edge.u); | ||
| const auto b = facade.GetCoordinateOfNode(edge.v); | ||
| // Calculate the length in meters | ||
| const double length = osrm::util::coordinate_calculation::haversineDistance(a, b); | ||
|
|
||
| // Weight values | ||
| const auto forward_weight_vector = | ||
| facade.GetUncompressedForwardWeights(geometry_id); | ||
|
|
@@ -439,6 +445,14 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| use_line_value(forward_weight); | ||
| use_line_value(reverse_weight); | ||
|
|
||
| std::uint32_t forward_rate = | ||
| static_cast<std::uint32_t>(round(length / forward_weight * 10.)); | ||
| std::uint32_t reverse_rate = | ||
| static_cast<std::uint32_t>(round(length / reverse_weight * 10.)); | ||
|
|
||
| use_line_value(forward_rate); | ||
|
There was a problem hiding this comment. I like storing it as int as well here. My first intuition was to store it as double. There was a problem hiding this comment. Yeah - Mapbox GL does some ugly formatting on floats mapbox/mapbox-gl-js#3358, so some of the reasoning for using ints vs doubles in this tile encoder is driven by rendering ugliness :-( |
||
| use_line_value(reverse_rate); | ||
|
|
||
| // Duration values | ||
| const auto forward_duration_vector = | ||
| facade.GetUncompressedForwardDurations(geometry_id); | ||
|
|
@@ -518,6 +532,7 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| &max_datasource_id, | ||
| &used_line_ints](const FixedLine &tile_line, | ||
| const std::uint32_t speed_kmh, | ||
| const std::uint32_t rate, | ||
|
There was a problem hiding this comment. how do you feel about making clearer that all of these are offsets? The first time I though these were supposed to be actual values (like the speed is, since we store all 127 possible values). There was a problem hiding this comment. Yeah, that's a vector tile encoding gotcha - I agree, it's a bit unintuitive if you haven't read the spec. I'll rename these, now is as good a time as any. |
||
| const std::size_t weight, | ||
| const std::size_t duration, | ||
| const DatasourceID datasource, | ||
|
|
@@ -564,6 +579,10 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
|
|
||
| field.add_element(130 + max_datasource_id + 1 + used_line_ints.size() + | ||
| name_idx); // name value offset | ||
|
|
||
| field.add_element(6); // rate tag key offset | ||
| field.add_element(130 + max_datasource_id + 1 + | ||
| rate); // rate goes in used_line_ints | ||
| } | ||
| { | ||
|
|
||
|
|
@@ -584,11 +603,18 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| std::uint32_t speed_kmh = | ||
| static_cast<std::uint32_t>(round(length / forward_duration * 10 * 3.6)); | ||
|
|
||
| // Rate values are in meters per weight-unit - and similar to speeds, we | ||
| // present 1 decimal place of precision (these values are added as | ||
| // double/10) lower down | ||
| std::uint32_t rate = | ||
| static_cast<std::uint32_t>(round(length / forward_weight * 10.)); | ||
|
|
||
| auto tile_line = coordinatesToTileLine(a, b, tile_bbox); | ||
| if (!tile_line.empty()) | ||
| { | ||
| encode_tile_line(tile_line, | ||
| speed_kmh, | ||
| rate, | ||
| line_int_offsets[forward_weight], | ||
| line_int_offsets[forward_duration], | ||
| forward_datasource, | ||
|
|
@@ -609,11 +635,18 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| std::uint32_t speed_kmh = | ||
| static_cast<std::uint32_t>(round(length / reverse_duration * 10 * 3.6)); | ||
|
|
||
| // Rate values are in meters per weight-unit - and similar to speeds, we | ||
| // present 1 decimal place of precision (these values are added as | ||
| // double/10) lower down | ||
| std::uint32_t rate = | ||
| static_cast<std::uint32_t>(round(length / reverse_weight * 10.)); | ||
|
|
||
| auto tile_line = coordinatesToTileLine(b, a, tile_bbox); | ||
| if (!tile_line.empty()) | ||
| { | ||
| encode_tile_line(tile_line, | ||
| speed_kmh, | ||
| rate, | ||
| line_int_offsets[reverse_weight], | ||
| line_int_offsets[reverse_duration], | ||
| reverse_datasource, | ||
|
|
@@ -634,6 +667,7 @@ void encodeVectorTile(const datafacade::ContiguousInternalMemoryDataFacadeBase & | |
| line_layer_writer.add_string(util::vector_tile::KEY_TAG, "weight"); | ||
| line_layer_writer.add_string(util::vector_tile::KEY_TAG, "duration"); | ||
| line_layer_writer.add_string(util::vector_tile::KEY_TAG, "name"); | ||
| line_layer_writer.add_string(util::vector_tile::KEY_TAG, "rate"); | ||
|
|
||
| // Now, we write out the possible speed value arrays and possible is_tiny | ||
| // values. Field type 4 is the "values" field. It's a variable type field, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when trying to refactor this I thought about this line a bit. It adds some overhead for calculating the distances. Whats is you opinion here @danpat on the potential slowdown? I feel it might be alright, but I am not entirely sure. (This length calculation is already happening further down as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super-concerned about performance for things like this - unless there's a really easy way to refactor it, I'm inclined to just eat the cost. If https://github.com/mapbox/vector-tile ever grows encoding ability, we could do a big refactor and clean up stuff like that. Until then, I'm totally OK with there being some technical debt here.