Review fixes

base/stl_helpers.hpp

@@ -442,21 +442,21 @@ struct EnumClassHash
   }
 };
 
-struct RetrieveKey
+struct RetrieveFirst
 {
-    template <typename T>
-    typename T::first_type const &  operator()(T const & keyValue) const
-    {
-        return keyValue.first;
-    }
+  template <typename T>
+  typename T::first_type const & operator()(T const & pair) const
+  {
+    return pair.first;
+  }
 };
 
-struct RetrieveValue
+struct RetrieveSecond
 {
-    template <typename T>
-    typename T::second_type const & operator()(T const & keyValue) const
-    {
-        return keyValue.second;
-    }
+  template <typename T>
+  typename T::second_type const & operator()(T const & pair) const
+  {
+    return pair.second;
+  }
 };
 }  // namespace base

generator/final_processor_intermediate_mwm.cpp

@@ -220,7 +220,7 @@ public:
     std::vector<FeatureBuilder> fbs;
     fbs.reserve(fbsWithIds.size());
     std::transform(std::cbegin(fbsWithIds), std::cend(fbsWithIds),
-                   std::back_inserter(fbs), base::RetrieveKey());
+                   std::back_inserter(fbs), base::RetrieveFirst());
 
     auto const affiliations = GetAffiliations(fbs, m_affiliation, m_threadsCount);
     AppendToCountries(fbs, affiliations, m_temporaryMwmPath, m_threadsCount);

generator/generator_tests/CMakeLists.txt

