From 23521ca30921f8f2a6063fc9156e6b2e49104c3b Mon Sep 17 00:00:00 2001 From: Konstantin Pastbin Date: Sat, 16 Nov 2024 21:53:49 +0300 Subject: [PATCH] [tests][routing] Update bicycle integration tests Signed-off-by: Konstantin Pastbin --- .../bicycle_route_test.cpp | 167 ++++++++++-------- .../routing_test_tools.hpp | 8 +- 2 files changed, 95 insertions(+), 80 deletions(-) diff --git a/routing/routing_integration_tests/bicycle_route_test.cpp b/routing/routing_integration_tests/bicycle_route_test.cpp index 6a49ec1b1b..5552fdce8b 100644 --- a/routing/routing_integration_tests/bicycle_route_test.cpp +++ b/routing/routing_integration_tests/bicycle_route_test.cpp @@ -15,6 +15,8 @@ using namespace routing::turns; UNIT_TEST(RussiaMoscowSevTushinoParkPreferingBicycleWay) { + // Prefer a good quality dedicated cycleway over bad quality path + footway. + /// @todo: replace with a better case that prefers a longer cycleway to e.g. shorter track of same quality. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(55.87445, 37.43711), {0., 0.}, @@ -23,6 +25,7 @@ UNIT_TEST(RussiaMoscowSevTushinoParkPreferingBicycleWay) UNIT_TEST(RussiaMoscowNahimovskyLongRoute) { + // Get onto a secondary and follow it. Same as GraphHopper. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(55.66151, 37.63320), {0., 0.}, @@ -31,24 +34,26 @@ UNIT_TEST(RussiaMoscowNahimovskyLongRoute) UNIT_TEST(RussiaDomodedovoSteps) { + // Use the steps as the detour is way too long. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), - mercator::FromLatLon(55.44010, 37.77416), {0., 0.}, - mercator::FromLatLon(55.43975, 37.77272), 100.0); + mercator::FromLatLon(55.4403114, 37.7740223), {0., 0.}, + mercator::FromLatLon(55.439703, 37.7725059), 139.058); } UNIT_TEST(SwedenStockholmCyclewayPriority) { + /// @todo(pastk): DELETE: the cycleway is the shortest and the only obvious option here anyway, what's the value of this test? CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(59.33151, 18.09347), {0., 0.}, mercator::FromLatLon(59.33052, 18.09391), 113.0); } -// Note. If the closest to start or finish road has "bicycle=no" tag the closest road where -// it's allowed to ride bicycle will be found. UNIT_TEST(NetherlandsAmsterdamBicycleNo) { + // Snap start/finish point to the closest suitable road. + // The closest road here has a bicycle=no tag so its ignored and the next closest cycleway is used. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(52.32716, 5.05932), {0., 0.}, @@ -57,18 +62,20 @@ UNIT_TEST(NetherlandsAmsterdamBicycleNo) UNIT_TEST(NetherlandsAmsterdamBicycleYes) { + // Test that a highway=unclassified gets a significant boost due to presence of bicycle=yes tag. + /// @todo(pastk): it shouldn't as there is no cycling infra (cycleway:both=no https://www.openstreetmap.org/way/214196820) + /// and bicycle=yes means "its legal", not "its fast", see https://github.com/organicmaps/organicmaps/issues/9593 TRouteResult const routeResult = CalculateRoute(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(52.32872, 5.07527), {0.0, 0.0}, mercator::FromLatLon(52.33853, 5.08941)); TEST_EQUAL(routeResult.second, RouterResultCode::NoError, ()); - TestRouteTime(*routeResult.first, 296.487); + TestRouteTime(*routeResult.first, 284.38); } -// This test on tag cycleway=opposite for a streets which have oneway=yes. -// It means bicycles may go in the both directions. -UNIT_TEST(NetherlandsAmsterdamSingelStCyclewayOpposite) +UNIT_TEST(Netherlands_Amsterdam_OnewaySt_CyclewayOpposite) { + // Bicycles can go against the car traffic on oneway=yes roads if there is a cycleway=opposite tag. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(52.37571, 4.88591), {0., 0.}, @@ -78,34 +85,16 @@ UNIT_TEST(NetherlandsAmsterdamSingelStCyclewayOpposite) UNIT_TEST(RussiaMoscowKashirskoe16ToCapLongRoute) { // There is no dedicated bicycle infra, except short end part. All OSM routers give different results, - // but OM and Valhalla looks better. + // OM yields a short and logical route shortcutting via some service roads and footways (allowed in Russia). CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(55.66230, 37.63214), {0., 0.}, - mercator::FromLatLon(55.68927, 37.70356), 7323.01); + mercator::FromLatLon(55.68927, 37.70356), 6968.64); } -// Passing through living_street and service are allowed in Russia -UNIT_TEST(RussiaMoscowServicePassThrough1) -{ - TRouteResult route = - integration::CalculateRoute(integration::GetVehicleComponents(VehicleType::Bicycle), - mercator::FromLatLon(55.66230, 37.63214), {0., 0.}, - mercator::FromLatLon(55.68895, 37.70286)); - TEST_EQUAL(route.second, RouterResultCode::NoError, ()); -} - -UNIT_TEST(RussiaMoscowServicePassThrough2) -{ - TRouteResult route = - integration::CalculateRoute(integration::GetVehicleComponents(VehicleType::Bicycle), - mercator::FromLatLon(55.69038, 37.70015), {0., 0.}, - mercator::FromLatLon(55.69123, 37.6953)); - TEST_EQUAL(route.second, RouterResultCode::NoError, ()); -} - -UNIT_TEST(RussiaMoscowServicePassThrough3) +UNIT_TEST(RussiaMoscowServicePassThrough) { + // Passing through living_street and service is allowed in Russia. TRouteResult route = integration::CalculateRoute(integration::GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(55.79649, 37.53738), {0., 0.}, @@ -115,62 +104,72 @@ UNIT_TEST(RussiaMoscowServicePassThrough3) UNIT_TEST(RussiaKerchStraitFerryRoute) { + // Use a cross-mwm ferry. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(45.4167, 36.7658), {0.0, 0.0}, mercator::FromLatLon(45.3653, 36.6161), 17151.4); } -// Test on building bicycle route past ferry. UNIT_TEST(SwedenStockholmBicyclePastFerry) { + // Several consecutive ferries. CalculateRouteAndTestRouteLength( GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(59.4725, 18.51355), {0.0, 0.0}, mercator::FromLatLon(59.42533, 18.35991), 14338.0); } -// Test on cross mwm bicycle routing. UNIT_TEST(CrossMwmKaliningradRegionToLiepaja) { + // A cross mwm route (3 regions). Includes a ferry. integration::CalculateRouteAndTestRouteLength( integration::GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(54.63519, 21.80749), {0., 0.}, - mercator::FromLatLon(56.51119, 21.01847), 271237); + mercator::FromLatLon(56.51119, 21.01847), 266992); +} +UNIT_TEST(Lithuania_Avoid_Ferry_Long_Route) +{ // Avoid ferry Dreverna-Juodkrantė-Klaipėda. RoutingOptionSetter optionsGuard(RoutingOptions::Ferry); - // Same as GraphHopper makes a detour (via unpaved), OSRM goes straight with highway=primary. + + // GraphHopper makes a detour (via unpaved), OSRM goes straight with highway=primary, + // Valhalla uses a part of the primary. A user says that GraphHopper is the best option. // https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=55.340%2C21.459%3B55.715%2C21.135 - // User says that GraphHopper is the best option. + // OM uses a much shorter (56km vs 64km) route with bicycle=yes tracks and a short path section. + /// @todo(pastk): the route goes through a landuse=military briefly and OM doesn't account for that. + /// And too much preference is given to bicycle=yes track, see https://github.com/organicmaps/organicmaps/issues/9593 integration::CalculateRouteAndTestRouteLength( integration::GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(55.3405073, 21.4595925), {0., 0.}, - mercator::FromLatLon(55.7140174, 21.1365445), 64134.8); + mercator::FromLatLon(55.7140174, 21.1365445), 56243.2); } -// Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters). UNIT_TEST(SpainTenerifeAdejeVilaflor) { + // Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters). + // A long ETA due to going uphill. TRouteResult const res = CalculateRoute(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(28.11984, -16.72592), {0., 0.}, mercator::FromLatLon(28.15865, -16.63704)); TEST_EQUAL(res.second, RouterResultCode::NoError, ()); TestRouteLength(*res.first, 26401); - TestRouteTime(*res.first, 11002); + TestRouteTime(*res.first, 10716); } -// Test on riding down from Vilaflor (altitude 1400 meters) to Adeje (sea level). UNIT_TEST(SpainTenerifeVilaflorAdeje) { + // Test on riding down from Vilaflor (altitude 1400 meters) to Adeje (sea level). + // A short ETA going downhill. TRouteResult const res = CalculateRoute(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(28.15865, -16.63704), {0., 0.}, mercator::FromLatLon(28.11984, -16.72592)); TEST_EQUAL(res.second, RouterResultCode::NoError, ()); - TestRouteLength(*res.first, 25601.6); - TestRouteTime(*res.first, 4756); + TestRouteLength(*res.first, 24582); + TestRouteTime(*res.first, 4459); } // Two tests on not building route against traffic on road with oneway:bicycle=yes. @@ -180,7 +179,7 @@ UNIT_TEST(Munich_OnewayBicycle1) integration::CalculateRouteAndTestRouteLength( integration::GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(48.1601673, 11.5630245), {0.0, 0.0}, - mercator::FromLatLon(48.1606349, 11.5631699), 279.536 /* expectedRouteMeters */); + mercator::FromLatLon(48.1606349, 11.5631699), 264.042 /* expectedRouteMeters */); } UNIT_TEST(Munich_OnewayBicycle2) @@ -190,19 +189,20 @@ UNIT_TEST(Munich_OnewayBicycle2) mercator::FromLatLon(48.17867, 11.57303), 201.532 /* expectedRouteMeters */); } -// https://github.com/organicmaps/organicmaps/issues/1603 UNIT_TEST(London_GreenwichTunnel) { - // Has bicycle=yes, because it belongs to https://www.openstreetmap.org/relation/9063263 + // Use the bicycle=dismount foot tunnel https://www.openstreetmap.org/way/4358990 + // as a detour is way too long. + // https://github.com/organicmaps/organicmaps/issues/8028 - /// @todo +50m detour is not suitable here, IMO. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(51.4817397, -0.0100070258), {0.0, 0.0}, - mercator::FromLatLon(51.4883739, -0.00809729298), 1305.81 /* expectedRouteMeters */); + mercator::FromLatLon(51.4883739, -0.00809729298), 1183.12 /* expectedRouteMeters */); } UNIT_TEST(Batumi_AvoidServiceDetour) { + // Go straight via a residential without a short detour via service road. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(41.6380014, 41.6269446), {0.0, 0.0}, mercator::FromLatLon(41.6392113, 41.6260084), 160.157 /* expectedRouteMeters */); @@ -210,14 +210,16 @@ UNIT_TEST(Batumi_AvoidServiceDetour) UNIT_TEST(Gdansk_AvoidLongCyclewayDetour) { + /// @todo(pastk): DELETE: the preferred route goes by a shared cycleway also - replace with a better case (the next one!) CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(54.2632738, 18.6771661), {0.0, 0.0}, - mercator::FromLatLon(54.2698882, 18.6765837), 775.81 /* expectedRouteMeters */); + mercator::FromLatLon(54.2698882, 18.6765837), 760.749 /* expectedRouteMeters */); } -// https://github.com/organicmaps/organicmaps/issues/4145 UNIT_TEST(Belarus_StraightFootway) { + // Prefer footways over roads in Belarus due to local laws. + // https://github.com/organicmaps/organicmaps/issues/4145 CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(53.8670285, 30.3162749), {0.0, 0.0}, mercator::FromLatLon(53.876436, 30.3348084), 1613.34 /* expectedRouteMeters */); @@ -225,6 +227,8 @@ UNIT_TEST(Belarus_StraightFootway) UNIT_TEST(Spain_Madrid_ElevationsDetour) { + /// @todo(pastk): DELETE: there are no detours here atm, the second route is just a tad longer + /// and is of the same alt difference, so this test is currently useless TRouteResult const r1 = CalculateRoute(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(40.459616, -3.690031), {0., 0.}, mercator::FromLatLon(40.4408171, -3.69261893)); @@ -243,74 +247,83 @@ UNIT_TEST(Spain_Madrid_ElevationsDetour) UNIT_TEST(Spain_Zaragoza_Fancy_NoBicycle_Crossings) { + // A highway=crossing node https://www.openstreetmap.org/node/258776322 on the way + // has bicycle=no, which doesn't prevent routing along this road. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(41.6523561, -0.881151311), {0.0, 0.0}, mercator::FromLatLon(41.6476614, -0.885694674), 649.855 /* expectedRouteMeters */); } -// https://github.com/organicmaps/organicmaps/issues/1201#issuecomment-946042937 -UNIT_TEST(Netherlands_Use_Bicycle_Track) +UNIT_TEST(Germany_Use_Bicycle_Track) { + // Avoid primary and prefer smaller roads and tracks with bicycle=yes. + /// @todo Still prefers a no-cycling-infra but bicycle=yes secondary rather than a paved track, + /// see https://github.com/organicmaps/organicmaps/issues/1201#issuecomment-946042937 CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(48.420723, 9.90350146), {0.0, 0.0}, mercator::FromLatLon(48.4080367, 9.86597073), 3778.41 /* expectedRouteMeters */); } -// https://github.com/orgs/organicmaps/discussions/5158#discussioncomment-5938807 UNIT_TEST(Finland_Use_Tertiary_LowTraffic) { + // Detour via a tertiary to avoid a secondary. + // https://github.com/orgs/organicmaps/discussions/5158#discussioncomment-5938807 + /// @todo(pastk): prefer roads with lower maxspeed. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(61.5445696, 23.9394003), {0.0, 0.0}, mercator::FromLatLon(61.6153965, 23.876755), 9876.65 /* expectedRouteMeters */); } -UNIT_TEST(Stolbcy_Use_Unpaved) +UNIT_TEST(Belarus_Stolbcy_Use_Unpaved) { - // Same as GraphHopper. Valhalla uses ground path through ford. - // OSRM makes completely strange route. + // Goes by a track, unpaved and paved streets and an unpaved_bad track in the end. + // Closer as GraphHopper. Valhalla detours the unpaved street. + // OSRM shortcuts through paths and a ford. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(53.471389, 26.7422186), {0.0, 0.0}, - mercator::FromLatLon(53.454082, 26.7560061), 5148.45 /* expectedRouteMeters */); + mercator::FromLatLon(53.454082, 26.7560061), 4620.81 /* expectedRouteMeters */); } UNIT_TEST(Russia_UseTrunk) { - // Similar with GraphHopper. + // Prefer riding via a straight trunk road instead of taking a long +30% detour via smaller roads. + /// @todo(pastk): DELETE: This test is controversial as this route has sections with longer relative trunk-avoiding detours + /// e.g. from 67.9651692,32.8685132 to 68.022093,32.9654391 its +40% longer via a teriary and its OK? CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(66.271, 33.048), {0.0, 0.0}, - mercator::FromLatLon(68.95, 33.045), 412795 /* expectedRouteMeters */); + mercator::FromLatLon(68.95, 33.045), 404262 /* expectedRouteMeters */); } -// https://github.com/organicmaps/organicmaps/issues/3920 -UNIT_TEST(IgnoreCycleBarrier_WithoutAccess) +UNIT_TEST(Netherlands_IgnoreCycleBarrier_WithoutAccess) { + // There is a barrier=cycle_barrier in the beginning of the route + // and it doesn't affect routing as there is no explicit bicycle=no. + // https://github.com/organicmaps/organicmaps/issues/3920 + /// @todo(pastk): such barrier should have a small penalty in routing + /// as it slows down a cyclist. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(51.9960994, 5.67350176), {0.0, 0.0}, mercator::FromLatLon(51.9996861, 5.67299133), 428.801); - - // The difference here (end points are almost equal) because of some barrier=gate without access. - // https://www.openstreetmap.org/node/6993853766 - - /// @todo Make correct tunnels elevation (linear between start and finish). - CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), - mercator::FromLatLon(51.3822, -2.3886), {0.0, 0.0}, - mercator::FromLatLon(51.3574666, -2.31526436), 8583.27); - - CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), - mercator::FromLatLon(51.3822, -2.3886), {0.0, 0.0}, - mercator::FromLatLon(51.3576973, -2.31416085), 11131.6); } -UNIT_TEST(UK_Canterbury_UseDismount) +UNIT_TEST(UK_ForbidGates_WithoutAccess) { - CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Pedestrian), - mercator::FromLatLon(51.2794435, 1.05627788), {0.0, 0.0}, - mercator::FromLatLon(51.2818863, 1.05725286), 305); + // A barrier=gate without explicit access leads to a long detour. + // https://www.openstreetmap.org/node/6993853766 + /// @todo OSRM/Valhalla/GraphHopper ignore such gates, + /// see https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=51.3579329%2C-2.3137701%3B51.3574666%2C-2.3152644 + CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), + mercator::FromLatLon(51.3579329, -2.3137701), {0.0, 0.0}, + mercator::FromLatLon(51.3574666, -2.31526436), 1149.28); +} - /// @todo We have 231 sec bicycle detour here, while using dismount footway is 228 sec by foot :) +UNIT_TEST(UK_Canterbury_AvoidDismount) +{ + // A shortcut via a footway is 305 meters, ETAs are similar, but cyclists prefer to ride! + // Check the London_GreenwichTunnel case for when dismounting is reasonable as the detour is way too long. CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(51.2794435, 1.05627788), {0.0, 0.0}, - mercator::FromLatLon(51.2818863, 1.05725286), 305); + mercator::FromLatLon(51.2818863, 1.05725286), 976); } } // namespace bicycle_route_test diff --git a/routing/routing_integration_tests/routing_test_tools.hpp b/routing/routing_integration_tests/routing_test_tools.hpp index dcb3b61851..33eec788d0 100644 --- a/routing/routing_integration_tests/routing_test_tools.hpp +++ b/routing/routing_integration_tests/routing_test_tools.hpp @@ -26,18 +26,20 @@ * and Platform::ResourcesDir() only once and then reuse it. * Use GetCarComponents() or GetPedestrianComponents() with vector of maps parameter * only if you want to test something on a special map set. - * 2. Loading maps and calculating routes is a time consumption process. + * 2. Loading maps and calculating routes is a time consuming process. * Do this only if you really need it. * 3. If you want to check that a turn is absent - use TestTurnCount. * 4. The easiest way to gather all the information for writing an integration test is * - to put a break point in CalculateRoute() method; - * - to make a route with MapWithMe desktop application; + * - to make a route with the desktop application; * - to get all necessary parameters and result of the route calculation; * - to place them into the test you're writing. * 5. The recommended way for naming tests for a route from one place to another one is * - * 6. It's a good idea to use short routes for testing turns. The thing is geometry of long routes + * 6. Add a comment to describe what this particular test is testing (e.g. "respect the bicycle=no tag"). + * And add a ref to a Github issue if applicable. + * 7. It's a good idea to use short routes for testing turns. The thing is geometry of long routes * could be changed from one dataset to another. The shorter the route the less is the chance it's changed. */