From 273860976e77319eb588451e5e7043a76d2ba512 Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Mon, 5 Jun 2017 11:17:28 +0300 Subject: [PATCH] Turn generation fix. Removing segments duplicates. --- routing/bicycle_directions.cpp | 266 ++++++++++++++++++++------------ routing/bicycle_directions.hpp | 24 ++- routing/directions_engine.cpp | 2 +- routing/index_router.cpp | 30 ---- routing/loaded_path_segment.hpp | 2 + routing/routing_helpers.cpp | 18 +-- routing/turns.cpp | 32 ++-- routing/turns.hpp | 31 ++-- routing/turns_generator.cpp | 8 +- 9 files changed, 240 insertions(+), 173 deletions(-) diff --git a/routing/bicycle_directions.cpp b/routing/bicycle_directions.cpp index a28a3cc43a..9ba92dde89 100644 --- a/routing/bicycle_directions.cpp +++ b/routing/bicycle_directions.cpp @@ -15,6 +15,10 @@ #include "geometry/point2d.hpp" +#include +#include +#include + namespace { using namespace routing; @@ -118,6 +122,10 @@ void BicycleDirectionsEngine::Generate(RoadGraphBase const & graph, vectorGetId(inFeatureId.m_mwmId.GetInfo()->GetLocalFile().GetCountryFile()); - pathSegment.m_trafficSegs = {{numMwmId, inFeatureId.m_index, inSegId, inIsForward}}; - } - - auto const it = m_adjacentEdges.insert(make_pair(uniNodeId, move(adjacentEdges))); - ASSERT(it.second, ()); - UNUSED_VALUE(it); - m_pathSegments.push_back(move(pathSegment)); - } + if (cancellable.IsCancelled()) + return; RoutingResult resultGraph(routeEdges, m_adjacentEdges, m_pathSegments); RouterDelegate delegate; - Route::TTimes turnAnnotationTimes; + Route::TTimes dummyTimes; Route::TStreets streetNames; - MakeTurnAnnotation(resultGraph, delegate, routeGeometry, turns, turnAnnotationTimes, - streetNames, trafficSegs); - - // @TODO(bykoianko) The invariant below it's an issue but now it's so and it should be checked. - // The problem is every edge is added as a pair of points to route geometry. - // So all the points except for beginning and ending are duplicated. It should - // be fixed in the future. - - // Note 1. Accoding to current implementation all internal points are duplicated for - // all A* routes. - // Note 2. Number of |trafficSegs| should be equal to number of |routeEdges|. - ASSERT_EQUAL(routeGeometry.size(), 2 * (pathSize - 2) + 2, ()); - ASSERT_EQUAL(trafficSegs.size(), pathSize - 1, ()); + MakeTurnAnnotation(resultGraph, delegate, routeGeometry, turns, dummyTimes, streetNames, + trafficSegs); + CHECK_EQUAL(routeGeometry.size(), pathSize, ()); + if (!trafficSegs.empty()) + CHECK_EQUAL(trafficSegs.size(), routeEdges.size(), ()); } Index::FeaturesLoaderGuard & BicycleDirectionsEngine::GetLoader(MwmSet::MwmId const & id) @@ -222,26 +165,13 @@ Index::FeaturesLoaderGuard & BicycleDirectionsEngine::GetLoader(MwmSet::MwmId co return *m_loader; } -void BicycleDirectionsEngine::LoadPathGeometry(UniNodeId const & uniNodeId, - vector const & path, - LoadedPathSegment & pathSegment) +void BicycleDirectionsEngine::LoadPathAttributes(FeatureID const & featureId, LoadedPathSegment & pathSegment) { - pathSegment.Clear(); - - if (!uniNodeId.GetFeature().IsValid()) - { - ASSERT(false, ()); + if (!featureId.IsValid()) return; - } FeatureType ft; - if (!GetLoader(uniNodeId.GetFeature().m_mwmId) - .GetFeatureByIndex(uniNodeId.GetFeature().m_index, ft)) - { - // The feature can't be read, therefore path geometry can't be - // loaded. - return; - } + GetLoader(featureId.m_mwmId).GetFeatureByIndex(featureId.m_index, ft); auto const highwayClass = ftypes::GetHighwayClass(ft); ASSERT_NOT_EQUAL(highwayClass, ftypes::HighwayClass::Error, ()); @@ -249,12 +179,158 @@ void BicycleDirectionsEngine::LoadPathGeometry(UniNodeId const & uniNodeId, pathSegment.m_highwayClass = highwayClass; pathSegment.m_isLink = ftypes::IsLinkChecker::Instance()(ft); - ft.GetName(FeatureType::DEFAULT_LANG, pathSegment.m_name); - - pathSegment.m_nodeId = uniNodeId; pathSegment.m_onRoundabout = ftypes::IsRoundAboutChecker::Instance()(ft); - pathSegment.m_path = path; - // @TODO(bykoianko) It's better to fill pathSegment.m_weight. +} + +void BicycleDirectionsEngine::GetUniNodeIdAndAdjacentEdges( + IRoadGraph::TEdgeVector const & outgoingEdges, FeatureID const & inFeatureId, + uint32_t startSegId, uint32_t endSegId, bool inIsForward, UniNodeId & uniNodeId, + BicycleDirectionsEngine::AdjacentEdges & adjacentEdges) +{ + // Outgoing edge angle is not used for bicycle routing. + adjacentEdges.m_outgoingTurns.isCandidatesAngleValid = false; + adjacentEdges.m_outgoingTurns.candidates.reserve(outgoingEdges.size()); + uniNodeId = UniNodeId(inFeatureId, startSegId, endSegId, inIsForward); + + for (auto const & edge : outgoingEdges) + { + auto const & outFeatureId = edge.GetFeatureId(); + // Checking if |edge| is a fake edge. + if (!outFeatureId.IsValid()) + continue; + + FeatureType ft; + GetLoader(outFeatureId.m_mwmId).GetFeatureByIndex(outFeatureId.m_index, ft); + + auto const highwayClass = ftypes::GetHighwayClass(ft); + ASSERT_NOT_EQUAL(highwayClass, ftypes::HighwayClass::Error, ()); + ASSERT_NOT_EQUAL(highwayClass, ftypes::HighwayClass::Undefined, ()); + adjacentEdges.m_outgoingTurns.candidates.emplace_back(0.0 /* angle */, uniNodeId, highwayClass); + } +} + +bool BicycleDirectionsEngine::IsJoint(IRoadGraph::TEdgeVector const & ingoingEdges, + IRoadGraph::TEdgeVector const & outgoingEdges, + Edge const & ingoingRouteEdge, Edge const & outgoingRouteEdge) +{ + if (ingoingRouteEdge.GetFeatureId() != outgoingRouteEdge.GetFeatureId()) + return true; + + FeatureID const & featureId = ingoingRouteEdge.GetFeatureId(); + uint32_t const segOut = outgoingRouteEdge.GetSegId(); + for (Edge const & e : ingoingEdges) + { + // It's necessary to compare segments for cases when |featureId| is a loop. + if (e.GetFeatureId() != featureId || abs(static_cast(segOut - e.GetSegId())) != 1) + return true; + } + + uint32_t const segIn = ingoingRouteEdge.GetSegId(); + for (Edge const & e : outgoingEdges) + { + // It's necessary to compare segments for cases when |featureId| is a loop. + if (e.GetFeatureId() != featureId || abs(static_cast(segIn - e.GetSegId())) != 1) + return true; + } + return false; +} + +bool BicycleDirectionsEngine::GetSegment(FeatureID const & featureId, uint32_t segId, + bool isFeatureForward, Segment & segment) +{ + if (!m_numMwmIds) + return false; + + if (!featureId.m_mwmId.IsAlive()) + { + ASSERT(featureId.m_mwmId.IsAlive(), ("Feature:", featureId, "is not alive.")); + return false; + } + + NumMwmId const numMwmId = + m_numMwmIds->GetId(featureId.m_mwmId.GetInfo()->GetLocalFile().GetCountryFile()); + segment = Segment(numMwmId, featureId.m_index, segId, isFeatureForward); + return true; +} + +void BicycleDirectionsEngine::FillPathSegmentsAndAdjacentEdgesMap( + RoadGraphBase const & graph, vector const & path, + IRoadGraph::TEdgeVector const & routeEdges, my::Cancellable const & cancellable) +{ + size_t const pathSize = path.size(); + CHECK_GREATER(pathSize, 1, ()); + // Filling |m_adjacentEdges|. + m_adjacentEdges.insert(make_pair(UniNodeId(UniNodeId::Type::Mwm), AdjacentEdges(0))); + uint32_t constexpr kInvalidSegId = std::numeric_limits::max(); + // |startSegId| is a value to keep start segment id of a new instance of LoadedPathSegment. + uint32_t startSegId = kInvalidSegId; + vector prevJunctions; + vector prevSegments; + for (size_t i = 1; i < pathSize; ++i) + { + if (cancellable.IsCancelled()) + return; + + Junction const & prevJunction = path[i - 1]; + Junction const & currJunction = path[i]; + IRoadGraph::TEdgeVector ingoingEdges; + IRoadGraph::TEdgeVector outgoingEdges; + graph.GetOutgoingEdges(currJunction, outgoingEdges); + graph.GetIngoingEdges(currJunction, ingoingEdges); + ASSERT_EQUAL(routeEdges.size(), pathSize - 1, ()); + + Edge const & inEdge = routeEdges[i - 1]; + // Note. |inFeatureId| may be invalid in case of adding fake features. + // It happens for example near starts and a finishes. + FeatureID const & inFeatureId = inEdge.GetFeatureId(); + uint32_t const inSegId = inEdge.GetSegId(); + bool const inIsForward = inEdge.IsForward(); + + if (startSegId == kInvalidSegId) + startSegId = inSegId; + + if (inFeatureId.IsValid() && i + 1 != pathSize && !IsJoint(ingoingEdges, outgoingEdges, inEdge, routeEdges[i])) + { + prevJunctions.push_back(prevJunction); + Segment inSegment; + if (GetSegment(inFeatureId, inSegId, inIsForward, inSegment)) + prevSegments.push_back(inSegment); + continue; + } + CHECK_EQUAL(prevJunctions.size(), abs(static_cast(inSegId - startSegId)), ()); + + prevJunctions.push_back(prevJunction); + prevJunctions.push_back(currJunction); + Segment segment; + if (GetSegment(inFeatureId, inSegId, inIsForward, segment)) + prevSegments.push_back(segment); + + AdjacentEdges adjacentEdges(ingoingEdges.size()); + UniNodeId uniNodeId(UniNodeId::Type::Mwm); + GetUniNodeIdAndAdjacentEdges(outgoingEdges, inFeatureId, startSegId, inSegId + 1, inIsForward, + uniNodeId, adjacentEdges); + + size_t const prevJunctionSize = prevJunctions.size(); + LoadedPathSegment pathSegment(UniNodeId::Type::Mwm); + LoadPathAttributes(uniNodeId.GetFeature(), pathSegment); + pathSegment.m_nodeId = uniNodeId; + pathSegment.m_path = std::move(prevJunctions); + // @TODO(bykoianko) |pathSegment.m_weight| should be filled here. + + // Checking if |prevJunctions| reflects real feature ids. If not it means that |prevJunctions| + // reflects fake features near starts of finishes. + if (prevSegments.size() + 1 == prevJunctionSize) + pathSegment.m_trafficSegs = std::move(prevSegments); + + auto const it = m_adjacentEdges.insert(make_pair(uniNodeId, std::move(adjacentEdges))); + ASSERT(it.second, ()); + UNUSED_VALUE(it); + m_pathSegments.push_back(std::move(pathSegment)); + + prevJunctions.clear(); + prevSegments.clear(); + startSegId = kInvalidSegId; + } } } // namespace routing diff --git a/routing/bicycle_directions.hpp b/routing/bicycle_directions.hpp index 63a2efc2da..5d24a736c9 100644 --- a/routing/bicycle_directions.hpp +++ b/routing/bicycle_directions.hpp @@ -33,8 +33,28 @@ public: private: Index::FeaturesLoaderGuard & GetLoader(MwmSet::MwmId const & id); - void LoadPathGeometry(UniNodeId const & uniNodeId, vector const & path, - LoadedPathSegment & pathSegment); + void LoadPathAttributes(FeatureID const & featureId, LoadedPathSegment & pathSegment); + void GetUniNodeIdAndAdjacentEdges(IRoadGraph::TEdgeVector const & outgoingEdges, + FeatureID const & inFeatureId, uint32_t startSegId, + uint32_t endSegId, bool inIsForward, UniNodeId & uniNodeId, + BicycleDirectionsEngine::AdjacentEdges & adjacentEdges); + /// \brief The method gathers sequence of segments according to IsJoint() method + /// and fills |m_adjacentEdges| and |m_pathSegments|. + void FillPathSegmentsAndAdjacentEdgesMap(RoadGraphBase const & graph, + vector const & path, + IRoadGraph::TEdgeVector const & routeEdges, + my::Cancellable const & cancellable); + /// \brief This method should be called for an internal junction of the route with corresponding + /// |ingoingEdges|, |outgoingEdges|, |ingoingRouteEdge| and |outgoingRouteEdge|. + /// \returns false if the junction is an internal point of feature segment and can be considered as + /// a part of LoadedPathSegment and returns true if the junction should be considered as a beginning + /// of a new LoadedPathSegment. + bool IsJoint(IRoadGraph::TEdgeVector const & ingoingEdges, + IRoadGraph::TEdgeVector const & outgoingEdges, Edge const & ingoingRouteEdge, + Edge const & outgoingRouteEdge); + + bool GetSegment(FeatureID const & featureId, uint32_t segId, bool isFeatureForward, + Segment & segment); AdjacentEdgesMap m_adjacentEdges; TUnpackedPathSegments m_pathSegments; diff --git a/routing/directions_engine.cpp b/routing/directions_engine.cpp index cf20d4faec..5819c8d8ee 100644 --- a/routing/directions_engine.cpp +++ b/routing/directions_engine.cpp @@ -19,7 +19,7 @@ void IDirectionsEngine::CalculateTimes(RoadGraphBase const & graph, vector const & segments, RouterDelegate time += starter.CalcSegmentWeight(segments[i]); } - DuplicateRouteTimes(route.GetPoly().GetSize(), times); route.SetSectionTimes(move(times)); return true; } diff --git a/routing/loaded_path_segment.hpp b/routing/loaded_path_segment.hpp index c662d79edf..6c2f10dae2 100644 --- a/routing/loaded_path_segment.hpp +++ b/routing/loaded_path_segment.hpp @@ -54,6 +54,8 @@ struct LoadedPathSegment m_onRoundabout = false; m_isLink = false; } + + bool IsValid() const { return !m_path.empty(); } }; using TUnpackedPathSegments = vector; diff --git a/routing/routing_helpers.cpp b/routing/routing_helpers.cpp index 9ccd4cc98e..8491b7f319 100644 --- a/routing/routing_helpers.cpp +++ b/routing/routing_helpers.cpp @@ -63,7 +63,7 @@ void ReconstructRoute(IDirectionsEngine & engine, RoadGraphBase const & graph, vector traffic; if (trafficStash && !trafficSegs.empty()) { - traffic.reserve(2 * trafficSegs.size()); + traffic.reserve(trafficSegs.size()); for (Segment const & seg : trafficSegs) { traffic::TrafficInfo::RoadSegmentId roadSegment( @@ -78,22 +78,10 @@ void ReconstructRoute(IDirectionsEngine & engine, RoadGraphBase const & graph, if (it != trafficColoring->cend()) segTraffic = it->second; } - // @TODO It's written to compensate an error. The problem is in case of any routing except for osrm - // all route points except for begining and ending are duplicated. - // See a TODO in BicycleDirectionsEngine::Generate() for details. - // At this moment the route looks like: - // 0----1 4----5 - // 2----3 6---7 - // 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 trafficSegs[0] - // for segments 2-3 and 3-4 let's set trafficSegs[1] - // for segments 4-5 and 5-6 let's set trafficSegs[2] - // and for segment 6-7 let's set trafficSegs[3] - traffic.insert(traffic.end(), {segTraffic, segTraffic}); + traffic.push_back(segTraffic); } CHECK(!traffic.empty(), ()); - traffic.pop_back(); + CHECK_EQUAL(trafficSegs.size(), traffic.size(), ()); } route.SetTraffic(move(traffic)); diff --git a/routing/turns.cpp b/routing/turns.cpp index 3373dbafcb..cf9452f98d 100644 --- a/routing/turns.cpp +++ b/routing/turns.cpp @@ -52,6 +52,8 @@ static_assert(g_turnNames.size() == static_cast(TurnDirection::Count), namespace routing { // UniNodeId ------------------------------------------------------------------- +uint32_t UniNodeId::nextFakeId = 0; + bool UniNodeId::operator==(UniNodeId const & rhs) const { if (m_type != rhs.m_type) @@ -61,7 +63,8 @@ bool UniNodeId::operator==(UniNodeId const & rhs) const { case Type::Osrm: return m_nodeId == rhs.m_nodeId; case Type::Mwm: - return m_featureId == rhs.m_featureId && m_segId == rhs.m_segId && m_forward == rhs.m_forward; + return m_featureId == rhs.m_featureId && m_startSegId == rhs.m_startSegId && + m_endSegId == rhs.m_endSegId && m_forward == rhs.m_forward && m_nodeId == rhs.m_nodeId; } } @@ -77,17 +80,24 @@ bool UniNodeId::operator<(UniNodeId const & rhs) const if (m_featureId != rhs.m_featureId) return m_featureId < rhs.m_featureId; - if (m_segId != rhs.m_segId) - return m_segId < rhs.m_segId; + if (m_startSegId != rhs.m_startSegId) + return m_startSegId < rhs.m_startSegId; - return m_forward < rhs.m_forward; + if (m_endSegId != rhs.m_endSegId) + return m_endSegId < rhs.m_endSegId; + + if (m_forward != rhs.m_forward) + return m_forward < rhs.m_forward; + + return m_nodeId < rhs.m_nodeId; } } void UniNodeId::Clear() { m_featureId = FeatureID(); - m_segId = 0; + m_startSegId = 0; + m_endSegId = 0; m_forward = true; m_nodeId = SPECIAL_NODEID; } @@ -104,18 +114,6 @@ FeatureID const & UniNodeId::GetFeature() const return m_featureId; } -uint32_t UniNodeId::GetSegId() const -{ - ASSERT_EQUAL(m_type, Type::Mwm, ()); - return m_segId; -} - -bool UniNodeId::IsForward() const -{ - ASSERT_EQUAL(m_type, Type::Mwm, ()); - return m_forward; -} - string DebugPrint(UniNodeId::Type type) { switch (type) diff --git a/routing/turns.hpp b/routing/turns.hpp index df928aeae8..2582115230 100644 --- a/routing/turns.hpp +++ b/routing/turns.hpp @@ -18,19 +18,25 @@ using TEdgeWeight = double; /// \brief Unique identification for a road edge between two junctions (joints). /// In case of OSRM it's NodeID and in case of RoadGraph (IndexGraph) -/// it's mwm id, feature id, segment id and direction. +/// it's mwm id, feature id, range of segment ids [|m_startSegId|, |m_endSegId|) and direction. struct UniNodeId { enum class Type { - Osrm, - Mwm, + Osrm, // It's an OSRM node id so only |m_type| and |m_nodeId| are valid. + Mwm, // It's a node for A* router so |m_nodeId| is not valid. }; UniNodeId(Type type) : m_type(type) {} - UniNodeId(FeatureID const & featureId, uint32_t segId, bool forward) - : m_type(Type::Mwm), m_featureId(featureId), m_segId(segId), m_forward(forward) + UniNodeId(FeatureID const & featureId, uint32_t startSegId, uint32_t endSegId, bool forward) + : m_type(Type::Mwm) + , m_featureId(featureId) + , m_startSegId(startSegId) + , m_endSegId(endSegId) + , m_forward(forward) { + if (!m_featureId.IsValid()) + m_nodeId = nextFakeId++; } UniNodeId(uint32_t nodeId) : m_type(Type::Osrm), m_nodeId(nodeId) {} bool operator==(UniNodeId const & rh) const; @@ -38,16 +44,17 @@ struct UniNodeId void Clear(); uint32_t GetNodeId() const; FeatureID const & GetFeature() const; - uint32_t GetSegId() const; - bool IsForward() const; private: + static uint32_t nextFakeId; Type m_type; - /// \note In case of OSRM unique id is kept in |m_featureId.m_index|. - /// So |m_featureId.m_mwmId|, |m_segId| and |m_forward| have default values. - FeatureID m_featureId; // |m_featureId.m_index| is NodeID for OSRM. - uint32_t m_segId = 0; // Not valid for OSRM. - bool m_forward = true; // Segment direction in |m_featureId|. + FeatureID m_featureId; // Not valid for OSRM. + uint32_t m_startSegId = 0; // Not valid for OSRM. The first segment index of UniNodeId. + uint32_t m_endSegId = 0; // Not valid for OSRM. The segment index after last of UniNodeId. + bool m_forward = true; // Not valid for OSRM. Segment direction in |m_featureId|. + // Node id for OSRM case. Fake feature id if UniNodeId is based on an invalid feature id valid for + // mwm case. UniNodeId is based on an invalid feature id in case of fake edges near starts and + // finishes. NodeID m_nodeId = SPECIAL_NODEID; }; diff --git a/routing/turns_generator.cpp b/routing/turns_generator.cpp index 2c95c6b067..01bc6ca9f4 100644 --- a/routing/turns_generator.cpp +++ b/routing/turns_generator.cpp @@ -278,6 +278,8 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, for (auto loadedSegmentIt = loadedSegments.cbegin(); loadedSegmentIt != loadedSegments.cend(); ++loadedSegmentIt) { + CHECK(loadedSegmentIt->IsValid(), ()); + // ETA information. double const nodeTimeSeconds = loadedSegmentIt->m_weight; @@ -323,7 +325,11 @@ IRouter::ResultCode MakeTurnAnnotation(turns::IRoutingResult const & result, --skipTurnSegments; // Path geometry. - junctions.insert(junctions.end(), loadedSegmentIt->m_path.begin(), loadedSegmentIt->m_path.end()); + CHECK_GREATER_OR_EQUAL(loadedSegmentIt->m_path.size(), 2, ()); + junctions.insert(junctions.end(), loadedSegmentIt == loadedSegments.cbegin() + ? loadedSegmentIt->m_path.cbegin() + : loadedSegmentIt->m_path.cbegin() + 1, + loadedSegmentIt->m_path.cend()); trafficSegs.insert(trafficSegs.end(), loadedSegmentIt->m_trafficSegs.cbegin(), loadedSegmentIt->m_trafficSegs.cend()); }