From 9879d1808a7da38006d9067089215dec75d7aa6c Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Fri, 22 Jul 2016 10:56:39 +0300 Subject: [PATCH] Review fixes. --- generator/altitude_generator.cpp | 23 ++++---- generator/generator_tests/altitude_test.cpp | 6 +- indexer/altitude_loader.cpp | 65 +++++++++++---------- indexer/altitude_loader.hpp | 14 +++-- indexer/feature_altitude.hpp | 11 ++-- routing/features_road_graph.cpp | 2 +- routing/features_road_graph.hpp | 4 +- 7 files changed, 65 insertions(+), 60 deletions(-) diff --git a/generator/altitude_generator.cpp b/generator/altitude_generator.cpp index 4459173f1b..cf77e5b239 100644 --- a/generator/altitude_generator.cpp +++ b/generator/altitude_generator.cpp @@ -27,22 +27,22 @@ #include "defines.hpp" -#include "3party/succinct/elias_fano.hpp" -#include "3party/succinct/mapper.hpp" -#include "3party/succinct/rs_bit_vector.hpp" - #include "std/algorithm.hpp" #include "std/type_traits.hpp" #include "std/utility.hpp" #include "std/vector.hpp" +#include "3party/succinct/elias_fano.hpp" +#include "3party/succinct/mapper.hpp" +#include "3party/succinct/rs_bit_vector.hpp" + using namespace feature; namespace { using namespace routing; -TAltitudeSectionVersion constexpr kAltitudeSectionVersion = 1; +AltitudeHeader::TAltitudeSectionVersion constexpr kAltitudeSectionVersion = 1; class SrtmGetter : public IAltitudeGetter { @@ -151,7 +151,7 @@ uint32_t GetFileSize(string const & filePath) uint64_t size; if (!my::GetFileSize(filePath, size)) { - LOG(LERROR, (filePath, "size = 0")); + LOG(LERROR, (filePath, "Unable to get file size")); return 0; } @@ -182,8 +182,6 @@ void BuildRoadAltitudes(string const & baseDir, string const & countryName, IAlt Processor processor(altitudeGetter); feature::ForEachFromDat(mwmPath, processor); - processor.SortFeatureAltitudes(); - Processor::TFeatureAltitudes const & featureAltitudes = processor.GetFeatureAltitudes(); if (!processor.HasAltitudeInfo()) { @@ -191,6 +189,9 @@ void BuildRoadAltitudes(string const & baseDir, string const & countryName, IAlt return; } + processor.SortFeatureAltitudes(); + Processor::TFeatureAltitudes const & featureAltitudes = processor.GetFeatureAltitudes(); + // Writing compressed bit vector with features which have altitude information. succinct::rs_bit_vector altitudeAvailability(processor.GetAltitudeAvailability()); string const altitudeAvailabilityPath = my::JoinFoldersToPath(baseDir, "altitude_availability.bitvector"); @@ -216,7 +217,7 @@ void BuildRoadAltitudes(string const & baseDir, string const & countryName, IAlt // Writing feature altitude offsets. CHECK(is_sorted(offsets.begin(), offsets.end()), ()); CHECK(adjacent_find(offsets.begin(), offsets.end()) == offsets.end(), ()); - LOG(LINFO, ("Max altitude info offset =", offsets.back(), "offsets.size() =", offsets.size())); + LOG(LINFO, ("Max altitude info offset =", offsets.back(), "number of offsets = =", offsets.size())); succinct::elias_fano::elias_fano_builder builder(offsets.back(), offsets.size()); for (uint32_t offset : offsets) builder.push_back(offset); @@ -243,14 +244,14 @@ void BuildRoadAltitudes(string const & baseDir, string const & countryName, IAlt altitudeInfoOffset + sizeof(TAltitudeSectionOffset) /* for altitude info size */); header.Serialize(w); - // Coping parts of altitude sections to mwm. + // Copying parts of altitude sections to mwm. MoveFileToAltitudeSection(altitudeAvailabilityPath, altitudeAvailabilitySize, w); MoveFileToAltitudeSection(featuresTablePath, featuresTableSize, w); MoveFileToAltitudeSection(altitudeInfoPath, altitudeInfoSize, w); } catch (RootException const & e) { - LOG(LERROR, ("An exception happend while creating", ALTITUDES_FILE_TAG, "section. ", e.what())); + LOG(LERROR, ("An exception happened while creating", ALTITUDES_FILE_TAG, "section. ", e.what())); } } diff --git a/generator/generator_tests/altitude_test.cpp b/generator/generator_tests/altitude_test.cpp index 094d65780a..97d69e14b5 100644 --- a/generator/generator_tests/altitude_test.cpp +++ b/generator/generator_tests/altitude_test.cpp @@ -134,7 +134,7 @@ void BuildMwmWithoutAltitude(vector const & roadFeatures, LocalC builder.Add(TestStreet(ConvertTo2DGeom(geom3D), string(), string())); } -void ReadAndTestAltitudeInfo(MwmValue const * mwmValue, string const & mwmPath, MockAltitudeGetter & altitudeGetter) +void ReadAndTestAltitudeInfo(MwmValue const & mwmValue, string const & mwmPath, MockAltitudeGetter & altitudeGetter) { AltitudeLoader const loader(mwmValue); @@ -142,7 +142,7 @@ void ReadAndTestAltitudeInfo(MwmValue const * mwmValue, string const & mwmPath, { f.ParseGeometry(FeatureType::BEST_GEOMETRY); size_t const pointsCount = f.GetPointsCount(); - TAltitudes const altitudes = loader.GetAltitude(id, pointsCount); + TAltitudes const altitudes = loader.GetAltitudes(id, pointsCount); if (!routing::IsRoad(feature::TypesHolder(f))) { @@ -191,7 +191,7 @@ void TestAltitudeSection(vector const & roadFeatures) CHECK(mwmHandle.IsAlive(), ()); string const mwmPath = my::JoinFoldersToPath(testDirFullPath, kTestMwm + DATA_FILE_EXTENSION); - ReadAndTestAltitudeInfo(mwmHandle.GetValue(), mwmPath, altitudeGetter); + ReadAndTestAltitudeInfo(*mwmHandle.GetValue(), mwmPath, altitudeGetter); } } // namespace diff --git a/indexer/altitude_loader.cpp b/indexer/altitude_loader.cpp index c25fef6346..84ffc8a72d 100644 --- a/indexer/altitude_loader.cpp +++ b/indexer/altitude_loader.cpp @@ -12,46 +12,44 @@ namespace { -void ReadBuffer(ReaderSource & rs, vector & buf) +void ReadBuffer(ReaderSource & src, vector & buf) { uint32_t bufSz = 0; - rs.Read(&bufSz, sizeof(bufSz)); - if (bufSz > rs.Size() + rs.Pos()) + src.Read(&bufSz, sizeof(bufSz)); + if (bufSz > src.Size() + src.Pos()) { ASSERT(false, ()); return; } buf.clear(); buf.resize(bufSz); - rs.Read(buf.data(), bufSz); + src.Read(buf.data(), bufSz); } } // namespace namespace feature { -AltitudeLoader::AltitudeLoader(MwmValue const * mwmValue) +AltitudeLoader::AltitudeLoader(MwmValue const & mwmValue) { - if (!mwmValue || mwmValue->GetHeader().GetFormat() < version::Format::v8 ) + if (mwmValue.GetHeader().GetFormat() < version::Format::v8 ) return; - if (!mwmValue->m_cont.IsExist(ALTITUDES_FILE_TAG)) + if (!mwmValue.m_cont.IsExist(ALTITUDES_FILE_TAG)) return; try { - m_reader = make_unique(mwmValue->m_cont.GetReader(ALTITUDES_FILE_TAG)); - ReaderSource rs(*m_reader); - m_header.Deserialize(rs); + m_reader = make_unique(mwmValue.m_cont.GetReader(ALTITUDES_FILE_TAG)); + ReaderSource src(*m_reader); + m_header.Deserialize(src); - // Reading rs_bit_vector with altitude availability information. - ReadBuffer(rs, m_altitudeAvailabilitBuf); - m_altitudeAvailability = make_unique(); - succinct::mapper::map(*m_altitudeAvailability, m_altitudeAvailabilitBuf.data()); + // Reading src_bit_vector with altitude availability information. + ReadBuffer(src, m_altitudeAvailabilitBuf); + succinct::mapper::map(m_altitudeAvailability, m_altitudeAvailabilitBuf.data()); - // Reading table with altitude ofsets for features. - ReadBuffer(rs, m_featureTableBuf); - m_featureTable = make_unique(); - succinct::mapper::map(*m_featureTable, m_featureTableBuf.data()); + // Reading table with altitude offsets for features. + ReadBuffer(src, m_featureTableBuf); + succinct::mapper::map(m_featureTable, m_featureTableBuf.data()); } catch (Reader::OpenException const & e) { @@ -65,24 +63,28 @@ bool AltitudeLoader::IsAvailable() const return m_header.minAltitude != kInvalidAltitude && m_header.altitudeInfoOffset != 0; } -TAltitudes AltitudeLoader::GetAltitude(uint32_t featureId, size_t pointCount) const +TAltitudes const & AltitudeLoader::GetAltitudes(uint32_t featureId, size_t pointCount) const { if (m_header.altitudeInfoOffset == 0) { // The version of mwm is less then version::Format::v8 or there's no altitude section in mwm. - return TAltitudes(); + return m_dummy; } - if (!(*m_altitudeAvailability)[featureId]) + auto const it = m_cache.find(featureId); + if (it != m_cache.end()) + return it->second; + + if (!m_altitudeAvailability[featureId]) { LOG(LINFO, ("Feature featureId =", featureId, "does not contain any altitude information.")); - return TAltitudes(); + return m_cache.insert(make_pair(featureId, TAltitudes())).first->second; } - uint64_t const r = m_altitudeAvailability->rank(featureId); - CHECK_LESS(r, m_altitudeAvailability->size(), (featureId)); - uint64_t const offset = m_featureTable->select(r); - CHECK_LESS_OR_EQUAL(offset, m_featureTable->size(), (featureId)); + uint64_t const r = m_altitudeAvailability.rank(featureId); + CHECK_LESS(r, m_altitudeAvailability.size(), (featureId)); + uint64_t const offset = m_featureTable.select(r); + CHECK_LESS_OR_EQUAL(offset, m_featureTable.size(), (featureId)); uint64_t const altitudeInfoOffsetInSection = m_header.altitudeInfoOffset + offset; CHECK_LESS(altitudeInfoOffsetInSection, m_reader->Size(), ()); @@ -90,17 +92,16 @@ TAltitudes AltitudeLoader::GetAltitude(uint32_t featureId, size_t pointCount) co try { Altitude a; - ReaderSource rs(*m_reader); - rs.Skip(altitudeInfoOffsetInSection); - a.Deserialize(m_header.minAltitude, pointCount, rs); + ReaderSource src(*m_reader); + src.Skip(altitudeInfoOffsetInSection); + a.Deserialize(m_header.minAltitude, pointCount, src); - // @TODO(bykoianko) Considers using move semantic for returned value here. - return a.GetAltitudes(); + return m_cache.insert(make_pair(featureId, a.GetAltitudes())).first->second; } catch (Reader::OpenException const & e) { LOG(LERROR, ("Error while getting mwm data", e.Msg())); - return TAltitudes(); + return m_cache.insert(make_pair(featureId, TAltitudes())).first->second; } } } // namespace feature diff --git a/indexer/altitude_loader.hpp b/indexer/altitude_loader.hpp index cd2d1efbb4..4588e620c9 100644 --- a/indexer/altitude_loader.hpp +++ b/indexer/altitude_loader.hpp @@ -2,27 +2,29 @@ #include "indexer/feature_altitude.hpp" #include "indexer/index.hpp" -#include "3party/succinct/rs_bit_vector.hpp" - #include "std/unique_ptr.hpp" #include "std/vector.hpp" +#include "3party/succinct/rs_bit_vector.hpp" + namespace feature { class AltitudeLoader { public: - explicit AltitudeLoader(MwmValue const * mwmValue); + explicit AltitudeLoader(MwmValue const & mwmValue); - TAltitudes GetAltitude(uint32_t featureId, size_t pointCount) const; + TAltitudes const & GetAltitudes(uint32_t featureId, size_t pointCount) const; bool IsAvailable() const; private: vector m_altitudeAvailabilitBuf; vector m_featureTableBuf; - unique_ptr m_altitudeAvailability; - unique_ptr m_featureTable; + succinct::rs_bit_vector m_altitudeAvailability; + succinct::elias_fano m_featureTable; unique_ptr m_reader; + mutable map m_cache; + TAltitudes const m_dummy; AltitudeHeader m_header; }; } // namespace feature diff --git a/indexer/feature_altitude.hpp b/indexer/feature_altitude.hpp index 52f1032f53..8cb8502501 100644 --- a/indexer/feature_altitude.hpp +++ b/indexer/feature_altitude.hpp @@ -11,13 +11,14 @@ namespace feature { using TAltitude = int16_t; using TAltitudes = vector; -using TAltitudeSectionVersion = uint16_t; using TAltitudeSectionOffset = uint32_t; TAltitude constexpr kInvalidAltitude = numeric_limits::min(); struct AltitudeHeader { + using TAltitudeSectionVersion = uint16_t; + AltitudeHeader() { Reset(); @@ -109,10 +110,10 @@ public: private: /// \note |m_altitudes| is a vector of feature point altitudes. There's two posibilities: - /// * |m_altitudes| is empty. It means no altitude information defines. - /// * size of |m_altitudes| is equal to feature point number. In that case every item of - /// |m_altitudes| defines altitude in meters for every feature point. If altitude is not defined - /// for some feature point corresponding vector items are equel to |kInvalidAltitude| + /// * |m_altitudes| is empty. It means there is no altitude information for this feature. + /// * size of |m_pointAlt| is equal to the number of this feature's points. + /// In this case the i'th element of |m_pointAlt| corresponds to the altitude of the + /// i'th point of the feature and is set to kInvalidAltitude when there is no information about the point. TAltitudes m_altitudes; }; } // namespace feature diff --git a/routing/features_road_graph.cpp b/routing/features_road_graph.cpp index e425508393..1dc3c38a25 100644 --- a/routing/features_road_graph.cpp +++ b/routing/features_road_graph.cpp @@ -268,7 +268,7 @@ void FeaturesRoadGraph::ExtractRoadInfo(FeatureID const & featureId, FeatureType ri.m_speedKMPH = speedKMPH; ft.ParseGeometry(FeatureType::BEST_GEOMETRY); - feature::TAltitudes altitudes = value.altitudeLoader->GetAltitude(featureId.m_index, ft.GetPointsCount()); + feature::TAltitudes altitudes = value.altitudeLoader->GetAltitudes(featureId.m_index, ft.GetPointsCount()); size_t const pointsCount = ft.GetPointsCount(); if (altitudes.size() != pointsCount) diff --git a/routing/features_road_graph.hpp b/routing/features_road_graph.hpp index 819f01ace0..e45495adf3 100644 --- a/routing/features_road_graph.hpp +++ b/routing/features_road_graph.hpp @@ -82,7 +82,7 @@ private: struct Value { Value() = default; - Value(MwmSet::MwmHandle && handle) + explicit Value(MwmSet::MwmHandle && handle) : mwmHandle(move(handle)) { if (!mwmHandle.IsAlive()) @@ -90,7 +90,7 @@ private: ASSERT(false, ()); return; } - altitudeLoader = make_unique(mwmHandle.GetValue()); + altitudeLoader = make_unique(*mwmHandle.GetValue()); } bool IsAlive() const