@@ -9,6 +9,7 @@ set(
   cities_boundaries_checker_tests.cpp
   cities_ids_tests.cpp
   city_roads_tests.cpp
+  cluster_finder_tests.cpp
   coasts_test.cpp
   collector_city_area_tests.cpp
   common.cpp

generator/generator_tests/cluster_finder_tests.cpp

@@ -0,0 +1,146 @@
+#include "testing/testing.hpp"
+
+#include "generator/place_processor.hpp"
+
+#include "geometry/mercator.hpp"
+#include "geometry/point2d.hpp"
+#include "geometry/rect2d.hpp"
+
+#include "base/assert.hpp"
+
+#include <cmath>
+#include <list>
+#include <string>
+#include <vector>
+
+namespace
+{
+using namespace generator;
+
+enum class Type { T1 = 1, T2, T3 };
+
+std::string DebugPrint(Type const & t)
+{
+  return "T" + std::to_string(static_cast<int>(t));
+}
+
+struct NamedPoint
+{
+  NamedPoint(m2::PointD const & point, Type const & type, std::string const & name)
+    : m_point(point), m_type(type), m_name(name) {}
+
+  size_t m_id = m_counter++;
+  m2::PointD m_point;
+  Type m_type;
+  std::string m_name;
+
+private:
+  static size_t m_counter;
+};
+
+size_t NamedPoint::m_counter = 0;
+
+m2::RectD GetLimitRect(NamedPoint const & p)
+{
+   return m2::RectD(p.m_point, p.m_point);
+}
+
+bool operator==(NamedPoint const & left, NamedPoint const & right)
+{
+  return left.m_id == right.m_id;
+}
+
+std::string DebugPrint(NamedPoint const & t)
+{
+  return DebugPrint(t.m_point) + " " + std::to_string(static_cast<int>(t.m_type)) + " " + t.m_name;
+}
+
+auto const getRadiusMFunction = [](NamedPoint const & p) {
+  switch (p.m_type) {
+  case Type::T1: return 4000;
+  case Type::T2: return 8000;
+  case Type::T3: return 16000;
+  };
+  UNREACHABLE();
+};
+
+auto const isSameFunction = [](NamedPoint const & left, NamedPoint const & right) {
+  if (left.m_name != right.m_name)
+    return false;
+
+  if (left.m_type != right.m_type)
+    return false;
+
+  return MercatorBounds::DistanceOnEarth(left.m_point, right.m_point)  < getRadiusMFunction(left);
+};
+
+template <typename T, template<typename, typename> class Container, typename Alloc = std::allocator<T>>
+std::vector<std::vector<T>> GetClusters(
+    Container<T, Alloc> && container,
+    typename ClustersFinder<T, Container, Alloc>::RadiusFunc const & radiusFunc,
+    typename ClustersFinder<T, Container, Alloc>::IsSameFunc const & isSameFunc)
+{
+  return ClustersFinder<T, Container, Alloc>(std::forward<Container<T, Alloc>>(container),
+                                             radiusFunc, isSameFunc).Find();
+}
+
+UNIT_TEST(ClustersFinder_Empty)
+{
+  std::list<NamedPoint> emptyList;
+  TEST_EQUAL(GetClusters(std::move(emptyList), getRadiusMFunction, isSameFunction),
+             std::vector<std::vector<NamedPoint>>(), ());
+}
+
+UNIT_TEST(ClustersFinder_OneElement)
+{
+  NamedPoint p1({0.0, 0.0}, Type::T1, "name");
+  std::list<NamedPoint> l{p1};
+  std::vector<std::vector<NamedPoint>> expected{{p1}};
+  TEST_EQUAL(GetClusters(std::move(l), getRadiusMFunction, isSameFunction), expected, ());
+}
+
+UNIT_TEST(ClustersFinder_TwoElements)
+{
+  NamedPoint p1({0.0, 0.0}, Type::T1, "name");
+  NamedPoint p2({0.0001, 0.0001}, Type::T1, "name");
+  std::list<NamedPoint> l{p1, p2};
+
+  std::vector<std::vector<NamedPoint>> expected{{p1, p2}};
+  TEST_EQUAL(GetClusters(std::move(l), getRadiusMFunction, isSameFunction), expected, ());
+}
+
+UNIT_TEST(ClustersFinder_TwoClusters)
+{
+  {
+    NamedPoint p1({0.0, 0.0}, Type::T1, "name1");
+    NamedPoint p2({0.0001, 0.0001}, Type::T1, "name2");
+    std::list<NamedPoint> l{p1, p2};
+    std::vector<std::vector<NamedPoint>> expected{{p2}, {p1}};
+    TEST_EQUAL(GetClusters(std::move(l), getRadiusMFunction, isSameFunction), expected, ());
+  }
+  {
+    NamedPoint p1({0.0, 0.0}, Type::T1, "name");
+    NamedPoint p2({0.1, 0.1}, Type::T1, "name");
+    std::list<NamedPoint> l{p1, p2};
+
+    std::vector<std::vector<NamedPoint>> expected{{p1}, {p2}};
+    TEST_EQUAL(GetClusters(std::move(l), getRadiusMFunction, isSameFunction), expected, ());
+  }
+}
+
+UNIT_TEST(ClustersFinder_ThreeClusters)
+{
+  NamedPoint p1({0.0, 0.0}, Type::T1, "name");
+  NamedPoint p2({0.0, 0.00001}, Type::T1, "name");
+  NamedPoint p3({0.0001, 0.0000}, Type::T1, "name");
+
+  NamedPoint p11({0.0, 0.0}, Type::T2, "name");
+  NamedPoint p12({0.0, 0.001}, Type::T2, "name");
+  NamedPoint p13({0.001, 0.0000}, Type::T2, "name");
+
+  NamedPoint p21({0.0, 0.0}, Type::T1, "name21");
+  std::list<NamedPoint> l{p1, p2, p3, p11, p12, p13, p21};
+  std::vector<std::vector<NamedPoint>> expected{{p2, p1, p3}, {p11, p13, p12}, {p21}};
+  TEST_EQUAL(GetClusters(std::move(l), getRadiusMFunction, isSameFunction), expected, ());
+}
+}  // namespace

generator/generator_tests/place_processor_tests.cpp

@@ -120,11 +120,11 @@ void Sort(std::vector<generator::PlaceProcessor::PlaceWithIds> & value)
 }
 
 void Test(std::vector<generator::PlaceProcessor::PlaceWithIds> value,
-          std::vector<generator::PlaceProcessor::PlaceWithIds> excpected)
+          std::vector<generator::PlaceProcessor::PlaceWithIds> expected)
 {
   Sort(value);
-  Sort(excpected);
-  TEST_EQUAL(value, excpected, ());
+  Sort(expected);
+  TEST_EQUAL(value, expected, ());
 }
 }  // namespace
 

generator/place_processor.cpp

@@ -6,12 +6,9 @@
 #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>
 
