Include 'rate' property (reciprocal of weight) on debug tile edges. #4162
Conversation
| 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); |
There was a problem hiding this comment.
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.
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.
| 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 :-(
src/engine/plugins/tile.cpp
Outdated
| @@ -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.
| @@ -429,6 +431,7 @@ Vector tiles contain two layers: | |||
| | `bearing_in` | `integer` | the absolute bearing that approaches the intersection. -180 to +180, 0 = North, 90 = East | | |||
| | `turn_angle` | `integer` | the angle of the turn, relative to the `bearing_in`. -180 to +180, 0 = straight ahead, 90 = 90-degrees to the right | | |||
| | `cost` | `float` | the time we think it takes to make that turn, in seconds. May be negative, depending on how the data model is constructed (some turns get a "bonus"). | | |||
| | `weight` | `float` | the weight we think it takes to make that turn. May be negative, depending on how the data model is constructed (some turns get a "bonus"). ACTUAL ROUTING USES THIS VALUE | | |||
There was a problem hiding this comment.
Just a guess: should these also contain rate or don't we need it here?
There was a problem hiding this comment.
There's no rate on point features, the turn penalty values are simply additive numbers. The rate only applies on edges so that we have an easy way to visually compare (e.g. via colour) edges of differing lengths.
… table indexes, not actual values.
Issue
This PR is to address #4018 where we're not exposing the reciprocal of the
weightproperty (analogous tospeedif doing time-based routing).This PR exposes a new property called
ratefor each edge. Similar tospeed, it's encoded to 1 decimal place of precision, and the value is inmeters per unit(weight), whereweightis the internal unit-less cost of edge traversal.ALSO, this PR exposes the turn weight property for the turn layer. Given that actual routing occurs using weights, it'd be helpful to expose the
weightvalue for turns in addition to thecost(duration) (which is only used for ETA calculation).Tasklist