Skip to content

Commit 5d468f2

Browse files
mjjbellmjjbell
authored
Make edge metrics strongly typed (#6421)
This change takes the existing typedefs for weight, duration and distance, and makes them proper types, using the existing Alias functionality. Primarily this is to prevent bugs where the metrics are switched, but it also adds additional documentation. For example, it now makes it clear (despite the naming of variables) that most of the trip algorithm is running on the duration metric. I've not made any changes to the casts performed between metrics and numeric types, they now just more explicit.
1 parent 16685d0 commit 5d468f2

File tree

69 files changed

+927
-691
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

69 files changed

+927
-691
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
- FIXED: Handle snapping parameter for all plugins in NodeJs bindings, but not for Route only. [#6417](https://github.com/Project-OSRM/osrm-backend/pull/6417)
77
- FIXED: Fix annotations=true handling in NodeJS bindings & libosrm. [#6415](https://github.com/Project-OSRM/osrm-backend/pull/6415/)
88
- FIXED: Fix bindings compilation issue on the latest Node. Update NAN to 2.17.0. [#6416](https://github.com/Project-OSRM/osrm-backend/pull/6416)
9+
- CHANGED: Make edge metrics strongly typed [#6420](https://github.com/Project-OSRM/osrm-backend/pull/6420)
910
- Routing:
1011
- FIXED: Fix adding traffic signal penalties during compression [#6419](https://github.com/Project-OSRM/osrm-backend/pull/6419)
1112
# 5.27.1

‎include/contractor/contractor_graph.hpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ namespace contractor
1212
struct ContractorEdgeData
1313
{
1414
ContractorEdgeData()
15-
: weight(0), duration(0), distance(0), id(0), originalEdges(0), shortcut(0), forward(0),
15+
: weight{0}, duration{0}, distance{0}, id(0), originalEdges(0), shortcut(0), forward(0),
1616
backward(0)
1717
{
1818
}
1919
ContractorEdgeData(EdgeWeight weight,
20-
EdgeWeight duration,
20+
EdgeDuration duration,
2121
EdgeDistance distance,
2222
unsigned original_edges,
2323
unsigned id,
@@ -30,7 +30,7 @@ struct ContractorEdgeData
3030
{
3131
}
3232
EdgeWeight weight;
33-
EdgeWeight duration;
33+
EdgeDuration duration;
3434
EdgeDistance distance;
3535
unsigned id;
3636
unsigned originalEdges : 29;

‎include/contractor/graph_contractor_adaptors.hpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,18 +29,20 @@ ContractorGraph toContractorGraph(NodeID number_of_nodes, InputEdgeContainer inp
2929

3030
#ifndef NDEBUG
3131
const unsigned int constexpr DAY_IN_DECI_SECONDS = 24 * 60 * 60 * 10;
32-
if (static_cast<unsigned int>(std::max(input_edge.data.weight, 1)) > DAY_IN_DECI_SECONDS)
32+
if (from_alias<unsigned int>(std::max(input_edge.data.weight, EdgeWeight{1})) >
33+
DAY_IN_DECI_SECONDS)
3334
{
3435
util::Log(logWARNING) << "Edge weight large -> "
35-
<< static_cast<unsigned int>(std::max(input_edge.data.weight, 1))
36+
<< from_alias<unsigned int>(
37+
std::max(input_edge.data.weight, EdgeWeight{1}))
3638
<< " : " << static_cast<unsigned int>(input_edge.source) << " -> "
3739
<< static_cast<unsigned int>(input_edge.target);
3840
}
3941
#endif
4042
edges.emplace_back(input_edge.source,
4143
input_edge.target,
42-
std::max(input_edge.data.weight, 1),
43-
input_edge.data.duration,
44+
std::max(input_edge.data.weight, {1}),
45+
to_alias<EdgeDuration>(input_edge.data.duration),
4446
input_edge.data.distance,
4547
1,
4648
input_edge.data.turn_id,
@@ -50,8 +52,8 @@ ContractorGraph toContractorGraph(NodeID number_of_nodes, InputEdgeContainer inp
5052

5153
edges.emplace_back(input_edge.target,
5254
input_edge.source,
53-
std::max(input_edge.data.weight, 1),
54-
input_edge.data.duration,
55+
std::max(input_edge.data.weight, {1}),
56+
to_alias<EdgeDuration>(input_edge.data.duration),
5557
input_edge.data.distance,
5658
1,
5759
input_edge.data.turn_id,
@@ -109,19 +111,19 @@ ContractorGraph toContractorGraph(NodeID number_of_nodes, InputEdgeContainer inp
109111
// merge edges (s,t) and (t,s) into bidirectional edge
110112
if (forward_edge.data.weight == reverse_edge.data.weight)
111113
{
112-
if ((int)forward_edge.data.weight != INVALID_EDGE_WEIGHT)
114+
if (forward_edge.data.weight != INVALID_EDGE_WEIGHT)
113115
{
114116
forward_edge.data.backward = true;
115117
edges[edge++] = forward_edge;
116118
}
117119
}
118120
else
119121
{ // insert seperate edges
120-
if (((int)forward_edge.data.weight) != INVALID_EDGE_WEIGHT)
122+
if (forward_edge.data.weight != INVALID_EDGE_WEIGHT)
121123
{
122124
edges[edge++] = forward_edge;
123125
}
124-
if ((int)reverse_edge.data.weight != INVALID_EDGE_WEIGHT)
126+
if (reverse_edge.data.weight != INVALID_EDGE_WEIGHT)
125127
{
126128
edges[edge++] = reverse_edge;
127129
}
@@ -157,7 +159,7 @@ template <class Edge, typename GraphT> inline std::vector<Edge> toEdges(GraphT g
157159
new_edge.target = target;
158160
BOOST_ASSERT_MSG(SPECIAL_NODEID != new_edge.target, "Target id invalid");
159161
new_edge.data.weight = data.weight;
160-
new_edge.data.duration = data.duration;
162+
new_edge.data.duration = from_alias<EdgeDuration::value_type>(data.duration);
161163
new_edge.data.distance = data.distance;
162164
new_edge.data.shortcut = data.shortcut;
163165
new_edge.data.turn_id = data.id;

‎include/contractor/query_edge.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,15 @@ struct QueryEdge
1717
struct EdgeData
1818
{
1919
explicit EdgeData()
20-
: turn_id(0), shortcut(false), weight(0), duration(0), forward(false), backward(false),
21-
distance(0)
20+
: turn_id(0), shortcut(false), weight{0}, duration(0), forward(false),
21+
backward(false), distance{0}
2222
{
2323
}
2424

2525
EdgeData(const NodeID turn_id,
2626
const bool shortcut,
2727
const EdgeWeight weight,
28-
const EdgeWeight duration,
28+
const EdgeDuration duration,
2929
const EdgeDistance distance,
3030
const bool forward,
3131
const bool backward)
@@ -50,7 +50,7 @@ struct QueryEdge
5050
NodeID turn_id : 31;
5151
bool shortcut : 1;
5252
EdgeWeight weight;
53-
EdgeWeight duration : 30;
53+
EdgeDuration::value_type duration : 30;
5454
std::uint32_t forward : 1;
5555
std::uint32_t backward : 1;
5656
EdgeDistance distance;

‎include/customizer/cell_customizer.hpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ class CellCustomizer
6161
}
6262
}
6363
heap.Clear();
64-
heap.Insert(source, 0, {false, 0, 0});
64+
heap.Insert(source, {0}, {false, {0}, {0}});
6565

6666
// explore search space
6767
while (!heap.Empty() && !destinations_set.empty())
@@ -216,12 +216,11 @@ class CellCustomizer
216216
partition.GetCell(level - 1, to)))
217217
{
218218
const EdgeWeight to_weight = weight + data.weight;
219-
const EdgeDuration to_duration = duration + data.duration;
219+
const EdgeDuration to_duration = duration + to_alias<EdgeDuration>(data.duration);
220220
const EdgeDistance to_distance = distance + data.distance;
221221
if (!heap.WasInserted(to))
222222
{
223-
heap.Insert(
224-
to, to_weight, {false, duration + data.duration, distance + data.distance});
223+
heap.Insert(to, to_weight, {false, to_duration, to_distance});
225224
}
226225
else if (std::tie(to_weight, to_duration, to_distance) <
227226
std::tie(

‎include/customizer/edge_based_graph.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ class MultiLevelGraph : public partitioner::MultiLevelGraph<EdgeDataT, Ownership
9797

9898
EdgeWeight GetNodeWeight(NodeID node) const { return node_weights[node]; }
9999

100-
EdgeWeight GetNodeDuration(NodeID node) const { return node_durations[node]; }
100+
EdgeDuration GetNodeDuration(NodeID node) const { return node_durations[node]; }
101101

102102
EdgeDistance GetNodeDistance(NodeID node) const { return node_distances[node]; }
103103

‎include/engine/api/table_api.hpp

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,8 @@ class TableAPI final : public BaseAPI
133133
}
134134

135135
bool have_speed_cells =
136-
parameters.fallback_speed != INVALID_FALLBACK_SPEED && parameters.fallback_speed > 0;
136+
parameters.fallback_speed != from_alias<double>(INVALID_FALLBACK_SPEED) &&
137+
parameters.fallback_speed > 0;
137138
flatbuffers::Offset<flatbuffers::Vector<uint32_t>> speed_cells;
138139
if (have_speed_cells)
139140
{
@@ -223,7 +224,8 @@ class TableAPI final : public BaseAPI
223224
MakeDistanceTable(tables.second, number_of_sources, number_of_destinations);
224225
}
225226

226-
if (parameters.fallback_speed != INVALID_FALLBACK_SPEED && parameters.fallback_speed > 0)
227+
if (parameters.fallback_speed != from_alias<double>(INVALID_FALLBACK_SPEED) &&
228+
parameters.fallback_speed > 0)
227229
{
228230
response.values["fallback_speed_cells"] = MakeEstimatesTable(fallback_speed_cells);
229231
}
@@ -272,17 +274,17 @@ class TableAPI final : public BaseAPI
272274

273275
virtual flatbuffers::Offset<flatbuffers::Vector<float>>
274276
MakeDurationTable(flatbuffers::FlatBufferBuilder &builder,
275-
const std::vector<EdgeWeight> &values) const
277+
const std::vector<EdgeDuration> &values) const
276278
{
277279
std::vector<float> distance_table;
278280
distance_table.resize(values.size());
279281
std::transform(
280-
values.begin(), values.end(), distance_table.begin(), [](const EdgeWeight duration) {
282+
values.begin(), values.end(), distance_table.begin(), [](const EdgeDuration duration) {
281283
if (duration == MAXIMAL_EDGE_DURATION)
282284
{
283285
return 0.;
284286
}
285-
return duration / 10.;
287+
return from_alias<double>(duration) / 10.;
286288
});
287289
return builder.CreateVector(distance_table);
288290
}
@@ -299,7 +301,7 @@ class TableAPI final : public BaseAPI
299301
{
300302
return 0.;
301303
}
302-
return std::round(distance * 10) / 10.;
304+
return std::round(from_alias<double>(distance) * 10) / 10.;
303305
});
304306
return builder.CreateVector(duration_table);
305307
}
@@ -347,7 +349,7 @@ class TableAPI final : public BaseAPI
347349
return json_waypoints;
348350
}
349351

350-
virtual util::json::Array MakeDurationTable(const std::vector<EdgeWeight> &values,
352+
virtual util::json::Array MakeDurationTable(const std::vector<EdgeDuration> &values,
351353
std::size_t number_of_rows,
352354
std::size_t number_of_columns) const
353355
{
@@ -361,13 +363,14 @@ class TableAPI final : public BaseAPI
361363
std::transform(row_begin_iterator,
362364
row_end_iterator,
363365
json_row.values.begin(),
364-
[](const EdgeWeight duration) {
366+
[](const EdgeDuration duration) {
365367
if (duration == MAXIMAL_EDGE_DURATION)
366368
{
367369
return util::json::Value(util::json::Null());
368370
}
369371
// division by 10 because the duration is in deciseconds (10s)
370-
return util::json::Value(util::json::Number(duration / 10.));
372+
return util::json::Value(
373+
util::json::Number(from_alias<double>(duration) / 10.));
371374
});
372375
json_table.values.push_back(std::move(json_row));
373376
}
@@ -394,8 +397,8 @@ class TableAPI final : public BaseAPI
394397
return util::json::Value(util::json::Null());
395398
}
396399
// round to single decimal place
397-
return util::json::Value(
398-
util::json::Number(std::round(distance * 10) / 10.));
400+
return util::json::Value(util::json::Number(
401+
std::round(from_alias<double>(distance) * 10) / 10.));
399402
});
400403
json_table.values.push_back(std::move(json_row));
401404
}

‎include/engine/api/table_parameters.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ struct TableParameters : public BaseParameters
5959
{
6060
std::vector<std::size_t> sources;
6161
std::vector<std::size_t> destinations;
62-
double fallback_speed = INVALID_FALLBACK_SPEED;
62+
double fallback_speed = from_alias<double>(INVALID_FALLBACK_SPEED);
6363

6464
enum class FallbackCoordinateType
6565
{

‎include/engine/datafacade/algorithm_datafacade.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ template <> class AlgorithmDataFacade<MLD>
8383

8484
virtual EdgeWeight GetNodeWeight(const NodeID edge_based_node_id) const = 0;
8585

86-
virtual EdgeWeight
86+
virtual EdgeDuration
8787
GetNodeDuration(const NodeID edge_based_node_id) const = 0; // TODO: to be removed
8888

8989
virtual EdgeDistance GetNodeDistance(const NodeID edge_based_node_id) const = 0;

‎include/engine/geospatial_query.hpp

Lines changed: 43 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -320,75 +320,84 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
320320

321321
const auto forward_weight_offset =
322322
// NOLINTNEXTLINE(bugprone-fold-init-type)
323-
std::accumulate(forward_weights.begin(),
324-
forward_weights.begin() + data.fwd_segment_position,
325-
EdgeWeight{0});
323+
alias_cast<EdgeWeight>(
324+
std::accumulate(forward_weights.begin(),
325+
forward_weights.begin() + data.fwd_segment_position,
326+
SegmentWeight{0}));
326327

327328
const auto forward_duration_offset =
328329
// NOLINTNEXTLINE(bugprone-fold-init-type)
329-
std::accumulate(forward_durations.begin(),
330-
forward_durations.begin() + data.fwd_segment_position,
331-
EdgeDuration{0});
330+
alias_cast<EdgeDuration>(
331+
std::accumulate(forward_durations.begin(),
332+
forward_durations.begin() + data.fwd_segment_position,
333+
SegmentDuration{0}));
332334

333-
EdgeDistance forward_distance_offset = 0;
335+
EdgeDistance forward_distance_offset = {0};
334336
// Sum up the distance from the start to the fwd_segment_position
335337
for (auto current = forward_geometry.begin();
336338
current < forward_geometry.begin() + data.fwd_segment_position;
337339
++current)
338340
{
339-
forward_distance_offset += util::coordinate_calculation::greatCircleDistance(
340-
datafacade.GetCoordinateOfNode(*current),
341-
datafacade.GetCoordinateOfNode(*std::next(current)));
341+
forward_distance_offset +=
342+
to_alias<EdgeDistance>(util::coordinate_calculation::greatCircleDistance(
343+
datafacade.GetCoordinateOfNode(*current),
344+
datafacade.GetCoordinateOfNode(*std::next(current))));
342345
}
343346

344347
BOOST_ASSERT(data.fwd_segment_position <
345348
std::distance(forward_durations.begin(), forward_durations.end()));
346349

347-
EdgeWeight forward_weight = forward_weights[data.fwd_segment_position];
348-
EdgeDuration forward_duration = forward_durations[data.fwd_segment_position];
349-
EdgeDistance forward_distance = util::coordinate_calculation::greatCircleDistance(
350-
datafacade.GetCoordinateOfNode(forward_geometry(data.fwd_segment_position)),
351-
point_on_segment);
350+
EdgeWeight forward_weight =
351+
alias_cast<EdgeWeight>(forward_weights[data.fwd_segment_position]);
352+
EdgeDuration forward_duration =
353+
alias_cast<EdgeDuration>(forward_durations[data.fwd_segment_position]);
354+
EdgeDistance forward_distance =
355+
to_alias<EdgeDistance>(util::coordinate_calculation::greatCircleDistance(
356+
datafacade.GetCoordinateOfNode(forward_geometry(data.fwd_segment_position)),
357+
point_on_segment));
352358

353-
const auto reverse_weight_offset =
359+
const auto reverse_weight_offset = alias_cast<EdgeWeight>(
354360
std::accumulate(reverse_weights.begin(),
355361
reverse_weights.end() - data.fwd_segment_position - 1,
356-
EdgeWeight{0});
362+
SegmentWeight{0}));
357363

358-
const auto reverse_duration_offset =
364+
const auto reverse_duration_offset = alias_cast<EdgeDuration>(
359365
std::accumulate(reverse_durations.begin(),
360366
reverse_durations.end() - data.fwd_segment_position - 1,
361-
EdgeDuration{0});
367+
SegmentDuration{0}));
362368

363-
EdgeDistance reverse_distance_offset = 0;
369+
EdgeDistance reverse_distance_offset = {0};
364370
// Sum up the distance from just after the fwd_segment_position to the end
365371
for (auto current = forward_geometry.begin() + data.fwd_segment_position + 1;
366372
current != std::prev(forward_geometry.end());
367373
++current)
368374
{
369-
reverse_distance_offset += util::coordinate_calculation::greatCircleDistance(
370-
datafacade.GetCoordinateOfNode(*current),
371-
datafacade.GetCoordinateOfNode(*std::next(current)));
375+
reverse_distance_offset +=
376+
to_alias<EdgeDistance>(util::coordinate_calculation::greatCircleDistance(
377+
datafacade.GetCoordinateOfNode(*current),
378+
datafacade.GetCoordinateOfNode(*std::next(current))));
372379
}
373380

374-
EdgeWeight reverse_weight =
375-
reverse_weights[reverse_weights.size() - data.fwd_segment_position - 1];
376-
EdgeDuration reverse_duration =
377-
reverse_durations[reverse_durations.size() - data.fwd_segment_position - 1];
378-
EdgeDistance reverse_distance = util::coordinate_calculation::greatCircleDistance(
379-
point_on_segment,
380-
datafacade.GetCoordinateOfNode(forward_geometry(data.fwd_segment_position + 1)));
381+
EdgeWeight reverse_weight = alias_cast<EdgeWeight>(
382+
reverse_weights[reverse_weights.size() - data.fwd_segment_position - 1]);
383+
EdgeDuration reverse_duration = alias_cast<EdgeDuration>(
384+
reverse_durations[reverse_durations.size() - data.fwd_segment_position - 1]);
385+
EdgeDistance reverse_distance =
386+
to_alias<EdgeDistance>(util::coordinate_calculation::greatCircleDistance(
387+
point_on_segment,
388+
datafacade.GetCoordinateOfNode(forward_geometry(data.fwd_segment_position + 1))));
381389

382390
ratio = std::min(1.0, std::max(0.0, ratio));
383391
if (data.forward_segment_id.id != SPECIAL_SEGMENTID)
384392
{
385-
forward_weight = static_cast<EdgeWeight>(forward_weight * ratio);
386-
forward_duration = static_cast<EdgeDuration>(forward_duration * ratio);
393+
forward_weight = to_alias<EdgeWeight>(from_alias<double>(forward_weight) * ratio);
394+
forward_duration = to_alias<EdgeDuration>(from_alias<double>(forward_duration) * ratio);
387395
}
388396
if (data.reverse_segment_id.id != SPECIAL_SEGMENTID)
389397
{
390-
reverse_weight -= static_cast<EdgeWeight>(reverse_weight * ratio);
391-
reverse_duration -= static_cast<EdgeDuration>(reverse_duration * ratio);
398+
reverse_weight -= to_alias<EdgeWeight>(from_alias<double>(reverse_weight) * ratio);
399+
reverse_duration -=
400+
to_alias<EdgeDuration>(from_alias<double>(reverse_duration) * ratio);
392401
}
393402

394403
// check phantom node segments validity

0 commit comments

Comments
 (0)