From 0b43dc9d8d6164bd4bf9f8a1374d6aa428d6b788 Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Sat, 3 Dec 2016 14:14:51 +0300 Subject: [PATCH] Review fixes. --- map/traffic_manager.cpp | 11 ++++--- map/traffic_manager.hpp | 2 +- routing/edge_estimator.cpp | 8 ++--- routing/edge_estimator.hpp | 16 +--------- routing/routing_session.cpp | 19 ------------ routing/routing_session.hpp | 15 ++-------- .../routing_tests/applying_traffic_test.cpp | 10 ++----- routing/single_mwm_router.cpp | 14 +++++++++ traffic/traffic.pro | 2 ++ traffic/traffic_info.cpp | 8 +++++ traffic/traffic_info.hpp | 11 +------ traffic/traffic_info_getter.cpp | 23 ++++++++++++++ traffic/traffic_info_getter.hpp | 30 +++++++++++++++++++ 13 files changed, 94 insertions(+), 75 deletions(-) create mode 100644 traffic/traffic_info_getter.cpp create mode 100644 traffic/traffic_info_getter.hpp diff --git a/map/traffic_manager.cpp b/map/traffic_manager.cpp index 01e317d0f6..6d3110557f 100644 --- a/map/traffic_manager.cpp +++ b/map/traffic_manager.cpp @@ -192,7 +192,7 @@ void TrafficManager::ThreadRoutine() else { LOG(LWARNING, ("Traffic request failed. Mwm =", mwm)); - OnTrafficRequestFailed(info); + OnTrafficRequestFailed(move(info)); } } mwms.clear(); @@ -271,7 +271,7 @@ void TrafficManager::RequestTrafficData(MwmSet::MwmId const & mwmId) m_condition.notify_one(); } -void TrafficManager::OnTrafficRequestFailed(traffic::TrafficInfo const & info) +void TrafficManager::OnTrafficRequestFailed(traffic::TrafficInfo && info) { lock_guard lock(m_mutex); @@ -320,8 +320,7 @@ void TrafficManager::OnTrafficDataResponse(traffic::TrafficInfo && info) // Update cache. size_t constexpr kElementSize = sizeof(traffic::TrafficInfo::RoadSegmentId) + sizeof(traffic::SpeedGroup); - // Note. It's necessary to multiply by two because routing and rendering use separate caches. - size_t const dataSize = 2 * info.GetColoring().size() * kElementSize; + size_t const dataSize = info.GetColoring().size() * kElementSize; m_currentCacheSizeBytes += (dataSize - it->second.m_dataSize); it->second.m_dataSize = dataSize; CheckCacheSize(); @@ -353,8 +352,8 @@ void TrafficManager::CheckCacheSize() auto const it = m_mwmCache.find(mwmId); if (it->second.m_isLoaded) { - // Note. It's necessary to multiply by two because routing and rendering use separate caches. - m_currentCacheSizeBytes -= 2 * it->second.m_dataSize; + ASSERT_GREATER_OR_EQUAL(m_currentCacheSizeBytes, it->second.m_dataSize, ()); + m_currentCacheSizeBytes -= it->second.m_dataSize; m_drapeEngine->ClearTrafficCache(mwmId); m_observer.OnTrafficInfoRemoved(mwmId); } diff --git a/map/traffic_manager.hpp b/map/traffic_manager.hpp index c1c61bba4e..d2f208a356 100644 --- a/map/traffic_manager.hpp +++ b/map/traffic_manager.hpp @@ -78,7 +78,7 @@ private: bool WaitForRequest(vector & mwms); void OnTrafficDataResponse(traffic::TrafficInfo && info); - void OnTrafficRequestFailed(traffic::TrafficInfo const & info); + void OnTrafficRequestFailed(traffic::TrafficInfo && info); private: // This is a group of methods that haven't their own synchronization inside. diff --git a/routing/edge_estimator.cpp b/routing/edge_estimator.cpp index ce20bdd4a6..1d8cf53d26 100644 --- a/routing/edge_estimator.cpp +++ b/routing/edge_estimator.cpp @@ -76,15 +76,15 @@ double CarEdgeEstimator::CalcEdgesWeight(uint32_t featureId, RoadGeometry const : TrafficInfo::RoadSegmentId::kReverseDirection; for (uint32_t i = start; i < finish; ++i) { - double factor = 1.0; + double edgeWeight = TimeBetweenSec(road.GetPoint(i), road.GetPoint(i + 1), speedMPS); if (m_trafficInfo) { SpeedGroup const speedGroup = m_trafficInfo->GetSpeedGroup(TrafficInfo::RoadSegmentId(featureId, i, dir)); - CHECK_LESS(speedGroup, SpeedGroup::Count, ()); - factor = CalcTrafficFactor(speedGroup); + ASSERT_LESS(speedGroup, SpeedGroup::Count, ()); + edgeWeight *= CalcTrafficFactor(speedGroup); } - result += factor * TimeBetweenSec(road.GetPoint(i), road.GetPoint(i + 1), speedMPS); + result += edgeWeight; } return result; diff --git a/routing/edge_estimator.hpp b/routing/edge_estimator.hpp index 74315ed622..5dfe5a2801 100644 --- a/routing/edge_estimator.hpp +++ b/routing/edge_estimator.hpp @@ -3,7 +3,7 @@ #include "routing/geometry.hpp" #include "routing/vehicle_model.hpp" -#include "traffic/traffic_info.hpp" +#include "traffic/traffic_info_getter.hpp" #include "indexer/mwm_set.hpp" @@ -29,18 +29,4 @@ public: static shared_ptr CreateForCar(IVehicleModel const & vehicleModel, traffic::TrafficInfoGetter const & getter); }; - -class EstimatorGuard final -{ -public: - EstimatorGuard(MwmSet::MwmId const & mwmId, EdgeEstimator & estimator) : m_estimator(estimator) - { - m_estimator.Start(mwmId); - } - - ~EstimatorGuard() { m_estimator.Finish(); } - -private: - EdgeEstimator & m_estimator; -}; } // namespace routing diff --git a/routing/routing_session.cpp b/routing/routing_session.cpp index ea6b6baaf3..a61f85e172 100644 --- a/routing/routing_session.cpp +++ b/routing/routing_session.cpp @@ -49,25 +49,6 @@ uint32_t constexpr kMinimumETASec = 60; namespace routing { -void TrafficCache::Set(TrafficInfo && info) -{ - MwmSet::MwmId const mwmId = info.GetMwmId(); - m_trafficInfo[mwmId] = make_shared(move(info)); -} - -void TrafficCache::Remove(MwmSet::MwmId const & mwmId) { m_trafficInfo.erase(mwmId); } - -shared_ptr TrafficCache::Get(MwmSet::MwmId const & mwmId) const -{ - auto it = m_trafficInfo.find(mwmId); - - if (it == m_trafficInfo.cend()) - return shared_ptr(); - return it->second; -} - -void TrafficCache::Clear() { m_trafficInfo.clear(); } - RoutingSession::RoutingSession() : m_router(nullptr) , m_route(make_shared(string())) diff --git a/routing/routing_session.hpp b/routing/routing_session.hpp index c45572b28c..6e9b2b9a46 100644 --- a/routing/routing_session.hpp +++ b/routing/routing_session.hpp @@ -7,6 +7,7 @@ #include "routing/turns_notification_manager.hpp" #include "traffic/traffic_info.hpp" +#include "traffic/traffic_info_getter.hpp" #include "platform/location.hpp" #include "platform/measurement_utils.hpp" @@ -38,18 +39,6 @@ struct SpeedCameraRestriction SpeedCameraRestriction() : m_index(0), m_maxSpeedKmH(numeric_limits::max()) {} }; -class TrafficCache -{ -public: - void Set(traffic::TrafficInfo && info); - void Remove(MwmSet::MwmId const & mwmId); - shared_ptr Get(MwmSet::MwmId const & mwmId) const; - void Clear(); - -private: - map> m_trafficInfo; -}; - class RoutingSession : public traffic::TrafficObserver, public traffic::TrafficInfoGetter { friend void UnitTest_TestFollowRoutePercentTest(); @@ -203,7 +192,7 @@ private: double GetCompletionPercent() const; private: - TrafficCache m_trafficCache; + traffic::TrafficCache m_trafficCache; unique_ptr m_router; shared_ptr m_route; atomic m_state; diff --git a/routing/routing_tests/applying_traffic_test.cpp b/routing/routing_tests/applying_traffic_test.cpp index 61fdbb2d1f..b58af123a7 100644 --- a/routing/routing_tests/applying_traffic_test.cpp +++ b/routing/routing_tests/applying_traffic_test.cpp @@ -89,11 +89,6 @@ unique_ptr BuildXXGraph(shared_ptr estimator) class TrafficInfoGetterForTesting : public TrafficInfoGetter { public: - TrafficInfoGetterForTesting(TrafficInfo && trafficInfo) - { - m_trafficCache.Set(move(trafficInfo)); - } - // TrafficInfoGetter overrides: shared_ptr GetTrafficInfo(MwmSet::MwmId const & mwmId) const override { @@ -113,7 +108,8 @@ public: void SetEstimator(TrafficInfo::Coloring && coloring) { - m_trafficGetter = make_unique(move(coloring)); + m_trafficGetter = make_unique(); + UpdateTrafficInfo(move(coloring)); m_estimator = EdgeEstimator::CreateForCar(*make_shared()->GetVehicleModel(), *m_trafficGetter); } @@ -122,7 +118,7 @@ public: void UpdateTrafficInfo(TrafficInfo::Coloring && coloring) { - m_trafficGetter->UpdateTrafficInfo(TrafficInfo(move(coloring))); + m_trafficGetter->UpdateTrafficInfo(TrafficInfo::BuildForTesting(move(coloring))); } private: diff --git a/routing/single_mwm_router.cpp b/routing/single_mwm_router.cpp index bb8fa932b7..91a9bf9b14 100644 --- a/routing/single_mwm_router.cpp +++ b/routing/single_mwm_router.cpp @@ -29,6 +29,20 @@ size_t constexpr kMaxRoadCandidates = 6; float constexpr kProgressInterval = 2; uint32_t constexpr kDrawPointsPeriod = 10; +class EstimatorGuard final +{ +public: + EstimatorGuard(MwmSet::MwmId const & mwmId, EdgeEstimator & estimator) : m_estimator(estimator) + { + m_estimator.Start(mwmId); + } + + ~EstimatorGuard() { m_estimator.Finish(); } + +private: + EdgeEstimator & m_estimator; +}; + vector ConvertToJunctions(IndexGraphStarter & starter, vector const & joints) { vector roadPoints; diff --git a/traffic/traffic.pro b/traffic/traffic.pro index ecacd5f855..aaf100a6ad 100644 --- a/traffic/traffic.pro +++ b/traffic/traffic.pro @@ -9,7 +9,9 @@ include($$ROOT_DIR/common.pri) SOURCES += \ speed_groups.cpp \ traffic_info.cpp \ + traffic_info_getter.cpp \ HEADERS += \ speed_groups.hpp \ traffic_info.hpp \ + traffic_info_getter.hpp \ diff --git a/traffic/traffic_info.cpp b/traffic/traffic_info.cpp index f9e235c7a8..e422340ce7 100644 --- a/traffic/traffic_info.cpp +++ b/traffic/traffic_info.cpp @@ -84,6 +84,14 @@ TrafficInfo::TrafficInfo(TrafficInfo && info) , m_currentDataVersion(info.m_currentDataVersion) {} +// static +TrafficInfo TrafficInfo::BuildForTesting(Coloring && coloring) +{ + TrafficInfo info; + info.m_coloring = move(coloring); + return info; +} + bool TrafficInfo::ReceiveTrafficData() { auto const & info = m_mwmId.GetInfo(); diff --git a/traffic/traffic_info.hpp b/traffic/traffic_info.hpp index 1e99417dc7..c4b842161f 100644 --- a/traffic/traffic_info.hpp +++ b/traffic/traffic_info.hpp @@ -70,8 +70,7 @@ public: TrafficInfo(TrafficInfo && info); - // For testing only. - TrafficInfo(Coloring && coloring) : m_coloring(move(coloring)) {} + static TrafficInfo BuildForTesting(Coloring && coloring); // Fetches the latest traffic data from the server and updates the coloring. // Construct the url by passing an MwmId. @@ -106,12 +105,4 @@ public: virtual void OnTrafficInfoAdded(traffic::TrafficInfo && info) = 0; virtual void OnTrafficInfoRemoved(MwmSet::MwmId const & mwmId) = 0; }; - -class TrafficInfoGetter -{ -public: - virtual ~TrafficInfoGetter() = default; - - virtual shared_ptr GetTrafficInfo(MwmSet::MwmId const & mwmId) const = 0; -}; } // namespace traffic diff --git a/traffic/traffic_info_getter.cpp b/traffic/traffic_info_getter.cpp new file mode 100644 index 0000000000..822d7690d7 --- /dev/null +++ b/traffic/traffic_info_getter.cpp @@ -0,0 +1,23 @@ +#include "traffic/traffic_info_getter.hpp" + +namespace traffic +{ +void TrafficCache::Set(TrafficInfo && info) +{ + MwmSet::MwmId const mwmId = info.GetMwmId(); + m_trafficInfo[mwmId] = make_shared(move(info)); +} + +void TrafficCache::Remove(MwmSet::MwmId const & mwmId) { m_trafficInfo.erase(mwmId); } + +shared_ptr TrafficCache::Get(MwmSet::MwmId const & mwmId) const +{ + auto it = m_trafficInfo.find(mwmId); + + if (it == m_trafficInfo.cend()) + return shared_ptr(); + return it->second; +} + +void TrafficCache::Clear() { m_trafficInfo.clear(); } +} // namespace traffic diff --git a/traffic/traffic_info_getter.hpp b/traffic/traffic_info_getter.hpp new file mode 100644 index 0000000000..34933bd900 --- /dev/null +++ b/traffic/traffic_info_getter.hpp @@ -0,0 +1,30 @@ +#pragma once +#include "traffic/traffic_info.hpp" + +#include "indexer/mwm_set.hpp" + +#include "std/map.hpp" +#include "std/shared_ptr.hpp" + +namespace traffic +{ +class TrafficInfoGetter +{ +public: + virtual ~TrafficInfoGetter() = default; + + virtual shared_ptr GetTrafficInfo(MwmSet::MwmId const & mwmId) const = 0; +}; + +class TrafficCache final +{ +public: + void Set(traffic::TrafficInfo && info); + void Remove(MwmSet::MwmId const & mwmId); + shared_ptr Get(MwmSet::MwmId const & mwmId) const; + void Clear(); + +private: + map> m_trafficInfo; +}; +} // namespace traffic