From 5e9801839ded781213147c8d47792b2d1eab1e01 Mon Sep 17 00:00:00 2001 From: Ahmed Essam Date: Sun, 1 Mar 2020 02:44:34 +0200 Subject: [PATCH] Add move constructor to AABB tree and use smart pointers --- AABB_tree/include/CGAL/AABB_tree.h | 139 ++++++++++++------ .../CGAL/internal/AABB_tree/AABB_node.h | 77 +++------- .../internal/AABB_tree/AABB_search_tree.h | 18 +-- 3 files changed, 122 insertions(+), 112 deletions(-) diff --git a/AABB_tree/include/CGAL/AABB_tree.h b/AABB_tree/include/CGAL/AABB_tree.h index 99dfd89f7f9..52150f28352 100644 --- a/AABB_tree/include/CGAL/AABB_tree.h +++ b/AABB_tree/include/CGAL/AABB_tree.h @@ -13,6 +13,8 @@ #ifndef CGAL_AABB_TREE_H #define CGAL_AABB_TREE_H +#include + #include #include @@ -61,6 +63,8 @@ namespace CGAL { // type of the primitives container typedef std::vector Primitives; + typedef AABB_tree Self; + public: typedef AABBTraits AABB_traits; @@ -111,6 +115,14 @@ namespace CGAL { /// class using `traits`. AABB_tree(const AABBTraits& traits = AABBTraits()); + // move constructor and assignment operator + AABB_tree(Self&&) noexcept; + Self& operator=(Self&&) noexcept; + + // Disabled copy constructor & assignment operator + AABB_tree(const Self&) = delete; + Self& operator=(const Self&) = delete; + /** * @brief Builds the datastructure from a sequence of primitives. * @param first iterator over first primitive to insert @@ -477,10 +489,7 @@ public: // clear nodes void clear_nodes() { - if( size() > 1 ) { - delete [] m_p_root_node; - } - m_p_root_node = nullptr; + m_p_nodes.clear(); } // clears internal KD tree @@ -489,8 +498,7 @@ public: if ( m_search_tree_constructed ) { CGAL_assertion( m_p_search_tree!=nullptr ); - delete m_p_search_tree; - m_p_search_tree = nullptr; + m_p_search_tree.reset(); m_search_tree_constructed = false; } } @@ -516,6 +524,20 @@ public: private: typedef AABB_node Node; + /** + * @brief Builds the tree by recursive expansion. + * @param first the first primitive to insert + * @param last the last primitive to insert + * @param range the number of primitive of the range + * + * [first,last[ is the range of primitives to be added to the tree. + */ + template + void expand(Node& node, + ConstPrimitiveIterator first, + ConstPrimitiveIterator beyond, + const std::size_t range, + const AABBTraits&); public: // returns a point which must be on one primitive @@ -555,8 +577,8 @@ public: AABBTraits m_traits; // set of input primitives Primitives m_primitives; - // single root node - Node* m_p_root_node; + // tree nodes. first node is the root node + std::vector m_p_nodes; #ifdef CGAL_HAS_THREADS mutable CGAL_MUTEX internal_tree_mutex;//mutex used to protect const calls inducing build() mutable CGAL_MUTEX kd_tree_mutex;//mutex used to protect calls to accelerate_distance_queries @@ -572,7 +594,13 @@ public: #endif const_cast< AABB_tree* >(this)->build(); } - return m_p_root_node; + return std::addressof(m_p_nodes[0]); + } + + Node& new_node() + { + m_p_nodes.emplace_back(); + return m_p_nodes.back(); } const Primitive& singleton_data() const { @@ -581,17 +609,11 @@ public: } // search KD-tree - mutable const Search_tree* m_p_search_tree; + mutable std::unique_ptr m_p_search_tree; mutable bool m_search_tree_constructed; mutable bool m_default_search_tree_constructed; // indicates whether the internal kd-tree should be built bool m_need_build; - private: - // Disabled copy constructor & assignment operator - typedef AABB_tree Self; - AABB_tree(const Self& src); - Self& operator=(const Self& src); - }; // end class AABB_tree /// @} @@ -600,13 +622,31 @@ public: AABB_tree::AABB_tree(const Tr& traits) : m_traits(traits) , m_primitives() - , m_p_root_node(nullptr) + , m_p_nodes() , m_p_search_tree(nullptr) , m_search_tree_constructed(false) , m_default_search_tree_constructed(true) , m_need_build(false) {} + template + typename AABB_tree::Self& AABB_tree::operator=(Self&& tree) noexcept + { + m_traits = std::move(tree.traits); + m_primitives = std::exchange(tree.m_primitives, {}); + m_p_nodes = std::exchange(tree.m_p_nodes, {}); + m_p_search_tree = std::exchange(tree.m_p_search_tree, nullptr); + m_search_tree_constructed = std::exchange(tree.m_search_tree_constructed, false); + m_default_search_tree_constructed = std::exchange(tree.m_default_search_tree_constructed, true); + m_need_build = std::exchange(tree.m_need_build, false); + } + + template + AABB_tree::AABB_tree(Self&& tree) noexcept + { + *this = std::move(tree); + } + template template AABB_tree::AABB_tree(ConstPrimitiveIterator first, @@ -614,7 +654,7 @@ public: T&& ... t) : m_traits() , m_primitives() - , m_p_root_node(nullptr) + , m_p_nodes() , m_p_search_tree(nullptr) , m_search_tree_constructed(false) , m_default_search_tree_constructed(true) @@ -670,25 +710,51 @@ public: m_need_build = true; } + template + template + void + AABB_tree::expand(Node& node, + ConstPrimitiveIterator first, + ConstPrimitiveIterator beyond, + const std::size_t range, + const Tr& traits) + { + node.set_bbox(traits.compute_bbox_object()(first, beyond)); + + // sort primitives along longest axis aabb + traits.split_primitives_object()(first, beyond, node.bbox()); + + switch(range) + { + case 2: + node.set_children(*first, *(first+1)); + break; + case 3: + node.set_children(*first, new_node()); + expand(node.right_child(), first+1, beyond, 2, traits); + break; + default: + const std::size_t new_range = range/2; + node.set_children(new_node(), new_node()); + expand(node.left_child(), first, first + new_range, new_range, traits); + expand(node.right_child(), first + new_range, beyond, range - new_range, traits); + } + } + + // Build the data structure, after calls to insert(..) template void AABB_tree::build() { clear_nodes(); + if(m_primitives.size() > 1) { // allocates tree nodes - m_p_root_node = new Node[m_primitives.size()-1](); - if(m_p_root_node == nullptr) - { - std::cerr << "Unable to allocate memory for AABB tree" << std::endl; - CGAL_assertion(m_p_root_node != nullptr); - m_primitives.clear(); - clear(); - } + m_p_nodes.reserve(m_primitives.size()-1); // constructs the tree - m_p_root_node->expand(m_primitives.begin(), m_primitives.end(), + expand(new_node(), m_primitives.begin(), m_primitives.end(), m_primitives.size(), m_traits); } @@ -729,28 +795,19 @@ public: bool AABB_tree::build_kd_tree(ConstPointIterator first, ConstPointIterator beyond) const { - m_p_search_tree = new Search_tree(first, beyond); + m_p_search_tree = std::make_unique(first, beyond); m_default_search_tree_constructed = true; - if(m_p_search_tree != nullptr) - { - m_search_tree_constructed = true; - return true; - } - else - { - std::cerr << "Unable to allocate memory for accelerating distance queries" << std::endl; - return false; - } + m_search_tree_constructed = true; + return true; } template void AABB_tree::do_not_accelerate_distance_queries()const { - clear_search_tree(); - m_default_search_tree_constructed = false; + clear_search_tree(); + m_default_search_tree_constructed = false; } - // constructs the search KD tree from internal primitives template bool AABB_tree::accelerate_distance_queries() const diff --git a/AABB_tree/include/CGAL/internal/AABB_tree/AABB_node.h b/AABB_tree/include/CGAL/internal/AABB_tree/AABB_node.h index 3844166ed50..4e41cec195c 100644 --- a/AABB_tree/include/CGAL/internal/AABB_tree/AABB_node.h +++ b/AABB_tree/include/CGAL/internal/AABB_tree/AABB_node.h @@ -32,6 +32,9 @@ namespace CGAL { template class AABB_node { +private: + typedef AABB_node Self; + public: typedef typename AABBTraits::Bounding_box Bounding_box; @@ -41,27 +44,15 @@ public: , m_p_left_child(nullptr) , m_p_right_child(nullptr) { }; - /// Non virtual Destructor - /// Do not delete children because the tree hosts and delete them - ~AABB_node() { }; + AABB_node(Self&& node) = default; + + // Disabled copy constructor & assignment operator + AABB_node(const Self& src) = delete; + Self& operator=(const Self& src) = delete; /// Returns the bounding box of the node const Bounding_box& bbox() const { return m_bbox; } - /** - * @brief Builds the tree by recursive expansion. - * @param first the first primitive to insert - * @param last the last primitive to insert - * @param range the number of primitive of the range - * - * [first,last[ is the range of primitives to be added to the tree. - */ - template - void expand(ConstPrimitiveIterator first, - ConstPrimitiveIterator beyond, - const std::size_t range, - const AABBTraits&); - /** * @brief General traversal query * @param query the query @@ -93,8 +84,17 @@ public: { return *static_cast(m_p_left_child); } const Primitive& right_data() const { return *static_cast(m_p_right_child); } + template + void set_children(Left&& l, Right&& r) + { + m_p_left_child = static_cast(std::addressof(l)); + m_p_right_child = static_cast(std::addressof(r)); + } + void set_bbox(const Bounding_box& bbox) + { + m_bbox = bbox; + } -private: Node& left_child() { return *static_cast(m_p_left_child); } Node& right_child() { return *static_cast(m_p_right_child); } Primitive& left_data() { return *static_cast(m_p_left_child); } @@ -109,49 +109,8 @@ private: void *m_p_left_child; void *m_p_right_child; -private: - // Disabled copy constructor & assignment operator - typedef AABB_node Self; - AABB_node(const Self& src); - Self& operator=(const Self& src); - }; // end class AABB_node - -template -template -void -AABB_node::expand(ConstPrimitiveIterator first, - ConstPrimitiveIterator beyond, - const std::size_t range, - const Tr& traits) -{ - m_bbox = traits.compute_bbox_object()(first, beyond); - - // sort primitives along longest axis aabb - traits.split_primitives_object()(first, beyond, m_bbox); - - switch(range) - { - case 2: - m_p_left_child = &(*first); - m_p_right_child = &(*(++first)); - break; - case 3: - m_p_left_child = &(*first); - m_p_right_child = static_cast(this)+1; - right_child().expand(first+1, beyond, 2,traits); - break; - default: - const std::size_t new_range = range/2; - m_p_left_child = static_cast(this) + 1; - m_p_right_child = static_cast(this) + new_range; - left_child().expand(first, first + new_range, new_range,traits); - right_child().expand(first + new_range, beyond, range - new_range,traits); - } -} - - template template void diff --git a/AABB_tree/include/CGAL/internal/AABB_tree/AABB_search_tree.h b/AABB_tree/include/CGAL/internal/AABB_tree/AABB_search_tree.h index 96f08168671..f50afaf4f03 100644 --- a/AABB_tree/include/CGAL/internal/AABB_tree/AABB_search_tree.h +++ b/AABB_tree/include/CGAL/internal/AABB_tree/AABB_search_tree.h @@ -13,6 +13,8 @@ #ifndef CGAL_AABB_SEARCH_TREE_H #define CGAL_AABB_SEARCH_TREE_H +#include + #include @@ -83,7 +85,7 @@ namespace CGAL typedef typename CGAL::Orthogonal_k_neighbor_search Neighbor_search; typedef typename Neighbor_search::Tree Tree; private: - Tree* m_p_tree; + std::unique_ptr m_p_tree; Point_and_primitive_id get_p_and_p(const Point_and_primitive_id& p) @@ -104,21 +106,13 @@ namespace CGAL std::vector points; while(begin != beyond) { Point_and_primitive_id pp = get_p_and_p(*begin); - points.push_back(Decorated_point(pp.first,pp.second)); + points.emplace_back(pp.first,pp.second); ++begin; } - m_p_tree = new Tree(points.begin(), points.end()); - if(m_p_tree != nullptr) - m_p_tree->build(); - else - std::cerr << "unable to build the search tree!" << std::endl; + m_p_tree = std::make_unique(points.begin(), points.end()); + m_p_tree->build(); } - ~AABB_search_tree() { - delete m_p_tree; - } - - Point_and_primitive_id closest_point(const Point& query) const { Neighbor_search search(*m_p_tree, query, 1);