From 3cb8c405010ef303f2580f92d471c2b2905398aa Mon Sep 17 00:00:00 2001 From: Viktor Govako Date: Thu, 17 Aug 2023 22:25:09 -0300 Subject: [PATCH 1/3] [routing] Reduced ferry landing penalty. Signed-off-by: Viktor Govako --- routing/edge_estimator.cpp | 62 ++++++++----------- .../routing_integration_tests/route_test.cpp | 28 +++++++++ routing/single_vehicle_world_graph.cpp | 1 + 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/routing/edge_estimator.cpp b/routing/edge_estimator.cpp index c6c463efe8..f70aeef123 100644 --- a/routing/edge_estimator.cpp +++ b/routing/edge_estimator.cpp @@ -271,13 +271,12 @@ public: // EdgeEstimator overrides: double GetUTurnPenalty(Purpose /* purpose */) const override { return 0.0 /* seconds */; } - // Based on: https://confluence.mail.ru/display/MAPSME/Ferries double GetFerryLandingPenalty(Purpose purpose) const override { switch (purpose) { - case Purpose::Weight: return 20.0 * 60.0; // seconds - case Purpose::ETA: return 8.0 * 60.0; // seconds + case Purpose::Weight: return 10 * 60; // seconds + case Purpose::ETA: return 8 * 60; // seconds } UNREACHABLE(); } @@ -303,13 +302,12 @@ public: // EdgeEstimator overrides: double GetUTurnPenalty(Purpose /* purpose */) const override { return 20.0 /* seconds */; } - // Based on: https://confluence.mail.ru/display/MAPSME/Ferries double GetFerryLandingPenalty(Purpose purpose) const override { switch (purpose) { - case Purpose::Weight: return 20 * 60; // seconds - case Purpose::ETA: return 8 * 60; // seconds + case Purpose::Weight: return 10 * 60; // seconds + case Purpose::ETA: return 8 * 60; // seconds } UNREACHABLE(); } @@ -358,44 +356,36 @@ class CarEstimator final : public EdgeEstimator public: CarEstimator(DataSource * dataSourcePtr, std::shared_ptr numMwmIds, shared_ptr trafficStash, double maxWeightSpeedKMpH, - SpeedKMpH const & offroadSpeedKMpH); + SpeedKMpH const & offroadSpeedKMpH) + : EdgeEstimator(maxWeightSpeedKMpH, offroadSpeedKMpH, dataSourcePtr, numMwmIds) + , m_trafficStash(std::move(trafficStash)) + { + } // EdgeEstimator overrides: double CalcSegmentWeight(Segment const & segment, RoadGeometry const & road, Purpose purpose) const override; - double GetUTurnPenalty(Purpose /* purpose */) const override; - double GetFerryLandingPenalty(Purpose purpose) const override; + double GetUTurnPenalty(Purpose /* purpose */) const override + { + // Adds 2 minutes penalty for U-turn. The value is quite arbitrary + // and needs to be properly selected after a number of real-world + // experiments. + return 2 * 60; // seconds + } + + double GetFerryLandingPenalty(Purpose purpose) const override + { + switch (purpose) + { + case Purpose::Weight: return 20 * 60; // seconds + case Purpose::ETA: return 20 * 60; // seconds + } + UNREACHABLE(); + } private: shared_ptr m_trafficStash; }; -CarEstimator::CarEstimator(DataSource * dataSourcePtr, std::shared_ptr numMwmIds, - shared_ptr trafficStash, double maxWeightSpeedKMpH, - SpeedKMpH const & offroadSpeedKMpH) - : EdgeEstimator(maxWeightSpeedKMpH, offroadSpeedKMpH, dataSourcePtr, numMwmIds) - , m_trafficStash(std::move(trafficStash)) -{ -} - -double CarEstimator::GetUTurnPenalty(Purpose /* purpose */) const -{ - // Adds 2 minutes penalty for U-turn. The value is quite arbitrary - // and needs to be properly selected after a number of real-world - // experiments. - return 2 * 60; // seconds -} - -double CarEstimator::GetFerryLandingPenalty(Purpose purpose) const -{ - switch (purpose) - { - case Purpose::Weight: return 40 * 60; // seconds - // Based on https://confluence.mail.ru/display/MAPSME/Ferries - case Purpose::ETA: return 20 * 60; // seconds - } - UNREACHABLE(); -} - double CarEstimator::CalcSegmentWeight(Segment const & segment, RoadGeometry const & road, Purpose purpose) const { double result = road.GetDistance(segment.GetSegmentIdx()) / GetSpeedMpS(purpose, segment, road); diff --git a/routing/routing_integration_tests/route_test.cpp b/routing/routing_integration_tests/route_test.cpp index fa43f90858..7e7728a5f8 100644 --- a/routing/routing_integration_tests/route_test.cpp +++ b/routing/routing_integration_tests/route_test.cpp @@ -925,4 +925,32 @@ UNIT_TEST(Russia_Yekaterinburg_NChelny) finish, {0., 0.}, start, 755851); } +// https://github.com/organicmaps/organicmaps/issues/5695 +UNIT_TEST(Russia_CrossMwm_Ferry) +{ + TRouteResult const routeResult = CalculateRoute(GetVehicleComponents(VehicleType::Car), + FromLatLon(55.7840398, 54.0815156), {0., 0.}, + FromLatLon(55.7726245, 54.0752932)); + + RouterResultCode const result = routeResult.second; + TEST_EQUAL(result, RouterResultCode::NoError, ()); + + TEST(routeResult.first, ()); + Route const & route = *routeResult.first; + TestRouteLength(route, 1453); + // 2 hours duration (https://www.openstreetmap.org/way/426120647) + 20 minutes ferry landing penalty. + /// @todo Not working now, @see SingleVehicleWorldGraph::CalculateETA. + TEST_GREATER(route.GetTotalTimeSec(), 7200 + 20 * 60, ()); +} + +// https://github.com/organicmaps/organicmaps/issues/6035 +UNIT_TEST(Netherlands_CrossMwm_Ferry) +{ + /// @todo Should work after reducing ferry landing penalty, but nope .. + /// Can't realize what is going on here, maybe penalty is aggregated 2 times? + CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Car), + FromLatLon(52.3855418, 6.12969591), {0., 0.}, + FromLatLon(52.3924362, 6.12166998), 1322); +} + } // namespace route_test diff --git a/routing/single_vehicle_world_graph.cpp b/routing/single_vehicle_world_graph.cpp index 059814738c..1265d523ba 100644 --- a/routing/single_vehicle_world_graph.cpp +++ b/routing/single_vehicle_world_graph.cpp @@ -169,6 +169,7 @@ RouteWeight SingleVehicleWorldGraph::CalcOffroadWeight(ms::LatLon const & from, double SingleVehicleWorldGraph::CalculateETA(Segment const & from, Segment const & to) { + /// @todo Crutch, for example we can loose ferry penalty here (no twin segments), @see Russia_CrossMwm_Ferry. if (from.GetMwmId() != to.GetMwmId()) return CalculateETAWithoutPenalty(to); -- 2.45.3 From f429b647cd68e052cd4134ca371d53e384edfd21 Mon Sep 17 00:00:00 2001 From: Viktor Govako Date: Thu, 17 Aug 2023 22:24:46 -0300 Subject: [PATCH 2/3] [routing] Better max pedestrian model speed considering ferry. Signed-off-by: Viktor Govako --- routing_common/pedestrian_model.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routing_common/pedestrian_model.cpp b/routing_common/pedestrian_model.cpp index 26debcf350..3e7bf0333d 100644 --- a/routing_common/pedestrian_model.cpp +++ b/routing_common/pedestrian_model.cpp @@ -173,7 +173,7 @@ PedestrianModel::PedestrianModel(VehicleModel::LimitsInitList const & limits, AddAdditionalRoadTypes(cl, {{ std::move(hwtagYesFoot), kDefaultSpeeds.Get(HighwayType::HighwayLivingStreet) }}); // Update max pedestrian speed with possible ferry transfer. See EdgeEstimator::CalcHeuristic. - SpeedKMpH constexpr kMaxPedestrianSpeedKMpH(25.0); + SpeedKMpH constexpr kMaxPedestrianSpeedKMpH(60.0); CHECK_LESS(m_maxModelSpeed, kMaxPedestrianSpeedKMpH, ()); m_maxModelSpeed = kMaxPedestrianSpeedKMpH; } -- 2.45.3 From 6b312c81f08daca5da5eaa2d208a1c2b93e71095 Mon Sep 17 00:00:00 2001 From: Viktor Govako Date: Thu, 7 Sep 2023 13:27:15 -0300 Subject: [PATCH 3/3] [routing] Minor m_fakeJointSegments routine cleanup. Signed-off-by: Viktor Govako --- generator/routing_index_generator.cpp | 11 ++-- routing/index_graph_starter_joints.hpp | 88 ++++++++++---------------- routing/joint_segment.cpp | 15 ++--- routing/joint_segment.hpp | 8 +-- routing/segment.cpp | 2 - routing/segment.hpp | 7 +- 6 files changed, 52 insertions(+), 79 deletions(-) diff --git a/generator/routing_index_generator.cpp b/generator/routing_index_generator.cpp index 6fcae33b20..bdd4b25cbc 100644 --- a/generator/routing_index_generator.cpp +++ b/generator/routing_index_generator.cpp @@ -536,11 +536,12 @@ void FillWeights(string const & path, string const & mwmFile, string const & cou std::unordered_map> visitedVertexes; astar.PropagateWave( wrapper, wrapper.GetStartJoint(), - [&](JointSegment const & vertex) { + [&](JointSegment const & vertex) + { if (vertex.IsFake()) { - Segment start = wrapper.GetSegmentOfFakeJoint(vertex, true /* start */); - Segment end = wrapper.GetSegmentOfFakeJoint(vertex, false /* start */); + auto const & start = wrapper.GetSegmentOfFakeJoint(vertex, true /* start */); + auto const & end = wrapper.GetSegmentOfFakeJoint(vertex, false /* start */); if (start.IsForward() != end.IsForward()) return true; @@ -579,9 +580,7 @@ void FillWeights(string const & path, string const & mwmFile, string const & cou if (context.HasParent(jointSegment)) { JointSegment const & parent = context.GetParent(jointSegment); - parentSegment = parent.IsFake() ? wrapper.GetSegmentOfFakeJoint(parent, false /* start */) - : parent.GetSegment(false /* start */); - + parentSegment = wrapper.GetSegmentFromJoint(parent, false /* start */); weight = context.GetDistance(parent); } else diff --git a/routing/index_graph_starter_joints.hpp b/routing/index_graph_starter_joints.hpp index 41e8880e53..51f2d4e908 100644 --- a/routing/index_graph_starter_joints.hpp +++ b/routing/index_graph_starter_joints.hpp @@ -39,7 +39,10 @@ public: void Init(Segment const & startSegment, Segment const & endSegment); - ms::LatLon const & GetPoint(JointSegment const & jointSegment, bool start); + ms::LatLon const & GetPoint(JointSegment const & jointSegment, bool start) const + { + return m_graph.GetPoint(GetSegmentFromJoint(jointSegment, start), jointSegment.IsForward()); + } JointSegment const & GetStartJoint() const { return m_startJoint; } JointSegment const & GetFinishJoint() const { return m_endJoint; } @@ -77,14 +80,12 @@ public: if (!vertex.IsFake()) return; - auto const it = m_fakeJointSegments.find(vertex); - ASSERT(it != m_fakeJointSegments.cend(), ()); - - auto const & first = it->second.GetSegment(true /* start */); + auto const & fake = GetFakeSegmentSure(vertex); + auto const & first = fake.GetSegment(true /* start */); if (first.IsRealSegment()) vertex.AssignID(first); else - vertex.AssignID(it->second.GetSegment(false /* start */)); + vertex.AssignID(fake.GetSegment(false /* start */)); }; return m_graph.AreWavesConnectible(forwardParents, commonVertex, backwardParents, converter); @@ -105,7 +106,14 @@ public: return !segment.IsFakeCreated(); } - Segment const & GetSegmentOfFakeJoint(JointSegment const & joint, bool start); + Segment const & GetSegmentOfFakeJoint(JointSegment const & joint, bool start) const + { + return GetFakeSegmentSure(joint).GetSegment(start); + } + Segment GetSegmentFromJoint(JointSegment const & joint, bool start) const + { + return joint.IsFake() ? GetSegmentOfFakeJoint(joint, start) : joint.GetSegment(start); + } private: static auto constexpr kInvalidFeatureId = FakeFeatureIds::kIndexGraphStarterId; @@ -174,6 +182,13 @@ private: jointSegment.GetStartSegmentId() != kInvalidId; } + FakeJointSegment const & GetFakeSegmentSure(JointSegment const & key) const + { + auto const it = m_fakeJointSegments.find(key); + ASSERT(it != m_fakeJointSegments.end(), (key)); + return it->second; + } + Graph & m_graph; // Fake start and end joints. @@ -303,23 +318,6 @@ RouteWeight IndexGraphStarterJoints::HeuristicCostEstimate(JointSegment c : m_graph.HeuristicCostEstimate(fromSegment, m_startPoint); } -template -ms::LatLon const & -IndexGraphStarterJoints::GetPoint(JointSegment const & jointSegment, bool start) -{ - Segment segment; - if (jointSegment.IsFake()) - { - auto const it = m_fakeJointSegments.find(jointSegment); - CHECK(it != m_fakeJointSegments.end(), ("No such fake joint:", jointSegment)); - segment = it->second.GetSegment(start); - } - else - segment = jointSegment.GetSegment(start); - - return m_graph.GetPoint(segment, jointSegment.IsForward()); -} - template std::vector IndexGraphStarterJoints::ReconstructJoint(JointSegment const & joint) { @@ -367,18 +365,16 @@ void IndexGraphStarterJoints::AddFakeJoints(Segment const & segment, bool // of fake joints, entered to finish and vice versa. EdgeListT const & endings = isOutgoing ? m_endOutEdges : m_startOutEdges; - bool const opposite = !isOutgoing; for (auto const & edge : endings) { // The one end of FakeJointSegment is start/finish and the opposite end is real segment. // So we check, whether |segment| is equal to the real segment of FakeJointSegment. // If yes, that means, that we can go from |segment| to start/finish. auto const it = m_fakeJointSegments.find(edge.GetTarget()); - if (it == m_fakeJointSegments.cend()) + if (it == m_fakeJointSegments.end()) continue; - auto const & fakeJointSegment = it->second; - Segment const & firstSegment = fakeJointSegment.GetSegment(!opposite /* start */); + Segment const & firstSegment = it->second.GetSegment(isOutgoing /* start */); if (firstSegment == segment) { edges.emplace_back(edge); @@ -387,14 +383,6 @@ void IndexGraphStarterJoints::AddFakeJoints(Segment const & segment, bool } } -template -Segment const & IndexGraphStarterJoints::GetSegmentOfFakeJoint(JointSegment const & joint, bool start) -{ - auto const it = m_fakeJointSegments.find(joint); - CHECK(it != m_fakeJointSegments.cend(), ("No such fake joint:", joint)); - return it->second.GetSegment(start); -} - template std::optional IndexGraphStarterJoints::GetParentSegment( JointSegment const & vertex, bool isOutgoing, EdgeListT & edges) @@ -403,21 +391,18 @@ std::optional IndexGraphStarterJoints::GetParentSegment( bool const opposite = !isOutgoing; if (vertex.IsFake()) { - CHECK(m_fakeJointSegments.find(vertex) != m_fakeJointSegments.end(), - ("No such fake joint:", vertex, "in JointStarter.")); + FakeJointSegment const & fakeJointSegment = GetFakeSegmentSure(vertex); - FakeJointSegment const & fakeJointSegment = m_fakeJointSegments[vertex]; - - auto const & startSegment = isOutgoing ? m_startSegment : m_endSegment; auto const & endSegment = isOutgoing ? m_endSegment : m_startSegment; - auto const & endJoint = isOutgoing ? m_endJoint : m_startJoint; + parentSegment = fakeJointSegment.GetSegment(opposite /* start */); // This is case when we can build route from start to finish without real segment, only fake. // It happens when start and finish are close to each other. // If we want A* stop, we should add |endJoint| to its queue, then A* will see the vertex: |endJoint| // where it has already been and stop working. - if (fakeJointSegment.GetSegment(opposite /* start */) == endSegment) + if (parentSegment == endSegment) { + auto const & endJoint = isOutgoing ? m_endJoint : m_startJoint; if (isOutgoing) { static auto constexpr kZeroWeight = RouteWeight(0.0); @@ -434,8 +419,8 @@ std::optional IndexGraphStarterJoints::GetParentSegment( return {}; } - CHECK(fakeJointSegment.GetSegment(!opposite /* start */) == startSegment, ()); - parentSegment = fakeJointSegment.GetSegment(opposite /* start */); + auto const & startSegment = isOutgoing ? m_startSegment : m_endSegment; + ASSERT_EQUAL(fakeJointSegment.GetSegment(!opposite /* start */), startSegment, ()); } else { @@ -477,16 +462,11 @@ bool IndexGraphStarterJoints::FillEdgesAndParentsWeights( m_graph.GetEdgeList(vertexData, parentSegment, isOutgoing, edges, parentWeights); firstFakeId = edges.size(); - bool const opposite = !isOutgoing; for (size_t i = 0; i < firstFakeId; ++i) { - size_t prevSize = edges.size(); - auto const & target = edges[i].GetTarget(); + size_t const prevSize = edges.size(); - auto const & segment = target.IsFake() ? m_fakeJointSegments[target].GetSegment(!opposite) - : target.GetSegment(!opposite); - - AddFakeJoints(segment, isOutgoing, edges); + AddFakeJoints(GetSegmentFromJoint(edges[i].GetTarget(), isOutgoing), isOutgoing, edges); // If we add fake edge, we should add new parentWeight as "child[i] -> parent". // Because fake edge and current edge (jointEdges[i]) have the same first @@ -579,7 +559,7 @@ JointSegment IndexGraphStarterJoints::CreateFakeJoint(Segment const & fro uint32_t featureId/* = kInvalidFeatureId*/) { JointSegment const result = JointSegment::MakeFake(m_fakeId++, featureId); - m_fakeJointSegments[result] = FakeJointSegment(from, to); + m_fakeJointSegments.emplace(result, FakeJointSegment(from, to)); return result; } @@ -687,7 +667,7 @@ template JointSegment IndexGraphStarterJoints::CreateInvisibleJoint(Segment const & segment, bool start) { JointSegment const result = JointSegment::MakeFake(start ? kInvisibleStartId : kInvisibleEndId); - m_fakeJointSegments[result] = FakeJointSegment(segment, segment); + m_fakeJointSegments.emplace(result, FakeJointSegment(segment, segment)); return result; } diff --git a/routing/joint_segment.cpp b/routing/joint_segment.cpp index 62e44ef152..77854dbe76 100644 --- a/routing/joint_segment.cpp +++ b/routing/joint_segment.cpp @@ -76,11 +76,11 @@ Segment JointSegment::GetSegment(bool start) const bool JointSegment::operator<(JointSegment const & rhs) const { - if (m_featureId != rhs.GetFeatureId()) - return m_featureId < rhs.GetFeatureId(); + if (m_featureId != rhs.m_featureId) + return m_featureId < rhs.m_featureId; - if (m_forward != rhs.IsForward()) - return m_forward < rhs.IsForward(); + if (m_forward != rhs.m_forward) + return m_forward < rhs.m_forward; if (m_startSegmentId != rhs.m_startSegmentId) return m_startSegmentId < rhs.m_startSegmentId; @@ -88,7 +88,7 @@ bool JointSegment::operator<(JointSegment const & rhs) const if (m_endSegmentId != rhs.m_endSegmentId) return m_endSegmentId < rhs.m_endSegmentId; - return m_numMwmId < rhs.GetMwmId(); + return m_numMwmId < rhs.m_numMwmId; } bool JointSegment::operator==(JointSegment const & rhs) const @@ -98,11 +98,6 @@ bool JointSegment::operator==(JointSegment const & rhs) const m_numMwmId == rhs.m_numMwmId; } -bool JointSegment::operator!=(JointSegment const & rhs) const -{ - return !(*this == rhs); -} - std::string DebugPrint(JointSegment const & jointSegment) { std::ostringstream out; diff --git a/routing/joint_segment.hpp b/routing/joint_segment.hpp index 9954dd2e96..24c7f954f2 100644 --- a/routing/joint_segment.hpp +++ b/routing/joint_segment.hpp @@ -4,9 +4,6 @@ #include "routing/route_weight.hpp" #include "routing/segment.hpp" -#include "base/assert.hpp" - -#include #include #include @@ -37,7 +34,10 @@ public: bool operator<(JointSegment const & rhs) const; bool operator==(JointSegment const & rhs) const; - bool operator!=(JointSegment const & rhs) const; + bool operator!=(JointSegment const & rhs) const + { + return !(*this == rhs); + } private: static uint32_t constexpr kInvalidFeatureId = FakeFeatureIds::kIndexGraphStarterId; diff --git a/routing/segment.cpp b/routing/segment.cpp index 830a868968..cfc8150578 100644 --- a/routing/segment.cpp +++ b/routing/segment.cpp @@ -34,8 +34,6 @@ bool Segment::operator==(Segment const & seg) const m_mwmId == seg.m_mwmId && m_forward == seg.m_forward; } -bool Segment::operator!=(Segment const & seg) const { return !(*this == seg); } - bool Segment::IsInverse(Segment const & seg) const { return m_featureId == seg.m_featureId && m_segmentIdx == seg.m_segmentIdx && diff --git a/routing/segment.hpp b/routing/segment.hpp index 81e6e716c7..b10e25b922 100644 --- a/routing/segment.hpp +++ b/routing/segment.hpp @@ -5,8 +5,6 @@ #include "routing_common/num_mwm_id.hpp" -#include -#include #include namespace routing @@ -44,7 +42,10 @@ public: bool operator<(Segment const & seg) const; bool operator==(Segment const & seg) const; - bool operator!=(Segment const & seg) const; + bool operator!=(Segment const & seg) const + { + return !(*this == seg); + } bool IsInverse(Segment const & seg) const; Segment GetReversed() const { return { m_mwmId, m_featureId, m_segmentIdx, !m_forward }; } -- 2.45.3