@@ -29,7 +26,7 @@ namespace
 {
 using namespace generator;
 
-double GetThresholdM(ftypes::Type const & type)
+double GetRadiusM(ftypes::Type const & type)
 {
   switch (type)
   {
@@ -78,18 +75,19 @@ bool IsTheSamePlace(T const & left, T const & right)
     return true;
 
   auto const dist = MercatorBounds::DistanceOnEarth(left.GetKeyPoint(), right.GetKeyPoint());
-  return dist <= GetThresholdM(localityL);
+  return dist <= GetRadiusM(localityL);
 }
 
 std::vector<std::vector<FeaturePlace>> FindClusters(std::vector<FeaturePlace> && places)
 {
-  auto func = [](FeaturePlace const & fp) {
+  auto const func = [](FeaturePlace const & fp) {
     auto const & localityChecker = ftypes::IsLocalityChecker::Instance();
     auto const locality = localityChecker.GetType(GetPlaceType(fp.GetFb()));
-    return GetThresholdM(locality);
+    return GetRadiusM(locality);
   };
-  ClustersFinder<FeaturePlace, std::vector> f(std::move(places), func, IsTheSamePlace<FeaturePlace>);
-  return f.Find();
+  ClustersFinder<FeaturePlace, std::vector> finder(std::move(places), func,
+                                                   IsTheSamePlace<FeaturePlace>);
+  return finder.Find();
 }
 }  // namespace
 
@@ -157,6 +155,11 @@ bool FeaturePlace::IsPoint() const
   return GetFb().IsPoint();
 }
 
+m2::RectD GetLimitRect(FeaturePlace const & fp)
+{
+  return fp.GetLimitRect();
+}
+
 PlaceProcessor::PlaceProcessor(std::shared_ptr<OsmIdToBoundariesTable> boundariesTable)
   : m_boundariesTable(boundariesTable) {}
 

generator/place_processor.hpp

@@ -14,26 +14,30 @@
 #include <set>
 #include <string>
 #include <unordered_map>
+#include <utility>
 #include <vector>
 
 namespace generator
 {
+// The class ClustersFinder finds clusters of objects for which IsSameFunc will return the true.
+// RadiusFunc should return the same radius for all objects in one cluster.
 template <typename T, template<typename, typename> class Container, typename Alloc = std::allocator<T>>
 class ClustersFinder
 {
 public:
-  using DistanceFunc = std::function<double(T const &)>;
+  using RadiusFunc = std::function<double(T const &)>;
   using IsSameFunc = std::function<bool(T const &, T const &)>;
   using ConstIterator = T const *;
 
-  ClustersFinder(Container<T, Alloc> && container, DistanceFunc distanceFunc, IsSameFunc isSameFunc)
-    : m_container(std::move(container)), m_distanseFunc(distanceFunc), m_isSameFunc(isSameFunc)
+  ClustersFinder(Container<T, Alloc> && container, RadiusFunc const & radiusFunc,
+                 IsSameFunc const & isSameFunc)
+    : m_container(std::move(container)), m_radiusFunc(radiusFunc), m_isSameFunc(isSameFunc)
   {
     for (auto const & e : m_container)
       m_tree.Add(&e);
   }
 
-  std::vector<std::vector<T>> Find()
+  std::vector<std::vector<T>> Find() const
   {
     std::vector<std::vector<T>> clusters;
     std::set<ConstIterator> unviewed;
@@ -49,10 +53,10 @@ public:
 private:
   struct TraitsDef
   {
-    m2::RectD const LimitRect(ConstIterator const & it) const { return it->GetLimitRect(); }
+    m2::RectD const LimitRect(ConstIterator const & it) const { return GetLimitRect(*it); }
   };
 
-  std::vector<T> FindOneCluster(ConstIterator const & it, std::set<ConstIterator> & unviewed)
+  std::vector<T> FindOneCluster(ConstIterator const & it, std::set<ConstIterator> & unviewed) const
   {
     std::vector<T> cluster{*it};
     std::queue<ConstIterator> queue;
@@ -63,24 +67,24 @@ private:
       auto const current = queue.front();
       queue.pop();
       auto const queryBbox = GetBboxFor(current);
-      m_tree.ForEachInRect(queryBbox, [&](auto const & conditate) {
-        if (current == conditate || unviewed.count(conditate) == 0 || !m_isSameFunc(*current, *conditate))
+      m_tree.ForEachInRect(queryBbox, [&](auto const & candidate) {
+        if (unviewed.count(candidate) == 0 || !m_isSameFunc(*current, *candidate))
           return;
 
-        unviewed.erase(conditate);
-        queue.emplace(conditate);
-        cluster.emplace_back(*conditate);
+        unviewed.erase(candidate);
+        queue.emplace(candidate);
+        cluster.emplace_back(*candidate);
       });
     }
 
     return cluster;
   }
 
-  m2::RectD GetBboxFor(ConstIterator const & it)
+  m2::RectD GetBboxFor(ConstIterator const & it) const
   {
     m2::RectD bbox;
-    it->GetLimitRect().ForEachCorner([&](auto const & p) {
-      auto const dist = m_distanseFunc(*it);
+    auto const dist = m_radiusFunc(*it);
+    GetLimitRect(*it).ForEachCorner([&](auto const & p) {
       bbox.Add(MercatorBounds::RectByCenterXYAndSizeInMeters(p, dist));
     });
 
@@ -88,7 +92,7 @@ private:
   }
 
   Container<T, Alloc> m_container;
-  DistanceFunc m_distanseFunc;
+  RadiusFunc m_radiusFunc;
   IsSameFunc m_isSameFunc;
   m4::Tree<ConstIterator, TraitsDef> m_tree;
 };
@@ -119,6 +123,8 @@ private:
   size_t m_bestIndex;
 };
 
+m2::RectD GetLimitRect(FeaturePlace const & fp);
+
 // The class PlaceProcessor is responsible for the union of boundaries of the places.
 class PlaceProcessor
 {