[generator] Fixed duplicated places.

generator/CMakeLists.txt

@@ -127,8 +127,6 @@ set(
   osm_o5m_source.hpp
   osm_source.cpp
   osm_xml_source.hpp
-  place.cpp
-  place.hpp
   place_node.hpp
   place_processor.cpp
   place_processor.hpp

generator/final_processor_intermediate_mwm.cpp

@@ -139,21 +139,18 @@ public:
   static bool IsPlace(FeatureBuilder const & fb)
   {
     auto const type = GetPlaceType(fb);
-    return type != ftype::GetEmptyValue()  && !fb.GetName().empty();
+    return type != ftype::GetEmptyValue()  && !fb.GetName().empty() && NeedProcessPlace(fb);
   }
 
   bool Process(FeatureBuilder const & fb)
   {
-    if (!IsPlace(fb))
-      return false;
-
-    m_processor.TryUpdate(fb);
+    m_processor.Add(fb);
     return true;
   }
 
-  std::vector<FeatureBuilder> GetFeatures() const
+  std::vector<FeatureBuilder> GetFeatures()
   {
-    return m_processor.GetFeatures();
+    return m_processor.ProcessPlaces();
   }
 
   std::shared_ptr<OsmIdToBoundariesTable> GetTable() const

generator/generator_integration_tests/features_tests.cpp

@@ -127,17 +127,17 @@ public:
     rawGenerator.GenerateCountries();
     TEST(rawGenerator.Execute(), ());
 
-    TestCountry(northAuckland, 1811971 /* fbsCnt */, 12195948 /* pointsCnt */, 1007372 /* pointCnt */,
-                205469 /* lineCnt */, 599130 /* areaCnt */, 212087 /* poiCnt */,
+    TestCountry(northAuckland, 1811963 /* fbsCnt */, 12195155 /* pointsCnt */, 1007377 /* pointCnt */,
+                205469 /* lineCnt */, 599117 /* areaCnt */, 212086 /* poiCnt */,
                 521 /* cityTownOrVillageCnt */, 3557 /* bookingHotelsCnt */);
-    TestCountry(northWellington, 797778 /* fbsCnt */, 7772270 /* pointsCnt */, 460446 /* pointCnt */,
-                87058 /* lineCnt */, 250274 /* areaCnt */, 95651 /* poiCnt */,
+    TestCountry(northWellington, 797790 /* fbsCnt */, 7772135 /* pointsCnt */, 460460 /* pointCnt */,
+                87058 /* lineCnt */, 250272 /* areaCnt */, 95650 /* poiCnt */,
                 297 /* cityTownOrVillageCnt */, 1062 /* bookingHotelsCnt */);
-    TestCountry(southCanterbury, 636934 /* fbsCnt */, 6984360 /* pointsCnt */, 397634 /* pointCnt */,
-                81712 /* lineCnt */, 157588 /* areaCnt */, 89249 /* poiCnt */,
+    TestCountry(southCanterbury, 636992 /* fbsCnt */, 6984268 /* pointsCnt */, 397694 /* pointCnt */,
+                81712 /* lineCnt */, 157586 /* areaCnt */, 89249 /* poiCnt */,
                 331 /* cityTownOrVillageCnt */, 2085 /* bookingHotelsCnt */);
-    TestCountry(southSouthland, 340491 /* fbsCnt */, 5342804 /* pointsCnt */, 185845 /* pointCnt */,
-                40124 /* lineCnt */, 114522 /* areaCnt */, 40497 /* poiCnt */,
+    TestCountry(southSouthland, 340492 /* fbsCnt */, 5342793 /* pointsCnt */, 185847 /* pointCnt */,
+                40124 /* lineCnt */, 114521 /* areaCnt */, 40497 /* poiCnt */,
                 297 /* cityTownOrVillageCnt */, 1621 /* bookingHotelsCnt */);
   }
 
@@ -169,7 +169,7 @@ public:
     TestGeneratedFile(metalines, 288032 /* fileSize */);
     TestGeneratedFile(restrictions, 273283 /* fileSize */);
     TestGeneratedFile(roadAccess, 1918315 /* fileSize */);
-    TestGeneratedFile(m_genInfo.m_citiesBoundariesFilename, 2435 /* fileSize */);
+    TestGeneratedFile(m_genInfo.m_citiesBoundariesFilename, 87 /* fileSize */);
   }
 
   void BuildWorldOneThread()

generator/generator_tests/CMakeLists.txt

@@ -28,6 +28,7 @@ set(
   osm2meta_test.cpp
   osm_o5m_source_test.cpp
   osm_type_test.cpp
+  place_processor_tests.cpp
   popularity_builder_tests.cpp
   region_info_collector_tests.cpp
   regions_tests.cpp

generator/place.cpp

@@ -1,75 +0,0 @@
-#include "generator/place.hpp"
-
-#include "indexer/classificator.hpp"
-#include "indexer/ftypes_matcher.hpp"
-
-#include "geometry/mercator.hpp"
-
-using namespace feature;
-
-namespace generator
-{
-Place::Place(FeatureBuilder const & ft, uint32_t type, bool saveParams) :
-  m_ft(ft),
-  m_pt(ft.GetKeyPoint()),
-  m_type(type)
-{
-  using namespace ftypes;
-
-  if (!saveParams)
-    m_ft.SetParams({}/* params */);
-
-  switch (IsLocalityChecker::Instance().GetType(m_type))
-  {
-  case COUNTRY: m_thresholdM = 300000.0; break;
-  case STATE: m_thresholdM = 100000.0; break;
-  case CITY: m_thresholdM = 30000.0; break;
-  case TOWN: m_thresholdM = 20000.0; break;
-  case VILLAGE: m_thresholdM = 10000.0; break;
-  default: m_thresholdM = 10000.0; break;
-  }
-}
-
-m2::RectD Place::GetLimitRect() const
-{
-  return MercatorBounds::RectByCenterXYAndSizeInMeters(m_pt, m_thresholdM);
-}
-
-bool Place::IsEqual(Place const & r) const
-{
-  return (AreTypesEqual(m_type, r.m_type) &&
-          m_ft.GetName() == r.m_ft.GetName() &&
-          (IsPoint() || r.IsPoint()) &&
-          MercatorBounds::DistanceOnEarth(m_pt, r.m_pt) < m_thresholdM);
-}
-
-bool Place::IsBetterThan(Place const & r) const
-{
-  // Check ranks.
-  uint8_t const r1 = m_ft.GetRank();
-  uint8_t const r2 = r.m_ft.GetRank();
-  if (r1 != r2)
-    return r1 > r2;
-
-  // Check types length.
-  // ("place-city-capital-2" is better than "place-city").
-  uint8_t const l1 = ftype::GetLevel(m_type);
-  uint8_t const l2 = ftype::GetLevel(r.m_type);
-  if (l1 != l2)
-    return l1 > l2;
-
-  // Assume that area places has better priority than point places at the very end ...
-  /// @todo It was usefull when place=XXX type has any area fill style.
-  /// Need to review priority logic here (leave the native osm label).
-  return !IsPoint() && r.IsPoint();
-}
-
-bool Place::AreTypesEqual(uint32_t t1, uint32_t t2)
-{
-  // Use 2-arity places comparison for filtering.
-  // ("place-city-capital-2" is equal to "place-city")
-  ftype::TruncValue(t1, 2);
-  ftype::TruncValue(t2, 2);
-  return (t1 == t2);
-}
-}  // namespace generator

generator/place.hpp

@@ -1,33 +0,0 @@
-#pragma once
-
-#include "generator/feature_builder.hpp"
-
-#include "geometry/point2d.hpp"
-#include "geometry/rect2d.hpp"
-
-#include <cstdint>
-
-namespace generator
-{
-/// Used to make a "good" node for a highway graph with OSRM for low zooms.
-class Place
-{
-public:
-  Place(feature::FeatureBuilder const & ft, uint32_t type, bool saveParams = true);
-
-  feature::FeatureBuilder const & GetFeature() const { return m_ft; }
-  m2::RectD GetLimitRect() const;
-  bool IsEqual(Place const & r) const;
-  /// Check whether we need to replace place @r with place @this.
-  bool IsBetterThan(Place const & r) const;
-
-private:
-  bool IsPoint() const { return (m_ft.GetGeomType() == feature::GeomType::Point); }
-  static bool AreTypesEqual(uint32_t t1, uint32_t t2);
-
-  feature::FeatureBuilder m_ft;
-  m2::PointD m_pt;
-  uint32_t m_type;
-  double m_thresholdM;
-};
-}  // namespace generator

generator/place_processor.cpp

@@ -1,69 +1,245 @@
 #include "generator/place_processor.hpp"
 
+#include "generator/feature_maker_base.hpp"
 #include "generator/type_helper.hpp"
 
 #include "indexer/classificator.hpp"
+#include "indexer/ftypes_matcher.hpp"
+
+#include "geometry/mercator.hpp"
 
 #include "base/assert.hpp"
 
+#include <algorithm>
+#include <functional>
+#include <iterator>
+#include <tuple>
+
 using namespace feature;
 
 namespace generator
 {
+uint32_t GetPlaceType(generator::FeaturePlace const & place)
+{
+  return GetPlaceType(place.GetFb());
+}
+}  // namespace generator
+
 namespace
 {
-std::shared_ptr<OsmIdToBoundariesTable>
-GetOrCreateBoundariesTable(std::shared_ptr<OsmIdToBoundariesTable> boundariesTable)
+using namespace generator;
+
+double GetThresholdM(ftypes::Type const & type)
 {
-  return boundariesTable ? boundariesTable : std::make_shared<OsmIdToBoundariesTable>();
+  switch (type)
+  {
+  case ftypes::COUNTRY: return 1000000.0;
+  case ftypes::STATE: return 100000.0;
+  case ftypes::CITY: return 30000.0;
+  case ftypes::TOWN: return 20000.0;
+  case ftypes::VILLAGE: return 5000.0;
+  default: return 10000.0;
+  }
+}
+
+// Returns true if left place is worse than right place.
+template <typename T>
+bool IsWorsePlace(T const & left, T const & right)
+{
+  auto const rankL = left.GetRank();
+  auto const rankR = right.GetRank();
+  auto const levelL = ftype::GetLevel(GetPlaceType(left));
+  auto const levelR = ftype::GetLevel(GetPlaceType(right));
+  auto const langCntL = left.GetMultilangName().CountLangs();
+  auto const langCntR = right.GetMultilangName().CountLangs();
+  auto const isPointL = left.IsPoint();
+  auto const isPointR = right.IsPoint();
+  auto const boxAreaL = left.GetLimitRect().Area();
+  auto const boxAreaR = right.GetLimitRect().Area();
+  return std::tie(rankL, levelL, langCntL, isPointL, boxAreaL) <
+      std::tie(rankR, levelR, langCntR, isPointR, boxAreaR);
+}
+
+template <typename T>
+bool IsTheSamePlace(T const & left, T const & right)
+{
+  if (left.GetName() != right.GetName())
+    return false;
+
+  auto const & localityChecker = ftypes::IsLocalityChecker::Instance();
+  auto const localityL = localityChecker.GetType(GetPlaceType(left));
+  auto const localityR = localityChecker.GetType(GetPlaceType(right));
+  if (localityL != localityR)
+    return false;
+
+  auto const & rectL = left.GetLimitRect();
+  auto const & rectR = right.GetLimitRect();
+  if (rectL.IsIntersect(rectR))
+    return true;
+
+  auto const dist = MercatorBounds::DistanceOnEarth(left.GetKeyPoint(), right.GetKeyPoint());
+  return dist <= GetThresholdM(localityL);
+}
+
+template <typename Iterator>
+Iterator FindGroupOfTheSamePlaces(Iterator start, Iterator end)
+{
+  CHECK(start != end, ());
+  auto next = start;
+  while (++next != end)
+  {
+    if (!IsTheSamePlace(*next, *start))
+      return next;
+
+    start = next;
+  }
+  return end;
 }
 }  // namespace
 
+namespace generator
+{
+bool NeedProcessPlace(feature::FeatureBuilder const & fb)
+{
+  auto const & islandChecker = ftypes::IsIslandChecker::Instance();
+  auto const & localityChecker = ftypes::IsLocalityChecker::Instance();
+  return islandChecker(fb.GetTypes()) || localityChecker.GetType(GetPlaceType(fb)) != ftypes::NONE;
+}
+
+void FeaturePlace::Append(FeatureBuilder const & fb)
+{
+  if (m_fbs.empty() || IsWorsePlace(m_fbs[m_bestIndex], fb))
+    m_bestIndex = m_fbs.size();
+
+  m_fbs.emplace_back(fb);
+  m_limitRect.Add(fb.GetLimitRect());
+}
+
+FeatureBuilder const & FeaturePlace::GetFb() const
+{
+  CHECK_LESS(m_bestIndex, m_fbs.size(), ());
+  return m_fbs[m_bestIndex];
+}
+
+FeaturePlace::FeaturesBuilders const & FeaturePlace::GetFbs() const
+{
+  return m_fbs;
+}
+
+m2::RectD const & FeaturePlace::GetLimitRect() const
+{
+  return m_limitRect;
+}
+
+uint8_t FeaturePlace::GetRank() const
+{
+  return GetFb().GetRank();
+}
+
+std::string FeaturePlace::GetName() const
+{
+  return GetFb().GetName();
+}
+
+m2::PointD FeaturePlace::GetKeyPoint() const
+{
+  return GetFb().GetKeyPoint();
+}
+
+StringUtf8Multilang const & FeaturePlace::GetMultilangName() const
+{
+  return GetFb().GetMultilangName();
+}
+
+bool FeaturePlace::IsPoint() const
+{
+  return GetFb().IsPoint();
+}
+
 PlaceProcessor::PlaceProcessor(std::shared_ptr<OsmIdToBoundariesTable> boundariesTable)
-  : m_boundariesTable(GetOrCreateBoundariesTable(boundariesTable))
+  : m_boundariesTable(boundariesTable) {}
+
+void PlaceProcessor::FillTable(FeaturePlaces::const_iterator start, FeaturePlaces::const_iterator end,
+                               FeaturePlaces::const_iterator best) const
 {
-  ASSERT(m_boundariesTable.get(), ());
+  CHECK(m_boundariesTable, ());
+  base::GeoObjectId lastId;
+  for (auto outerIt = start; outerIt != end; ++outerIt)
+  {
+    auto const & fbs = outerIt->GetFbs();
+    for (auto const & fb : fbs)
+    {
+      if (!(fb.IsArea() && ftypes::IsCityTownOrVillage(fb.GetTypes())))
+        continue;
+
+      auto const id = fb.GetLastOsmId();
+      m_boundariesTable->Append(id, indexer::CityBoundary(fb.GetOuterGeometry()));
+      if (lastId != base::GeoObjectId())
+        m_boundariesTable->Union(id, lastId);
+
+      lastId = id;
+    }
+  }
+
+  if (lastId != base::GeoObjectId())
+    m_boundariesTable->Union(lastId, best->GetFb().GetMostGenericOsmId());
 }
 
-void PlaceProcessor::UnionEqualPlacesIds(Place const & place)
+std::vector<FeatureBuilder> PlaceProcessor::ProcessPlaces()
 {
-  auto const id = place.GetFeature().GetLastOsmId();
-  m_places.ForEachInRect(place.GetLimitRect(), [&](Place const & p) {
-    if (p.IsEqual(place))
-      m_boundariesTable->Union(p.GetFeature().GetLastOsmId(), id);
-  });
+  std::vector<FeatureBuilder> finalPlaces;
+  for (auto & nameToGeoObjectIdToFeaturePlaces : m_nameToPlaces)
+  {
+    std::vector<FeaturePlace> places;
+    places.reserve(nameToGeoObjectIdToFeaturePlaces.second.size());
+    for (auto const & geoObjectIdToFeaturePlaces : nameToGeoObjectIdToFeaturePlaces.second)
+      places.emplace_back(geoObjectIdToFeaturePlaces.second);
+    // Firstly, objects are geometrically sorted.
+    std::sort(std::begin(places), std::end(places), [](auto const & l, auto const & r) {
+      auto const & rectL = l.GetLimitRect();
+      auto const & rectR = r.GetLimitRect();
+      return rectL.Center() < rectR.Center();
+    });
+    // Secondly, a group of similar places is searched for, then a better place is searched in
+    // this group.
+    auto start = std::begin(places);
+    while (start != std::cend(places))
+    {
+      auto end = FindGroupOfTheSamePlaces(start, std::end(places));
+      auto best = std::max_element(start, end, IsWorsePlace<FeaturePlace>);
+      auto bestFb = best->GetFb();
+      auto const & localityChecker = ftypes::IsLocalityChecker::Instance();
+      if (bestFb.IsArea() && localityChecker.GetType(GetPlaceType(bestFb)) != ftypes::NONE)
+      {
+        LOG(LWARNING, (bestFb, "is transforming to point."));
+        TransformAreaToPoint(bestFb);
+      }
+      finalPlaces.emplace_back(std::move(bestFb));
+      if (m_boundariesTable)
+        FillTable(start, end, best);
+
+      start = end;
+    }
+  }
+  return finalPlaces;
 }
 
-std::vector<FeatureBuilder> PlaceProcessor::GetFeatures() const
+// static
+std::string PlaceProcessor::GetKey(FeatureBuilder const & fb)
 {
-  std::vector<FeatureBuilder> result;
-  m_places.ForEach([&result](Place const & p) {
-    result.emplace_back(p.GetFeature());
-  });
-
-  return result;
+  auto type = GetPlaceType(fb);
+  ftype::TruncValue(type, 2);
+  return fb.GetName() + "/" + std::to_string(type);
 }
 
 void PlaceProcessor::Add(FeatureBuilder const & fb)
 {
   auto const type = GetPlaceType(fb);
-  if (type == ftype::GetEmptyValue())
+  if (type == ftype::GetEmptyValue() || !NeedProcessPlace(fb))
     return;
-
-  auto const id = fb.GetLastOsmId();
-  m_boundariesTable->Append(id, indexer::CityBoundary(fb.GetOuterGeometry()));
-  UnionEqualPlacesIds(Place(fb, type, false /* saveParams */));
-}
-
-void PlaceProcessor::TryUpdate(FeatureBuilder const & fb)
-{
-  auto const type = GetPlaceType(fb);
-  Place const place(fb, type);
-  UnionEqualPlacesIds(place);
-  m_places.ReplaceEqualInRect(place, [](Place const & p1, Place const & p2) {
-    return p1.IsEqual(p2);
-  }, [](Place const & p1, Place const & p2) {
-    return p1.IsBetterThan(p2);
-  });
+  // Objects are grouped with the same name and type. This does not guarantee that all objects describe
+  // the same place. The logic for the separation of different places of the same name is
+  // implemented in the function GetPlaces().
+  m_nameToPlaces[GetKey(fb)][fb.GetMostGenericOsmId()].Append(fb);
 }
 }  // namespace generator

generator/place_processor.hpp

@@ -2,29 +2,56 @@
 
 #include "generator/cities_boundaries_builder.hpp"
 #include "generator/feature_builder.hpp"
-#include "generator/place.hpp"
-
-#include "geometry/tree4d.hpp"
 
 #include <memory>
+#include <string>
+#include <unordered_map>
 #include <vector>
 
 namespace generator
 {
+bool NeedProcessPlace(feature::FeatureBuilder const & fb);
+
+// This structure encapsulates work with elements of different types.
+// This means that we can consider a set of polygons of one relation as a single entity.
+class FeaturePlace
+{
+public:
+  using FeaturesBuilders = std::vector<feature::FeatureBuilder>;
+
+  void Append(feature::FeatureBuilder const & fb);
+  feature::FeatureBuilder const & GetFb() const;
+  FeaturesBuilders const & GetFbs() const;
+  m2::RectD const & GetLimitRect() const;
+  uint8_t GetRank() const;
+  std::string GetName() const;
+  m2::PointD GetKeyPoint() const;
+  StringUtf8Multilang const & GetMultilangName() const;
+  bool IsPoint() const;
+
+private:
+  m2::RectD m_limitRect;
+  FeaturesBuilders m_fbs;
+  size_t m_bestIndex;
+};
+
 // The class PlaceProcessor is responsible for the union of boundaries of the places.
 class PlaceProcessor
 {
 public:
-  PlaceProcessor(std::shared_ptr<OsmIdToBoundariesTable> boundariesTable);
+  PlaceProcessor(std::shared_ptr<OsmIdToBoundariesTable> boundariesTable = {});
 
   void Add(feature::FeatureBuilder const & fb);
-  void TryUpdate(feature::FeatureBuilder const & fb);
-  std::vector<feature::FeatureBuilder> GetFeatures() const;
+  std::vector<feature::FeatureBuilder> ProcessPlaces();
 
 private:
-  void UnionEqualPlacesIds(Place const & place);
+  using FeaturePlaces = std::vector<FeaturePlace>;
 
+  static std::string GetKey(feature::FeatureBuilder const & fb);
+  void FillTable(FeaturePlaces::const_iterator start, FeaturePlaces::const_iterator end,
+                 FeaturePlaces::const_iterator best) const;
+
+  std::unordered_map<std::string, std::unordered_map<base::GeoObjectId, FeaturePlace>> m_nameToPlaces;
   std::shared_ptr<OsmIdToBoundariesTable> m_boundariesTable;
-  m4::Tree<Place> m_places;
 };
 } // namespace generator