Node access is now done solely by index

Nodes no longer hold references to their parents and children

Orthtree/include/CGAL/Orthtree.h

@@ -197,7 +197,8 @@ public:
       , m_range(point_range)
       , m_point_map(point_map) {
 
-    m_nodes.reserve(1'000'000); // todo: temporary, for testing
+    // fixme: this can be removed once traversal doesn't require pointer stability
+    m_nodes.reserve(1'000'000);
     m_nodes.emplace_back();
 
     Array bbox_min;
@@ -266,7 +267,11 @@ public:
       , m_point_map(other.m_point_map)
       , m_nodes(std::move(other.m_nodes))
       , m_bbox_min(other.m_bbox_min)
-      , m_side_per_depth(other.m_side_per_depth) {}
+      , m_side_per_depth(other.m_side_per_depth) {
+
+    // todo: makes sure moved-from is still valid. Maybe this shouldn't be necessary.
+    other.m_nodes.emplace_back();
+  }
 
   // Non-necessary but just to be clear on the rule of 5:
 
@@ -471,12 +476,13 @@ public:
     \return a forward input iterator over the nodes of the tree
    */
   template <typename Traversal>
-  Node_range traverse(const Traversal& traversal) const {
+  Node_range traverse(Traversal traversal) const {
 
     const Node* first = traversal.first();
 
-    Node_traversal_method_const next
+    Node_traversal_method_const next = [=](const Node* n) -> const Node* {
-      = [&](const Node* n) -> const Node* { return traversal.next(n); };
+      return traversal.next(n);
+    };
 
     return boost::make_iterator_range(Traversal_iterator<const Node>(first, next),
                                       Traversal_iterator<const Node>());
@@ -485,7 +491,7 @@ public:
   // todo: document this convenience function
   template <typename Traversal>
   Node_range traverse() const {
-    return traverse<Traversal>(Traversal(*this));
+    return traverse<Traversal>({*this});
   }
 
   /*!
@@ -655,24 +661,26 @@ public:
    */
   const Node& parent(const Node& node) const {
     CGAL_precondition (!node.is_root());
-    return *node.m_parent;
+    return m_nodes[node.m_parent_index.get()];
   }
 
   Node& parent(Node& node) {
     CGAL_precondition (!node.is_root());
-    return *node.m_parent;
+    return m_nodes[node.m_parent_index.get()];
   }
 
+  // todo: these types can probably be moved out of Node
   using Children = typename Node::Children;
+  using Children_const = typename Node::Children_const;
 
-  Children& children(Node& node) {
+  Children children(Node& node) {
     CGAL_precondition (!node.is_leaf());
-    return node.m_children.get();
+    return Children{&m_nodes[node.m_children_index.get()], Degree::value};
   }
 
-  const Children& children(const Node& node) const {
+  Children_const children(const Node& node) const {
     CGAL_precondition (!node.is_leaf());
-    return node.m_children.get();
+    return Children_const{&m_nodes[node.m_children_index.get()], Degree::value};
   }
 
   const Node* next_sibling(const Node* n) const {
@@ -770,12 +778,11 @@ public:
     CGAL_precondition (node.is_leaf());
 
     // Split the node to create children
-    using Children = typename Node::Children;
     using Local_coordinates = typename Node::Local_coordinates;
-    for (int index = 0; index < Degree::value; index++) {
+    for (int i = 0; i < Degree::value; i++) {
-      m_nodes.emplace_back(&node, Local_coordinates{index});
+      m_nodes.emplace_back(&node, index(node), Local_coordinates{i});
     }
-    node.m_children = Children{&*(m_nodes.end() - Degree::value), Degree::value}; // todo: temporary, for testing
+    node.m_children_index = m_nodes.size() - Degree::value;
 
     // Find the point to around which the node is split
     Point center = barycenter(node);
@@ -792,31 +799,10 @@ public:
    * Idempotent, un-splitting a leaf node has no effect.
    */
   void unsplit(Node& node) {
-    node.m_children.reset();
+    node.m_children_index.reset();
   }
 
-  // todo: this can be removed when nodes store indices instead of references!
+  // todo: documentation
-  Node deep_copy(const Node& node, Node* parent = nullptr) const {
-
-    Node out;
-
-    out.m_parent = parent;
-    out.m_points = node.m_points;
-    out.m_depth = node.m_depth;
-    out.m_global_coordinates = node.m_global_coordinates;
-
-    if (!node.is_leaf()) {
-      out.m_children = std::make_shared<typename Node::Children>();
-      for (int index = 0; index < Degree::value; index++)
-        (*out.m_children)[index] = deep_copy((*node.m_children)[index], &out);
-    }
-
-    return out;
-  }
-
-
-  // TODO: Document this
-  // TODO: Could this method name be reduced to just "center" ?
   Point barycenter(const Node& node) const {
 
     // Determine the side length of this node
@@ -999,8 +985,8 @@ private: // functions :
     Range_iterator split_point = std::partition(
       begin, end,
       [&](const Range_type& a) -> bool {
-        // This should be done with cartesian iterator but it seems
+        // This should be done with cartesian iterator,
-        // complicated to do efficiently
+        // but it seems complicated to do efficiently
         return (get(m_point_map, a)[int(dimension)] < center[int(dimension)]);
       }
     );

Orthtree/include/CGAL/Orthtree/Node.h

@@ -60,6 +60,7 @@ public:
    * \brief Array for containing the child nodes of this node.
    */
   typedef boost::span<Self, Degree::value> Children;
+  typedef boost::span<const Self, Degree::value> Children_const;
   /// \endcond
 
   /*!
@@ -102,11 +103,12 @@ private:
   /// \endcond
 
   Point_range m_points;
-  Self* m_parent = nullptr; // todo: use optional<reference_wrapper<Self>> instead of Self *
   std::uint8_t m_depth = 0;
   Global_coordinates m_global_coordinates{};
-  boost::optional<Children> m_children{};
 
+  // todo
+  boost::optional<std::size_t> m_parent_index{};
+  boost::optional<std::size_t> m_children_index{};
 
   // Only the Orthtree class has access to the non-default
   // constructor, mutators, etc.
@@ -136,8 +138,10 @@ public:
     \param parent the node containing this one
     \param index this node's relationship to its parent
   */
-  explicit Node(Self* parent, Local_coordinates local_coordinates)
+  explicit Node(Self* parent, boost::optional<std::size_t> parent_index, Local_coordinates local_coordinates) :
-    : m_parent(parent) {
+    m_parent_index(parent_index) {
+
+    // todo: this can be cleaned up; it probably doesn't need to take a reference to the parent
 
     if (parent != nullptr) {
       m_depth = parent->m_depth + 1;
@@ -183,7 +187,7 @@ public:
     \pre `!is_null()`
   */
   bool is_root() const {
-    return m_parent == nullptr;
+    return !m_parent_index.has_value();
   }
 
   /*!
@@ -191,7 +195,7 @@ public:
     \pre `!is_null()`
   */
   bool is_leaf() const {
-    return (!m_children.has_value());
+    return !m_children_index.has_value();
   }
 
   /*!
@@ -270,10 +274,9 @@ public:
    * \return whether the nodes have different topology.
    */
   bool operator==(const Self& rhs) const {
-
     // todo: This is a trivial implementation, maybe it can be set to =default in c++17?
-    return rhs.m_parent == m_parent &&
+    return rhs.m_parent_index == m_parent_index &&
-           //rhs.m_children == m_children && // todo: this might be wrong for deep-copies
+           rhs.m_children_index == m_children_index &&
            rhs.m_points == m_points &&
            rhs.m_depth == m_depth &&
            rhs.m_global_coordinates == m_global_coordinates;

Orthtree/include/CGAL/Orthtree/Traversal_iterator.h

@@ -30,10 +30,9 @@ namespace CGAL {
  *
  * \tparam Value
  */
-template<class Value>
+template <class Value>
-class Traversal_iterator :
+class Traversal_iterator
-        public boost::iterator_facade<Traversal_iterator<Value>, Value, boost::forward_traversal_tag> {
+  : public boost::iterator_facade<Traversal_iterator<Value>, Value, boost::forward_traversal_tag> {
-
 public:
 
   /// \name Types
@@ -44,7 +43,7 @@ public:
    *
    * \todo
    */
-  typedef std::function<Value *(Value *)> Traversal_function;
+  typedef std::function<Value*(Value*)> Traversal_function;
 
   /// @}
 
@@ -68,14 +67,14 @@ public:
    * \param first
    * \param next
    */
-  Traversal_iterator(Value *first, const Traversal_function &next) : m_value(first), m_next(next) {}
+  Traversal_iterator(Value* first, const Traversal_function& next) : m_value(first), m_next(next) {}
 
   /// @}
 
 private:
   friend class boost::iterator_core_access;
 
-  bool equal(Traversal_iterator<Value> const &other) const {
+  bool equal(Traversal_iterator<Value> const& other) const {
     return m_value == other.m_value;
   }
 
@@ -83,13 +82,13 @@ private:
     m_value = m_next(m_value);
   }
 
-  Value &dereference() const {
+  Value& dereference() const {
     return *m_value;
   }
 
 private:
 
-  Value *m_value;
+  Value* m_value;
   Traversal_function m_next;
 };
 }

Orthtree/include/CGAL/Orthtree/Traversals.h

@@ -57,6 +57,8 @@ public:
 
   const Node* next(const Node* n) const {
 
+    if (n == nullptr) return nullptr;
+
     if (n->is_leaf()) {
 
       auto next = m_orthtree.next_sibling(n);