diff --git a/include/engine/guidance/assemble_geometry.hpp b/include/engine/guidance/assemble_geometry.hpp index fb0ea7d760..981daa37ca 100644 --- a/include/engine/guidance/assemble_geometry.hpp +++ b/include/engine/guidance/assemble_geometry.hpp @@ -67,7 +67,7 @@ inline LegGeometry assembleGeometry(const datafacade::BaseDataFacade &facade, auto prev_coordinate = geometry.locations.front(); for (const auto &path_point : leg_data) { - auto coordinate = facade.GetCoordinateOfNode(path_point.turn_via_node); + auto coordinate = coordinateOf(facade, path_point); current_distance = util::coordinate_calculation::greatCircleDistance(prev_coordinate, coordinate); cumulative_distance += current_distance; diff --git a/include/engine/guidance/assemble_leg.hpp b/include/engine/guidance/assemble_leg.hpp index 9af1903f7c..2a71eb43cd 100644 --- a/include/engine/guidance/assemble_leg.hpp +++ b/include/engine/guidance/assemble_leg.hpp @@ -172,7 +172,7 @@ inline RouteLeg assembleLeg(const datafacade::BaseDataFacade &facade, auto prev_coordinate = source_node.location; for (const auto &path_point : route_data) { - auto coordinate = facade.GetCoordinateOfNode(path_point.turn_via_node); + auto coordinate = coordinateOf(facade, path_point); distance += util::coordinate_calculation::greatCircleDistance(prev_coordinate, coordinate); prev_coordinate = coordinate; } diff --git a/include/engine/guidance/assemble_steps.hpp b/include/engine/guidance/assemble_steps.hpp index 53030e9b30..3009fb6f28 100644 --- a/include/engine/guidance/assemble_steps.hpp +++ b/include/engine/guidance/assemble_steps.hpp @@ -193,7 +193,7 @@ inline std::vector assembleSteps(const datafacade::BaseDataFacade &fa intersection.in = bearing_class.findMatchingBearing(bearings.first); intersection.out = bearing_class.findMatchingBearing(bearings.second); - intersection.location = facade.GetCoordinateOfNode(path_point.turn_via_node); + intersection.location = coordinateOf(facade, path_point); intersection.bearings.clear(); intersection.bearings.reserve(bearing_data.size()); intersection.lanes = lane_data.first; diff --git a/include/engine/internal_route_result.hpp b/include/engine/internal_route_result.hpp index c95050e8a3..7c0e347fed 100644 --- a/include/engine/internal_route_result.hpp +++ b/include/engine/internal_route_result.hpp @@ -43,8 +43,34 @@ struct PathData DatasourceID datasource_id; // If segment precedes a turn, ID of the turn itself std::optional turn_edge; + // Where this point actually is, when that is not where turn_via_node is. + // + // Every point of a route normally lands on a node of the graph, and assembly finds + // its position by looking the node up. A point that is computed rather than + // traversed has nowhere to be looked up, and this is where it says where it is. + // Invalid by default, which means the node lookup, so a producer that does not set + // it is unaffected. + // + // The node id is left alone. A producer that sets this decides separately which + // node the point is attributed to, because that is what annotations report and it is + // not a question this field can answer. + util::Coordinate coordinate{}; }; +/** + * @brief Where a point of a route is. + * + * The one place that resolves PathData::coordinate against the node lookup. Distance, + * geometry and the steps have to agree about where a point is, and they agree by all + * asking here. + */ +template +inline util::Coordinate coordinateOf(const FacadeT &facade, const PathData &point) +{ + return point.coordinate.IsValid() ? point.coordinate + : facade.GetCoordinateOfNode(point.turn_via_node); +} + struct InternalRouteResult { std::vector> unpacked_path_segments; diff --git a/include/engine/routing_algorithms/routing_base.hpp b/include/engine/routing_algorithms/routing_base.hpp index ccd011584c..a3f551d679 100644 --- a/include/engine/routing_algorithms/routing_base.hpp +++ b/include/engine/routing_algorithms/routing_base.hpp @@ -357,7 +357,7 @@ double getPathDistance(const DataFacade &facade, for (const auto &p : unpacked_path) { - const auto current_coordinate = facade.GetCoordinateOfNode(p.turn_via_node); + const auto current_coordinate = coordinateOf(facade, p); distance += util::coordinate_calculation::greatCircleDistance(prev_coordinate, current_coordinate); diff --git a/unit_tests/engine/path_data_coordinate.cpp b/unit_tests/engine/path_data_coordinate.cpp new file mode 100644 index 0000000000..ad5515e83e --- /dev/null +++ b/unit_tests/engine/path_data_coordinate.cpp @@ -0,0 +1,141 @@ +#include "engine/internal_route_result.hpp" + +#include "engine/guidance/assemble_geometry.hpp" +#include "engine/guidance/assemble_leg.hpp" + +#include "mocks/mock_datafacade.hpp" + +#include + +#include + +BOOST_AUTO_TEST_SUITE(path_data_coordinate) + +using namespace osrm; +using namespace osrm::engine; +using namespace osrm::util; + +namespace +{ + +// The mock puts every node at the origin, which is what makes this testable: a location +// away from the origin can only have come from the field. +const Coordinate ORIGIN{FixedLongitude{0}, FixedLatitude{0}}; +const Coordinate ELSEWHERE{FixedLongitude{1000}, FixedLatitude{2000}}; + +/** + * The shared mock hands back an empty datasource range, which assembly indexes into. It + * is enough for everything else in here, so this adds the one thing that is missing + * rather than growing the mock for one caller. + */ +struct Facade final : test::MockBaseDataFacade +{ + using Base = test::MockBaseDataFacade; + + Base::DatasourceForwardRange GetUncompressedForwardDatasources(const EdgeID) const override + { + static const DatasourceID datasources[] = {0, 0, 0, 0}; + return {datasources, 4}; + } +}; + +PathData at_node(const NodeID node) +{ + PathData point{}; + point.turn_via_node = node; + return point; +} + +PhantomNode endpoint(const Coordinate location) +{ + PhantomNode node; + node.location = location; + node.forward_segment_id = {0, true}; + node.reverse_segment_id = {0, true}; + node.forward_weight = {0}; + node.reverse_weight = {0}; + node.forward_duration = {0}; + node.reverse_duration = {0}; + node.fwd_segment_position = 0; + return node; +} + +} // namespace + +// Nothing sets the field, so nothing changes: the position still comes from the node. +BOOST_AUTO_TEST_CASE(a_point_without_a_coordinate_is_placed_by_its_node) +{ + const Facade facade; + const std::vector leg{at_node(1), at_node(2)}; + + BOOST_CHECK(!leg.front().coordinate.IsValid()); + BOOST_CHECK_EQUAL(coordinateOf(facade, leg.front()).lon, ORIGIN.lon); + BOOST_CHECK_EQUAL(coordinateOf(facade, leg.front()).lat, ORIGIN.lat); +} + +// And when it is set, it wins. +BOOST_AUTO_TEST_CASE(a_coordinate_overrides_the_node_lookup) +{ + const Facade facade; + auto point = at_node(1); + point.coordinate = ELSEWHERE; + + BOOST_CHECK_EQUAL(coordinateOf(facade, point).lon, ELSEWHERE.lon); + BOOST_CHECK_EQUAL(coordinateOf(facade, point).lat, ELSEWHERE.lat); +} + +// The drawn line follows the field. +// +// This is the property the field exists for. A point that is computed rather than +// traversed has no node to be looked up, and without this it would be drawn at whatever +// the lookup happened to return. +BOOST_AUTO_TEST_CASE(the_geometry_follows_the_coordinate) +{ + const Facade facade; + const auto source = endpoint(ORIGIN); + const auto target = endpoint(ORIGIN); + + std::vector leg{at_node(1)}; + const auto plain = + engine::guidance::assembleGeometry(facade, leg, source, target, false, false); + + leg.front().coordinate = ELSEWHERE; + const auto moved = + engine::guidance::assembleGeometry(facade, leg, source, target, false, false); + + // Same shape, one point in a different place. + BOOST_REQUIRE_EQUAL(plain.locations.size(), moved.locations.size()); + BOOST_CHECK_EQUAL(plain.locations[1].lon, ORIGIN.lon); + BOOST_CHECK_EQUAL(moved.locations[1].lon, ELSEWHERE.lon); + BOOST_CHECK_EQUAL(moved.locations[1].lat, ELSEWHERE.lat); + + // And the node id is untouched, because the field does not claim to answer that. + BOOST_CHECK(plain.node_ids == moved.node_ids); +} + +// The reported distance follows it too, which is the whole reason the field lives on +// PathData rather than being applied to the geometry after assembly. A line that is +// drawn one way and measured another is worse than either. +BOOST_AUTO_TEST_CASE(the_distance_follows_the_coordinate) +{ + const Facade facade; + const auto source = endpoint(ORIGIN); + const auto target = endpoint(ORIGIN); + + std::vector leg{at_node(1)}; + const auto plain = engine::guidance::assembleLeg(facade, leg, source, target, false); + + leg.front().coordinate = ELSEWHERE; + const auto moved = engine::guidance::assembleLeg(facade, leg, source, target, false); + + // Everything is at the origin in the first case, so the leg has no length at all. + BOOST_CHECK_SMALL(plain.distance, 1e-9); + BOOST_CHECK_GT(moved.distance, 0.0); + + // Out to the moved point and back, so the geometry above and this agree. The leg + // reports its distance rounded to a tenth of a metre, which is the tolerance here. + const auto out = coordinate_calculation::greatCircleDistance(ORIGIN, ELSEWHERE); + BOOST_CHECK_SMALL(moved.distance - 2.0 * out, 0.1); +} + +BOOST_AUTO_TEST_SUITE_END()