From 2e84f45b8848e4eb3de42d20902fa7ce3053e641 Mon Sep 17 00:00:00 2001 From: Maksim Andrianov Date: Wed, 26 Sep 2018 13:51:41 +0300 Subject: [PATCH] Review fixes --- base/geo_object_id.hpp | 2 -- generator/CMakeLists.txt | 4 +-- generator/generator_tests/regions_tests.cpp | 2 +- generator/generator_tool/generator_tool.cpp | 4 +-- generator/regions.hpp | 27 --------------------- generator/regions/node.cpp | 6 ++--- generator/regions/node.hpp | 7 +++--- generator/regions/region.cpp | 4 +-- generator/regions/region_base.hpp | 3 ++- generator/regions/region_info_collector.cpp | 22 ++++++++++++----- generator/regions/region_info_collector.hpp | 23 +++++++++++++++--- generator/{ => regions}/regions.cpp | 27 ++++++++++++--------- generator/regions/regions.hpp | 14 +++++++++++ generator/regions/regions_builder.cpp | 17 ++++++------- generator/regions/regions_builder.hpp | 4 ++- generator/regions/to_string_policy.cpp | 3 ++- generator/regions/to_string_policy.hpp | 1 - generator/translator_region.cpp | 2 +- 18 files changed, 93 insertions(+), 79 deletions(-) delete mode 100644 generator/regions.hpp rename generator/{ => regions}/regions.cpp (92%) create mode 100644 generator/regions/regions.hpp diff --git a/base/geo_object_id.hpp b/base/geo_object_id.hpp index 4c6caa8899..8d01f7db0d 100644 --- a/base/geo_object_id.hpp +++ b/base/geo_object_id.hpp @@ -73,8 +73,6 @@ public: // Returns the source type of the object. Type GetType() const; - bool IsValid() const { return m_encodedId != kInvalid; } - bool operator<(GeoObjectId const & other) const { return m_encodedId < other.m_encodedId; } bool operator==(GeoObjectId const & other) const { return m_encodedId == other.m_encodedId; } bool operator!=(GeoObjectId const & other) const { return !(*this == other); } diff --git a/generator/CMakeLists.txt b/generator/CMakeLists.txt index 54ad116575..a2ec7bbe64 100644 --- a/generator/CMakeLists.txt +++ b/generator/CMakeLists.txt @@ -89,8 +89,6 @@ set(SRC relation_tags.hpp region_meta.cpp region_meta.hpp - regions.cpp - regions.hpp regions/city.hpp regions/node.cpp regions/node.hpp @@ -100,6 +98,8 @@ set(SRC regions/region_base.hpp regions/region_info_collector.cpp regions/region_info_collector.hpp + regions/regions.cpp + regions/regions.hpp regions/regions_builder.cpp regions/regions_builder.hpp regions/to_string_policy.cpp diff --git a/generator/generator_tests/regions_tests.cpp b/generator/generator_tests/regions_tests.cpp index b192fbc97a..ce264ad22a 100644 --- a/generator/generator_tests/regions_tests.cpp +++ b/generator/generator_tests/regions_tests.cpp @@ -1,8 +1,8 @@ #include "testing/testing.hpp" #include "generator/osm_element.hpp" -#include "generator/regions.hpp" #include "generator/regions/region_info_collector.hpp" +#include "generator/regions/regions.hpp" #include "generator/regions/regions_builder.hpp" #include "generator/regions/to_string_policy.hpp" diff --git a/generator/generator_tool/generator_tool.cpp b/generator/generator_tool/generator_tool.cpp index aa8e1df3dd..399113ab83 100644 --- a/generator/generator_tool/generator_tool.cpp +++ b/generator/generator_tool/generator_tool.cpp @@ -15,7 +15,7 @@ #include "generator/metalines_builder.hpp" #include "generator/osm_source.hpp" #include "generator/popular_places_section_builder.hpp" -#include "generator/regions.hpp" +#include "generator/regions/regions.hpp" #include "generator/restriction_generator.hpp" #include "generator/road_access_generator.hpp" #include "generator/routing_index_generator.hpp" @@ -397,7 +397,7 @@ int main(int argc, char ** argv) { CHECK(FLAGS_generate_region_features, ("Option --generate_regions_kv can be used only " "together with option --generate_region_features.")); - if (!GenerateRegions(genInfo)) + if (!regions::GenerateRegions(genInfo)) { LOG(LCRITICAL, ("Error generating regions kv.")); return EXIT_FAILURE; diff --git a/generator/regions.hpp b/generator/regions.hpp deleted file mode 100644 index 754508bf5a..0000000000 --- a/generator/regions.hpp +++ /dev/null @@ -1,27 +0,0 @@ -#pragma once - -#include "generator/feature_builder.hpp" -#include "generator/regions/region_info_collector.hpp" - -#include "geometry/rect2d.hpp" - - - -#include "base/geo_object_id.hpp" - -#include -#include -#include -#include - -#include "3party/boost/boost/geometry.hpp" - -namespace feature -{ -struct GenerateInfo; -} // namespace feature - -namespace generator -{ -bool GenerateRegions(feature::GenerateInfo const & genInfo); -} // namespace generator diff --git a/generator/regions/node.cpp b/generator/regions/node.cpp index ac7643a6d9..60284fc21b 100644 --- a/generator/regions/node.cpp +++ b/generator/regions/node.cpp @@ -94,7 +94,7 @@ size_t MaxDepth(Node::Ptr node) void PrintTree(Node::Ptr node, std::ostream & stream = std::cout, std::string prefix = "", bool isTail = true) { - auto const & childern = node->GetChildren(); + auto const & children = node->GetChildren(); stream << prefix; if (isTail) { @@ -115,8 +115,8 @@ void PrintTree(Node::Ptr node, std::ostream & stream = std::cout, std::string pr << ";" << static_cast(d.GetRank()) << ";[" << point.get<0>() << "," << point.get<1>() << "])" << std::endl; - for (size_t i = 0, size = childern.size(); i < size; ++i) - PrintTree(childern[i], stream, prefix, i == size - 1); + for (size_t i = 0, size = children.size(); i < size; ++i) + PrintTree(children[i], stream, prefix, i == size - 1); } void DebugPrintTree(Node::Ptr tree, std::ostream & stream) diff --git a/generator/regions/node.hpp b/generator/regions/node.hpp index c2f399d674..3dd4929c08 100644 --- a/generator/regions/node.hpp +++ b/generator/regions/node.hpp @@ -6,8 +6,6 @@ #include #include -#include "3party/boost/boost/geometry.hpp" - namespace generator { namespace regions @@ -37,13 +35,16 @@ private: }; size_t TreeSize(Node::Ptr node); + size_t MaxDepth(Node::Ptr node); + void DebugPrintTree(Node::Ptr tree, std::ostream & stream = std::cout); + // This function merges two trees if the roots have the same ids. Node::Ptr MergeTree(Node::Ptr l, Node::Ptr r); + // This function corrects the tree. It traverses the whole node and unites children with // the same ids. void NormalizeTree(Node::Ptr tree); - } // namespace regions } // namespace generator diff --git a/generator/regions/region.cpp b/generator/regions/region.cpp index 84cd40f348..833a817f5d 100644 --- a/generator/regions/region.cpp +++ b/generator/regions/region.cpp @@ -8,7 +8,7 @@ #include #include -#include "3party/boost/boost/geometry.hpp" +#include namespace generator { @@ -81,7 +81,7 @@ double Region::CalculateOverlapPercentage(Region const & other) const boost::geometry::intersection(*other.m_polygon, *m_polygon, coll); auto const min = std::min(boost::geometry::area(*other.m_polygon), boost::geometry::area(*m_polygon)); - auto const binOp = [] (double x, BoostPolygon const & y) { return x + boost::geometry::area(y); }; + auto const binOp = [](double x, BoostPolygon const & y) { return x + boost::geometry::area(y); }; auto const sum = std::accumulate(std::begin(coll), std::end(coll), 0., binOp); return (sum / min) * 100; } diff --git a/generator/regions/region_base.hpp b/generator/regions/region_base.hpp index a3e1490480..17641a293b 100644 --- a/generator/regions/region_base.hpp +++ b/generator/regions/region_base.hpp @@ -12,7 +12,7 @@ #include #include -#include "3party/boost/boost/geometry.hpp" +#include namespace generator { @@ -51,6 +51,7 @@ struct RegionWithData base::GeoObjectId GetAdminCenterId() const; bool HasIsoCode() const; std::string GetIsoCode() const; + // Absolute rank values do not mean anything. But if the rank of the first object is more than the // rank of the second object, then the first object is considered more nested. uint8_t GetRank() const; diff --git a/generator/regions/region_info_collector.cpp b/generator/regions/region_info_collector.cpp index 531d5e31bf..1cfa312be1 100644 --- a/generator/regions/region_info_collector.cpp +++ b/generator/regions/region_info_collector.cpp @@ -116,7 +116,7 @@ void RegionInfoCollector::Save(std::string const & filename) } } -RegionDataProxy RegionInfoCollector::Get(base::GeoObjectId const & osmId) const +RegionDataProxy RegionInfoCollector::Get(base::GeoObjectId const & osmId) { return RegionDataProxy(*this, osmId); } @@ -161,7 +161,7 @@ void RegionInfoCollector::FillIsoCode(base::GeoObjectId const & osmId, OsmElemen rd.SetNumeric(el.GetTag("ISO3166-1:numeric")); } -RegionDataProxy::RegionDataProxy(RegionInfoCollector const & regionInfoCollector, +RegionDataProxy::RegionDataProxy(RegionInfoCollector & regionInfoCollector, base::GeoObjectId const & osmId) : m_regionInfoCollector(regionInfoCollector), m_osmId(osmId) @@ -173,6 +173,16 @@ RegionInfoCollector const & RegionDataProxy::GetCollector() const return m_regionInfoCollector; } +RegionInfoCollector & RegionDataProxy::GetCollector() +{ + return m_regionInfoCollector; +} + +RegionInfoCollector::MapRegionData & RegionDataProxy::GetMapRegionData() +{ + return GetCollector().m_mapRegionData; +} + RegionInfoCollector::MapRegionData const & RegionDataProxy::GetMapRegionData() const { @@ -202,12 +212,12 @@ PlaceType RegionDataProxy::GetPlaceType() const void RegionDataProxy::SetAdminLevel(AdminLevel adminLevel) { - const_cast(GetMapRegionData().at(m_osmId).m_adminLevel) = adminLevel; + GetMapRegionData().at(m_osmId).m_adminLevel = adminLevel; } void RegionDataProxy::SetPlaceType(PlaceType placeType) { - const_cast(GetMapRegionData().at(m_osmId).m_place) = placeType; + GetMapRegionData().at(m_osmId).m_place = placeType; } bool RegionDataProxy::HasAdminLevel() const @@ -260,12 +270,12 @@ std::string RegionDataProxy::GetIsoCodeAlphaNumeric() const bool RegionDataProxy::HasAdminCenter() const { return (GetMapRegionData().count(m_osmId) != 0) && - (GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.IsValid()); + (GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.HasId()); } base::GeoObjectId RegionDataProxy::GetAdminCenter() const { - return GetMapRegionData().at(m_osmId).m_osmIdAdminCenter; + return GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.GetId(); } } // namespace regions } // namespace generator diff --git a/generator/regions/region_info_collector.hpp b/generator/regions/region_info_collector.hpp index 56706f82f4..633684bed1 100644 --- a/generator/regions/region_info_collector.hpp +++ b/generator/regions/region_info_collector.hpp @@ -69,7 +69,7 @@ public: explicit RegionInfoCollector(std::string const & filename); explicit RegionInfoCollector(Platform::FilesList const & filenames); void Add(base::GeoObjectId const & osmId, OsmElement const & el); - RegionDataProxy Get(base::GeoObjectId const & osmId) const; + RegionDataProxy Get(base::GeoObjectId const & osmId); void Save(std::string const & filename); private: @@ -98,12 +98,25 @@ private: char m_numeric[4] = {}; }; + struct AdminCenter + { + AdminCenter() : m_has(false) {} + AdminCenter(base::GeoObjectId const & id) : m_has(true), m_id(id) {} + + bool HasId() const { return m_has; } + base::GeoObjectId GetId() const { return m_id; } + + private: + bool m_has; + base::GeoObjectId m_id; + }; + struct RegionData { base::GeoObjectId m_osmId; AdminLevel m_adminLevel = AdminLevel::Unknown; PlaceType m_place = PlaceType::Unknown; - base::GeoObjectId m_osmIdAdminCenter; + AdminCenter m_osmIdAdminCenter; }; using MapRegionData = std::unordered_map; @@ -144,7 +157,7 @@ private: class RegionDataProxy { public: - RegionDataProxy(RegionInfoCollector const & regionInfoCollector, base::GeoObjectId const & osmId); + RegionDataProxy(RegionInfoCollector & regionInfoCollector, base::GeoObjectId const & osmId); base::GeoObjectId const & GetOsmId() const; AdminLevel GetAdminLevel() const; @@ -169,11 +182,13 @@ public: private: bool HasIsoCode() const; + RegionInfoCollector & GetCollector(); RegionInfoCollector const & GetCollector() const; + RegionInfoCollector::MapRegionData & GetMapRegionData(); RegionInfoCollector::MapRegionData const & GetMapRegionData() const; RegionInfoCollector::MapIsoCode const & GetMapIsoCode() const; - std::reference_wrapper m_regionInfoCollector; + std::reference_wrapper m_regionInfoCollector; base::GeoObjectId m_osmId; }; diff --git a/generator/regions.cpp b/generator/regions/regions.cpp similarity index 92% rename from generator/regions.cpp rename to generator/regions/regions.cpp index ad6986dbd3..b1fb6cc71e 100644 --- a/generator/regions.cpp +++ b/generator/regions/regions.cpp @@ -1,4 +1,4 @@ -#include "generator/regions.hpp" +#include "generator/regions/regions.hpp" #include "generator/feature_builder.hpp" #include "generator/generate_info.hpp" @@ -13,6 +13,7 @@ #include "coding/transliteration.hpp" #include "base/logging.hpp" +#include "base/stl_helpers.hpp" #include "base/timer.hpp" #include @@ -24,6 +25,7 @@ #include #include #include +#include #include #include @@ -68,14 +70,14 @@ struct RegionsFixer auto const placeType = adminCenter.GetPlaceType(); if (placeType == PlaceType::Town || placeType == PlaceType::City) { - for (size_t j = i + 1; j < m_regionsWithAdminCenter.size() - 1; ++j) + for (size_t j = i + 1; j + 1 < m_regionsWithAdminCenter.size(); ++j) { if (m_regionsWithAdminCenter[j].ContainsRect(regionWithAdminCenter)) unsuitable[j] = true; } } - if (ExistsRegionAsCity(adminCenter)) + if (RegionExistsAsCity(adminCenter)) continue; regionWithAdminCenter.SetInfo(adminCenter); @@ -88,7 +90,7 @@ struct RegionsFixer } private: - bool ExistsRegionAsCity(const City & city) + bool RegionExistsAsCity(City const & city) { auto const range = m_nameRegionMap.equal_range(city.GetName()); for (auto it = range.first; it != range.second; ++it) @@ -103,11 +105,10 @@ private: void SplitRegionsByAdminCenter() { - auto const pred = [](Region const & region) { return region.GetAdminCenterId().IsValid(); }; + auto const pred = [](Region const & region) { return region.HasAdminCenter(); }; std::copy_if(std::begin(m_regions), std::end(m_regions), std::back_inserter(m_regionsWithAdminCenter), pred); - auto const it = std::remove_if(std::begin(m_regions), std::end(m_regions), pred); - m_regions.erase(it, std::end(m_regions)); + base::EraseIf(m_regions, pred); } void CreateNameRegionMap() @@ -133,7 +134,7 @@ private: }; std::tuple -ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector const & collector) +ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector & collector) { RegionsBuilder::Regions regions; PointCitiesMap pointCitiesMap; @@ -154,7 +155,7 @@ ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector }; feature::ForEachFromDatRawFormat(tmpMwmFilename, toDo); - return std::make_tuple(regions, pointCitiesMap); + return std::make_tuple(std::move(regions), std::move(pointCitiesMap)); } void FilterRegions(RegionsBuilder::Regions & regions) @@ -170,7 +171,7 @@ void FilterRegions(RegionsBuilder::Regions & regions) } RegionsBuilder::Regions ReadData(feature::GenerateInfo const & genInfo, - RegionInfoCollector const & regionsInfoCollector) + RegionInfoCollector & regionsInfoCollector) { RegionsBuilder::Regions regions; PointCitiesMap pointCitiesMap; @@ -180,9 +181,7 @@ RegionsBuilder::Regions ReadData(feature::GenerateInfo const & genInfo, FilterRegions(regions); return std::move(regions); } - } // namespace -} // namespace regions bool GenerateRegions(feature::GenerateInfo const & genInfo) { @@ -207,6 +206,9 @@ bool GenerateRegions(feature::GenerateInfo const & genInfo) for (auto const & countryName : kvBuilder->GetCountryNames()) { auto const tree = kvBuilder->GetNormalizedCountryTree(countryName); + if (!tree) + continue; + if (genInfo.m_verbose) DebugPrintTree(tree); @@ -224,4 +226,5 @@ bool GenerateRegions(feature::GenerateInfo const & genInfo) LOG(LINFO, ("Finish generating regions.", timer.ElapsedSeconds(), "seconds.")); return true; } +} // namespace regions } // namespace generator diff --git a/generator/regions/regions.hpp b/generator/regions/regions.hpp new file mode 100644 index 0000000000..c87a9cc669 --- /dev/null +++ b/generator/regions/regions.hpp @@ -0,0 +1,14 @@ +#pragma once + +namespace feature +{ +struct GenerateInfo; +} // namespace feature + +namespace generator +{ +namespace regions +{ +bool GenerateRegions(feature::GenerateInfo const & genInfo); +} // namespace regions +} // namespace generator diff --git a/generator/regions/regions_builder.cpp b/generator/regions/regions_builder.cpp index 4464dab015..14eedb85be 100644 --- a/generator/regions/regions_builder.cpp +++ b/generator/regions/regions_builder.cpp @@ -1,18 +1,16 @@ #include "generator/regions/regions_builder.hpp" #include "base/assert.hpp" +#include "base/stl_helpers.hpp" #include #include #include #include #include -#include #include -#include #include #include -#include #include "3party/ThreadPool/ThreadPool.h" @@ -29,10 +27,9 @@ RegionsBuilder::RegionsBuilder(Regions && regions, ASSERT(m_toStringPolicy, ()); ASSERT(m_cpuCount != 0, ()); - auto const isCountry = [](Region const & r){ return r.IsCountry(); }; + auto const isCountry = [](Region const & r) { return r.IsCountry(); }; std::copy_if(std::begin(regions), std::end(regions), std::back_inserter(m_countries), isCountry); - auto const it = std::remove_if(std::begin(regions), std::end(regions), isCountry); - regions.erase(it, std::end(regions)); + base::EraseIf(regions, isCountry); auto const cmp = [](Region const & l, Region const & r) { return l.GetArea() > r.GetArea(); }; std::sort(std::begin(m_countries), std::end(m_countries), cmp); @@ -173,14 +170,14 @@ void RegionsBuilder::MakeCountryTrees(Regions const & regions) ThreadPool threadPool(cpuCount); for (auto const & country : GetCountries()) { - auto f = threadPool.enqueue(&RegionsBuilder::BuildCountryRegionTree, country, regions); - results.emplace_back(std::move(f)); + auto result = threadPool.enqueue(&RegionsBuilder::BuildCountryRegionTree, country, regions); + results.emplace_back(std::move(result)); } } - for (auto & r : results) + for (auto & result : results) { - auto tree = r.get(); + auto tree = result.get(); m_countryTrees.emplace(tree->GetData().GetName(), std::move(tree)); } } diff --git a/generator/regions/regions_builder.hpp b/generator/regions/regions_builder.hpp index 69a74b6cdc..f1f66f99ca 100644 --- a/generator/regions/regions_builder.hpp +++ b/generator/regions/regions_builder.hpp @@ -4,7 +4,10 @@ #include "generator/regions/region.hpp" #include "generator/regions/to_string_policy.hpp" +#include #include +#include +#include namespace generator { @@ -42,6 +45,5 @@ private: Regions m_countries; int m_cpuCount; }; - } // namespace regions } // namespace generator diff --git a/generator/regions/to_string_policy.cpp b/generator/regions/to_string_policy.cpp index 6704722e41..4c78739f83 100644 --- a/generator/regions/to_string_policy.cpp +++ b/generator/regions/to_string_policy.cpp @@ -2,8 +2,9 @@ #include +#include + #include "3party/jansson/myjansson.hpp" -#include "3party/boost/boost/range/adaptor/reversed.hpp" namespace generator { diff --git a/generator/regions/to_string_policy.hpp b/generator/regions/to_string_policy.hpp index 76b0908f48..b5ca70a60e 100644 --- a/generator/regions/to_string_policy.hpp +++ b/generator/regions/to_string_policy.hpp @@ -16,7 +16,6 @@ public: virtual std::string ToString(Node::PtrList const & nodePtrList) = 0; }; - class JsonPolicy : public ToStringPolicyInterface { public: diff --git a/generator/translator_region.cpp b/generator/translator_region.cpp index 331a487978..44da42255b 100644 --- a/generator/translator_region.cpp +++ b/generator/translator_region.cpp @@ -66,7 +66,7 @@ bool TranslatorRegion::IsSuitableElement(OsmElement const * p) const auto const & members = p->Members(); auto const pred = [](OsmElement::Member const & m) { return m.role == "admin_centre"; }; - if (t.key == "boundary" && t.value == "police" && + if (t.key == "boundary" && t.value == "political" && std::find_if(std::begin(members), std::end(members), pred) != std::end(members)) return true;