From c98bde1268e0c2fef6eacdc60b6e1a35842b4b1b Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Tue, 13 Dec 2016 08:57:20 +0300 Subject: [PATCH] Review fixes. --- routing/bicycle_directions.cpp | 20 +++++++++++++------ routing/bicycle_directions.hpp | 2 +- routing/car_router.cpp | 14 ++++++++++---- routing/directions_engine.hpp | 2 +- routing/loaded_path_segment.hpp | 4 ++-- routing/pedestrian_directions.cpp | 2 +- routing/pedestrian_directions.hpp | 2 +- routing/road_graph_router.cpp | 3 ++- routing/route.cpp | 14 ++++++-------- routing/routing_helpers.cpp | 32 +++++++++++++++++-------------- routing/routing_helpers.hpp | 2 +- routing/single_mwm_router.cpp | 2 ++ routing/turns_generator.cpp | 8 ++++---- routing/turns_generator.hpp | 3 +-- 14 files changed, 64 insertions(+), 46 deletions(-) diff --git a/routing/bicycle_directions.cpp b/routing/bicycle_directions.cpp index 00f7d47471..37cca4247b 100644 --- a/routing/bicycle_directions.cpp +++ b/routing/bicycle_directions.cpp @@ -87,7 +87,7 @@ BicycleDirectionsEngine::BicycleDirectionsEngine(Index const & index) : m_index( void BicycleDirectionsEngine::Generate(IRoadGraph const & graph, vector const & path, Route::TTimes & times, Route::TTurns & turns, vector & routeGeometry, - vector & routeSegs, + vector & trafficSegs, my::Cancellable const & cancellable) { times.clear(); @@ -95,7 +95,7 @@ void BicycleDirectionsEngine::Generate(IRoadGraph const & graph, vector(routeEdges[i - 1].GetSegId()), routeEdges[i - 1].IsForward() ? TrafficInfo::RoadSegmentId::kForwardDirection @@ -186,12 +189,17 @@ void BicycleDirectionsEngine::Generate(IRoadGraph const & graph, vector const & path, Route::TTimes & times, Route::TTurns & turns, vector & routeGeometry, - vector & routeSegs, + vector & trafficSegs, my::Cancellable const & cancellable) override; private: diff --git a/routing/car_router.cpp b/routing/car_router.cpp index 432a2471c9..0e8c013a70 100644 --- a/routing/car_router.cpp +++ b/routing/car_router.cpp @@ -207,7 +207,7 @@ IRouter::ResultCode FindSingleOsrmRoute(FeatureGraphNode const & source, Route::TTurns turns; Route::TTimes times; Route::TStreets streets; - vector routeSegs; + vector trafficSegs; LOG(LINFO, ("OSRM route from", MercatorBounds::ToLatLon(source.segmentPoint), "to", MercatorBounds::ToLatLon(target.segmentPoint))); @@ -218,7 +218,7 @@ IRouter::ResultCode FindSingleOsrmRoute(FeatureGraphNode const & source, OSRMRoutingResult routingResult(index, *mapping, rawRoutingResult); routing::IRouter::ResultCode const result = - MakeTurnAnnotation(routingResult, delegate, geometry, turns, times, streets, routeSegs); + MakeTurnAnnotation(routingResult, delegate, geometry, turns, times, streets, trafficSegs); if (result != routing::IRouter::NoError) { LOG(LWARNING, ("Can't load road path data from disk for", mapping->GetCountryName(), @@ -276,8 +276,14 @@ bool CarRouter::FindRouteMSMT(TFeatureGraphNodeVec const & sources, { for (auto const & sourceEdge : sources) { - if (FindSingleRouteDispatcher(sourceEdge, targetEdge, delegate, mapping, route) == NoError) - return true; + IRouter::ResultCode const code = + FindSingleRouteDispatcher(sourceEdge, targetEdge, delegate, mapping, route); + switch (code) + { + case NoError: return true; + case Cancelled: return false; + default: continue; + } } } return false; diff --git a/routing/directions_engine.hpp b/routing/directions_engine.hpp index 8fc71d79d9..e2c59a22bd 100644 --- a/routing/directions_engine.hpp +++ b/routing/directions_engine.hpp @@ -20,7 +20,7 @@ public: virtual void Generate(IRoadGraph const & graph, vector const & path, Route::TTimes & times, Route::TTurns & turns, vector & routeGeometry, - vector & routeSegs, + vector & trafficSegs, my::Cancellable const & cancellable) = 0; protected: diff --git a/routing/loaded_path_segment.hpp b/routing/loaded_path_segment.hpp index 729ac7b012..72fd2f61eb 100644 --- a/routing/loaded_path_segment.hpp +++ b/routing/loaded_path_segment.hpp @@ -34,7 +34,7 @@ struct LoadedPathSegment string m_name; TEdgeWeight m_weight; /*!< Time in seconds to pass the segment. */ TNodeId m_nodeId; /*!< May be NodeId for OSRM router or FeatureId::index for graph router. */ - vector m_routeSegs; /*!< Route segments for |m_path|. */ + vector m_trafficSegs; /*!< Traffic segments for |m_path|. */ ftypes::HighwayClass m_highwayClass; bool m_onRoundabout; bool m_isLink; @@ -51,7 +51,7 @@ struct LoadedPathSegment m_name.clear(); m_weight = 0; m_nodeId = 0; - m_routeSegs.clear(); + m_trafficSegs.clear(); m_highwayClass = ftypes::HighwayClass::Undefined; m_onRoundabout = false; m_isLink = false; diff --git a/routing/pedestrian_directions.cpp b/routing/pedestrian_directions.cpp index 8ac564a274..4b9273348e 100644 --- a/routing/pedestrian_directions.cpp +++ b/routing/pedestrian_directions.cpp @@ -35,7 +35,7 @@ PedestrianDirectionsEngine::PedestrianDirectionsEngine() void PedestrianDirectionsEngine::Generate(IRoadGraph const & graph, vector const & path, Route::TTimes & times, Route::TTurns & turns, vector & routeGeometry, - vector &, + vector & /* trafficSegs */, my::Cancellable const & cancellable) { times.clear(); diff --git a/routing/pedestrian_directions.hpp b/routing/pedestrian_directions.hpp index f200bf4724..555dc50330 100644 --- a/routing/pedestrian_directions.hpp +++ b/routing/pedestrian_directions.hpp @@ -13,7 +13,7 @@ public: // IDirectionsEngine override: void Generate(IRoadGraph const & graph, vector const & path, Route::TTimes & times, Route::TTurns & turns, vector & routeGeometry, - vector &, + vector & /* trafficSegs */, my::Cancellable const & cancellable) override; private: diff --git a/routing/road_graph_router.cpp b/routing/road_graph_router.cpp index 127078329e..36a1433884 100644 --- a/routing/road_graph_router.cpp +++ b/routing/road_graph_router.cpp @@ -227,7 +227,8 @@ IRouter::ResultCode RoadGraphRouter::CalculateRoute(m2::PointD const & startPoin ASSERT_EQUAL(result.path.front(), startPos, ()); ASSERT_EQUAL(result.path.back(), finalPos, ()); ASSERT_GREATER(result.distance, 0., ()); - ReconstructRoute(m_directionsEngine.get(), *m_roadGraph, nullptr, delegate, result.path, route); + ReconstructRoute(m_directionsEngine.get(), *m_roadGraph, nullptr /* trafficColoring */, + delegate, result.path, route); } m_roadGraph->ResetFakes(); diff --git a/routing/route.cpp b/routing/route.cpp index d8b3ff84de..b99d49be6d 100644 --- a/routing/route.cpp +++ b/routing/route.cpp @@ -15,10 +15,10 @@ #include "std/numeric.hpp" -namespace routing -{ using namespace traffic; +namespace routing +{ namespace { double constexpr kLocationTimeThreshold = 60.0 * 1.0; @@ -368,17 +368,14 @@ void Route::AppendTraffic(Route const & route) if (!IsValid()) { - m_traffic.assign(route.GetTraffic().cbegin(), route.GetTraffic().cend()); + m_traffic = route.GetTraffic(); return; } // Note. At this point the last item of |m_poly| should be removed. - // So the size of |m_traffic| should be equal to size of |m_poly. + // So the size of |m_traffic| should be equal to size of |m_poly|. if (GetTraffic().empty()) - { - CHECK_GREATER_OR_EQUAL(m_poly.GetPolyline().GetSize(), 1, ()); m_traffic.resize(m_poly.GetPolyline().GetSize(), SpeedGroup::Unknown); - } CHECK_EQUAL(GetTraffic().size(), m_poly.GetPolyline().GetSize(), ()); @@ -386,13 +383,14 @@ void Route::AppendTraffic(Route const & route) { CHECK_GREATER_OR_EQUAL(route.m_poly.GetPolyline().GetSize(), 1, ()); m_traffic.insert(m_traffic.end(), - route.m_poly.GetPolyline().GetSize() - 1 /* segment number is less by one */, + route.m_poly.GetPolyline().GetSize() - 1 /* number of segments is less by one */, SpeedGroup::Unknown); } else { m_traffic.insert(m_traffic.end(), route.GetTraffic().cbegin(), route.GetTraffic().cend()); } + CHECK_EQUAL(GetTraffic().size() + 1, m_poly.GetPolyline().GetSize(), ()); } void Route::AppendRoute(Route const & route) diff --git a/routing/routing_helpers.cpp b/routing/routing_helpers.cpp index a1ba85f8f1..beece98afc 100644 --- a/routing/routing_helpers.cpp +++ b/routing/routing_helpers.cpp @@ -8,7 +8,7 @@ namespace routing using namespace traffic; void ReconstructRoute(IDirectionsEngine * engine, IRoadGraph const & graph, - shared_ptr trafficColoring, + shared_ptr const & trafficColoring, my::Cancellable const & cancellable, vector & path, Route & route) { if (path.empty()) @@ -28,9 +28,13 @@ void ReconstructRoute(IDirectionsEngine * engine, IRoadGraph const & graph, vector junctions; // @TODO(bykoianko) streetNames is not filled in Generate(). It should be done. Route::TStreets streetNames; - vector routeSegs; + vector trafficSegs; if (engine) - engine->Generate(graph, path, times, turnsDir, junctions, routeSegs, cancellable); + engine->Generate(graph, path, times, turnsDir, junctions, trafficSegs, cancellable); + + // @TODO(bykoianko) If the start and the finish of a route lies on the same road segemnt + // engine->Generate() fills with empty vectors |times|, |turnsDir|, |junctions| and |trafficSegs|. + // It's not correct and should be fixed. It's necessary to work corrrectly with such routes. vector routeGeometry; JunctionsToPoints(junctions, routeGeometry); @@ -44,12 +48,12 @@ void ReconstructRoute(IDirectionsEngine * engine, IRoadGraph const & graph, route.SetAltitudes(move(altitudes)); vector traffic; - traffic.reserve(routeSegs.size()); - if (trafficColoring) + if (trafficColoring && !trafficSegs.empty()) { - for (TrafficInfo::RoadSegmentId const & rp : routeSegs) + traffic.reserve(2 * trafficSegs.size()); + for (TrafficInfo::RoadSegmentId const & seg : trafficSegs) { - auto const it = trafficColoring->find(rp); + auto const it = trafficColoring->find(seg); SpeedGroup segTraffic = (it == trafficColoring->cend()) ? SpeedGroup::Unknown : it->second; // @TODO It's written to compensate an error. The problem is in case of any routing except for osrm @@ -58,16 +62,16 @@ void ReconstructRoute(IDirectionsEngine * engine, IRoadGraph const & graph, // At this moment the route looks like: // 0----1 4----5 // 2----3 6---7 - // This route composes of 4 edges and there're 4 items in routeSegs. + // This route consists of 4 edges and there're 4 items in trafficSegs. // But it has 8 items in routeGeometry. - // So for segments 0-1 and 1-2 let's set routeSegs[0] - // for segments 2-3 and 3-4 let's set routeSegs[1] - // for segments 4-5 and 5-7 let's set routeSegs[2] - // and for segment 6-7 let's set routeSegs[3] + // So for segments 0-1 and 1-2 let's set trafficSegs[0] + // for segments 2-3 and 3-4 let's set trafficSegs[1] + // for segments 4-5 and 5-7 let's set trafficSegs[2] + // and for segment 6-7 let's set trafficSegs[3] traffic.insert(traffic.end(), {segTraffic, segTraffic}); } - if (!traffic.empty()) - traffic.pop_back(); + CHECK(!traffic.empty(), ()); + traffic.pop_back(); } route.SetTraffic(move(traffic)); diff --git a/routing/routing_helpers.hpp b/routing/routing_helpers.hpp index bf7306eb73..32556fec19 100644 --- a/routing/routing_helpers.hpp +++ b/routing/routing_helpers.hpp @@ -26,6 +26,6 @@ bool IsRoad(TTypes const & types) } void ReconstructRoute(IDirectionsEngine * engine, IRoadGraph const & graph, - shared_ptr trafficColoring, + shared_ptr const & trafficColoring, my::Cancellable const & cancellable, vector & path, Route & route); } // namespace rouing diff --git a/routing/single_mwm_router.cpp b/routing/single_mwm_router.cpp index 2bf8a04d63..bef9c9cdb6 100644 --- a/routing/single_mwm_router.cpp +++ b/routing/single_mwm_router.cpp @@ -151,6 +151,8 @@ IRouter::ResultCode SingleMwmRouter::DoCalculateRoute(MwmSet::MwmId const & mwmI vector path = ConvertToJunctions(starter, routingResult.path); shared_ptr trafficColoring = m_trafficCache.GetTrafficInfo(mwmId); ReconstructRoute(m_directionsEngine.get(), m_roadGraph, trafficColoring, delegate, path, route); + if (delegate.IsCancelled()) + return IRouter::Cancelled; return IRouter::NoError; } } diff --git a/routing/turns_generator.cpp b/routing/turns_generator.cpp index 272d714f71..a8f04925bb 100644 --- a/routing/turns_generator.cpp +++ b/routing/turns_generator.cpp @@ -258,7 +258,7 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, vector & junctions, Route::TTurns & turnsDir, Route::TTimes & times, Route::TStreets & streets, - vector & routeSegs) + vector & trafficSegs) { double estimatedTime = 0; @@ -273,7 +273,7 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, // Annotate turns. size_t skipTurnSegments = 0; auto const & loadedSegments = result.GetSegments(); - routeSegs.reserve(loadedSegments.size()); + trafficSegs.reserve(loadedSegments.size()); for (auto loadedSegmentIt = loadedSegments.cbegin(); loadedSegmentIt != loadedSegments.cend(); ++loadedSegmentIt) { @@ -323,8 +323,8 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, // Path geometry. junctions.insert(junctions.end(), loadedSegmentIt->m_path.begin(), loadedSegmentIt->m_path.end()); - routeSegs.insert(routeSegs.end(), loadedSegmentIt->m_routeSegs.cbegin(), - loadedSegmentIt->m_routeSegs.cend()); + trafficSegs.insert(trafficSegs.end(), loadedSegmentIt->m_trafficSegs.cbegin(), + loadedSegmentIt->m_trafficSegs.cend()); } // Path found. Points will be replaced by start and end edges junctions. diff --git a/routing/turns_generator.hpp b/routing/turns_generator.hpp index c867be1cbb..6bb230dde2 100644 --- a/routing/turns_generator.hpp +++ b/routing/turns_generator.hpp @@ -12,7 +12,6 @@ #include "traffic/traffic_info.hpp" #include "std/function.hpp" -#include "std/shared_ptr.hpp" #include "std/utility.hpp" #include "std/vector.hpp" @@ -51,7 +50,7 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, vector & points, Route::TTurns & turnsDir, Route::TTimes & times, Route::TStreets & streets, - vector & routeSegs); + vector & trafficSegs); /*! * \brief The TurnInfo struct is a representation of a junction.