Filter API improvements

CHANGELOG.md

@@ -6,6 +6,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 ### Changed
+- **API BREAKING CHANGE**: API of filters have been changed to be more correct, explicit and flexible.
+  [#21](https://github.com/tzaeschke/phtree-cpp/issues/21)
+  - Correctness: Converters and distance functions are not copied unnecessarily anymore.
+  - Explicit: 
+    Filters *must* have a mandatory parameter for a converter reference. This ensures that the correct
+            converter is used, probably `tree.converter()`.
+  - Flexible: 
+    Distance functions can be provided through a universal reference (forwarding reference).
+    Also, filters are now movable and copyable. 
+
 - **API BREAKING CHANGE**: Allow filtering on buckets in multimaps. Multimap filters have different functions
   and function signatures than normal `PhTree` filters. [#26](https://github.com/tzaeschke/phtree-cpp/issues/26)
 

phtree/common/filter.h

@@ -105,19 +105,16 @@ struct FilterNoOp {
  * The AABB filter can be used to query a point tree for an axis aligned bounding box (AABB).
  * The result is equivalent to that of the 'begin_query(...)' function.
  */
-template <typename CONVERTER = ConverterIEEE<3>>
+template <typename CONVERTER>
 class FilterAABB {
     using KeyExternal = typename CONVERTER::KeyExternal;
     using KeyInternal = typename CONVERTER::KeyInternal;
     using ScalarInternal = typename CONVERTER::ScalarInternal;
-
     static constexpr auto DIM = CONVERTER::DimInternal;
 
   public:
     FilterAABB(
-        const KeyExternal& min_include,
-        const KeyExternal& max_include,
-        CONVERTER converter = CONVERTER())
+        const KeyExternal& min_include, const KeyExternal& max_include, const CONVERTER& converter)
     : min_external_{min_include}
     , max_external_{max_include}
     , min_internal_{converter.pre(min_include)}
@@ -130,13 +127,13 @@ class FilterAABB {
     void set(const KeyExternal& min_include, const KeyExternal& max_include) {
         min_external_ = min_include;
         max_external_ = max_include;
-        min_internal_ = converter_.pre(min_include);
-        max_internal_ = converter_.pre(max_include);
+        min_internal_ = converter_.get().pre(min_include);
+        max_internal_ = converter_.get().pre(max_include);
     }
 
     template <typename T>
     [[nodiscard]] bool IsEntryValid(const KeyInternal& key, const T& /*value*/) const {
-        auto point = converter_.post(key);
+        auto point = converter_.get().post(key);
         for (dimension_t i = 0; i < DIM; ++i) {
             if (point[i] < min_external_[i] || point[i] > max_external_[i]) {
                 return false;
@@ -163,42 +160,39 @@ class FilterAABB {
     }
 
   private:
-    const KeyExternal min_external_;
-    const KeyExternal max_external_;
-    const KeyInternal min_internal_;
-    const KeyInternal max_internal_;
-    const CONVERTER converter_;
+    KeyExternal min_external_;
+    KeyExternal max_external_;
+    KeyInternal min_internal_;
+    KeyInternal max_internal_;
+    std::reference_wrapper<const CONVERTER> converter_;
 };
 
 /*
  * The sphere filter can be used to query a point tree for a sphere.
  */
-template <
-    typename CONVERTER = ConverterIEEE<3>,
-    typename DISTANCE = DistanceEuclidean<CONVERTER::DimInternal>>
+template <typename CONVERTER, typename DISTANCE>
 class FilterSphere {
     using KeyExternal = typename CONVERTER::KeyExternal;
     using KeyInternal = typename CONVERTER::KeyInternal;
     using ScalarInternal = typename CONVERTER::ScalarInternal;
-    using ScalarExternal = typename CONVERTER::ScalarExternal;
-
     static constexpr auto DIM = CONVERTER::DimInternal;
 
   public:
+    template <typename DIST = DistanceEuclidean<CONVERTER::DimExternal>>
     FilterSphere(
         const KeyExternal& center,
-        const ScalarExternal& radius,
-        CONVERTER converter = CONVERTER(),
-        DISTANCE distance_function = DISTANCE())
+        const double radius,
+        const CONVERTER& converter,
+        DIST&& distance_function = DIST())
     : center_external_{center}
     , center_internal_{converter.pre(center)}
     , radius_{radius}
     , converter_{converter}
-    , distance_function_{distance_function} {};
+    , distance_function_(std::forward<DIST>(distance_function)){};
 
     template <typename T>
     [[nodiscard]] bool IsEntryValid(const KeyInternal& key, const T&) const {
-        KeyExternal point = converter_.post(key);
+        KeyExternal point = converter_.get().post(key);
         return distance_function_(center_external_, point) <= radius_;
     }
 
@@ -226,17 +220,23 @@ class FilterSphere {
             closest_in_bounds[i] = std::clamp(center_internal_[i], lo, hi);
         }
 
-        KeyExternal closest_point = converter_.post(closest_in_bounds);
+        KeyExternal closest_point = converter_.get().post(closest_in_bounds);
         return distance_function_(center_external_, closest_point) <= radius_;
     }
 
   private:
-    const KeyExternal center_external_;
-    const KeyInternal center_internal_;
-    const ScalarExternal radius_;
-    const CONVERTER converter_;
-    const DISTANCE distance_function_;
+    KeyExternal center_external_;
+    KeyInternal center_internal_;
+    double radius_;
+    std::reference_wrapper<const CONVERTER> converter_;
+    DISTANCE distance_function_;
 };
+// deduction guide
+template <
+    typename CONV,
+    typename DIST = DistanceEuclidean<CONV::DimExternal>,
+    typename P = typename CONV::KeyExternal>
+FilterSphere(const P&, double, const CONV&, DIST&& fn = DIST()) -> FilterSphere<CONV, DIST>;
 
 /*
  * AABB filter for MultiMaps.
@@ -247,7 +247,7 @@ class FilterMultiMapAABB : public FilterAABB<CONVERTER> {
     using KeyInternal = typename CONVERTER::KeyInternal;
 
   public:
-    FilterMultiMapAABB(const Key& min_include, const Key& max_include, const CONVERTER& converter)
+    FilterMultiMapAABB(const Key& min_include, const Key& max_include, CONVERTER& converter)
     : FilterAABB<CONVERTER>(min_include, max_include, converter){};
 
     template <typename ValueT>
@@ -265,7 +265,7 @@ class FilterMultiMapSphere : public FilterSphere<CONVERTER, DISTANCE> {
     using KeyInternal = typename CONVERTER::KeyInternal;
 
   public:
-    template <typename DIST = DistanceEuclidean<CONVERTER::DimInternal>>
+    template <typename DIST = DistanceEuclidean<CONVERTER::DimExternal>>
     FilterMultiMapSphere(
         const Key& center, double radius, const CONVERTER& converter, DIST&& dist_fn = DIST())
     : FilterSphere<CONVERTER, DIST>(center, radius, converter, std::forward<DIST>(dist_fn)){};
@@ -276,7 +276,7 @@ class FilterMultiMapSphere : public FilterSphere<CONVERTER, DISTANCE> {
     }
 };
 // deduction guide
-template <typename CONV, typename DIST = DistanceEuclidean<CONV::DimInternal>, typename P>
+template <typename CONV, typename DIST = DistanceEuclidean<CONV::DimExternal>, typename P>
 FilterMultiMapSphere(const P&, double, const CONV&, DIST&& fn = DIST())
     -> FilterMultiMapSphere<CONV, DIST>;
 

phtree/common/filter_test.cc

@@ -21,7 +21,8 @@
 using namespace improbable::phtree;
 
 TEST(PhTreeFilterTest, FilterSphereTest) {
-    FilterSphere<ConverterNoOp<2, scalar_64_t>, DistanceEuclidean<2>> filter{{5, 3}, 5};
+    ConverterNoOp<2, scalar_64_t> conv{};
+    FilterSphere filter{{5, 3}, 5, conv, DistanceEuclidean<2>{}};
     // root is always valid
     ASSERT_TRUE(filter.IsNodeValid({0, 0}, 63));
     // valid because node encompasses the circle
@@ -44,8 +45,9 @@ TEST(PhTreeFilterTest, FilterSphereTest) {
     ASSERT_FALSE(filter.IsEntryValid({3, 8}, nullptr));
 }
 
-TEST(PhTreeFilterTest, BoxFilterTest) {
-    FilterAABB<ConverterNoOp<2, scalar_64_t>> filter{{3, 3}, {7, 7}};
+TEST(PhTreeFilterTest, FilterAABBTest) {
+    ConverterNoOp<2, scalar_64_t> conv{};
+    FilterAABB filter{{3, 3}, {7, 7}, conv};
     // root is always valid
     ASSERT_TRUE(filter.IsNodeValid({0, 0}, 63));
     // valid because node encompasses the AABB
@@ -63,4 +65,62 @@ TEST(PhTreeFilterTest, FilterNoOpSmokeTest) {
     auto filter = FilterNoOp();
     ASSERT_TRUE(filter.IsNodeValid<PhPoint<3>>({3, 7, 2}, 10));
     ASSERT_TRUE(filter.IsEntryValid<PhPoint<3>>({3, 7, 2}, 10));
-}
+}
+
+template <typename FILTER>
+void TestAssignability() {
+    ASSERT_TRUE(std::is_copy_constructible_v<FILTER>);
+    ASSERT_TRUE(std::is_copy_assignable_v<FILTER>);
+    ASSERT_TRUE(std::is_move_constructible_v<FILTER>);
+    ASSERT_TRUE(std::is_move_assignable_v<FILTER>);
+}
+
+TEST(PhTreeFilterTest, FilterAssignableTest) {
+    using CONV = ConverterIEEE<3>;
+    using DIST = DistanceEuclidean<3>;
+    TestAssignability<FilterNoOp>();
+    TestAssignability<FilterAABB<CONV>>();
+    TestAssignability<FilterSphere<CONV, DIST>>();
+    TestAssignability<FilterMultiMapAABB<CONV>>();
+    TestAssignability<FilterMultiMapSphere<CONV, DIST>>();
+}
+
+TEST(PhTreeFilterTest, ConverterAssignableTest) {
+    TestAssignability<ConverterIEEE<3>>();
+    TestAssignability<ScalarConverterIEEE>();
+}
+
+class TestConverter : public ConverterMultiply<2, 1, 1> {
+  public:
+    TestConverter() = default;
+
+    TestConverter(const TestConverter&) = delete;
+    TestConverter(TestConverter&&) = delete;
+    TestConverter& operator=(const TestConverter&) = delete;
+    TestConverter& operator=(TestConverter&&) = delete;
+};
+
+TEST(PhTreeFilterTest, ConstructFilterAABBTest) {
+    TestConverter conv;
+    FilterAABB filter1{{3, 3}, {7, 7}, conv};
+    ASSERT_TRUE(filter1.IsNodeValid({0, 0}, 63));
+
+    FilterAABB filter2{{3, 3}, {7, 7}, TestConverter()};
+    ASSERT_TRUE(filter2.IsNodeValid({0, 0}, 63));
+}
+
+TEST(PhTreeFilterTest, ConstructFilterSphereTest) {
+    DistanceL1<2> dist;
+    TestConverter conv;
+    FilterSphere filter1a{{3, 3}, 7, conv};
+    ASSERT_TRUE(filter1a.IsNodeValid({0, 0}, 63));
+    FilterSphere filter1b{{3, 3}, 7, conv, {}};
+    ASSERT_TRUE(filter1b.IsNodeValid({0, 0}, 63));
+    FilterSphere filter1c{{3, 3}, 7, conv, dist};
+    ASSERT_TRUE(filter1c.IsNodeValid({0, 0}, 63));
+    FilterSphere filter1d{{3, 3}, 7, conv, DistanceL1<2>{}};
+    ASSERT_TRUE(filter1d.IsNodeValid({0, 0}, 63));
+
+    FilterSphere filter2{{3, 3}, 7, TestConverter()};
+    ASSERT_TRUE(filter2.IsNodeValid({0, 0}, 63));
+}

phtree/phtree.h

@@ -40,10 +40,11 @@ class PhTree {
         typename std::conditional<(DIM == DimInternal), QueryPoint, QueryIntersect>::type;
 
   public:
+    // Unless specified otherwise this is just PhBox<DIM, SCALAR_EXTERNAL>
     using QueryBox = typename CONVERTER::QueryBoxExternal;
 
-    template <typename CONVERTER2 = CONVERTER>
-    explicit PhTree(CONVERTER2&& converter = CONVERTER())
+    template <typename CONV = CONVERTER>
+    explicit PhTree(CONV&& converter = CONV())
     : tree_{&converter_}, converter_{converter} {}
 
     PhTree(const PhTree& other) = delete;