Fixing memory deallocation by moving it into OcTreeBaseImpl, which no…

…w has the

responsibility of maintaining the tree structure (= "children" member of nodes)

octomap/include/octomap/OcTreeBaseImpl.h

@@ -491,6 +491,9 @@ namespace octomap {
 
     /// recursive call of writeData()
     std::ostream& writeNodesRecurs(const NODE*, std::ostream &s) const;
+
+    /// recursive delete of node and all children (deallocates memory)
+    void deleteNodeRecurs(NODE* node);
 
     /// recursive call of deleteNode()
     bool deleteNodeRecurs(NODE* node, unsigned int depth, unsigned int max_depth, const OcTreeKey& key);

octomap/include/octomap/OcTreeBaseImpl.hxx

@@ -66,10 +66,9 @@ namespace octomap {
 
   template <class NODE,class I>
   OcTreeBaseImpl<NODE,I>::~OcTreeBaseImpl(){
-    if (root)
-      delete root;
-
-    root = NULL;
+    if (root){
+      deleteNodeRecurs(root);
+    }
   }
 
 
@@ -188,7 +187,7 @@ namespace octomap {
   void OcTreeBaseImpl<NODE,I>::deleteNodeChild(NODE* node, unsigned int childIdx){
     assert((childIdx < 8) && (node->children != NULL));
     assert(node->children[childIdx] != NULL);
-    delete static_cast<NODE*>(node->children[childIdx]);
+    delete static_cast<NODE*>(node->children[childIdx]); // TODO delete check if empty
     node->children[childIdx] = NULL;
   }
 
@@ -246,9 +245,9 @@ namespace octomap {
     // set value to children's values (all assumed equal)
     node->copyData(*(getNodeChild(node, 0)));
 
-    // delete children
+    // delete children (known to be leafs at this point!)
     for (unsigned int i=0;i<8;i++) {
-      delete static_cast<NODE*>(node->children[i]);
+      deleteNodeChild(node, i);
     }
     delete[] node->children;
     node->children = NULL;
@@ -481,15 +480,13 @@ namespace octomap {
   template <class NODE,class I>
   void OcTreeBaseImpl<NODE,I>::clear() {
     if (this->root){
-      delete this->root;
-      this->root = NULL;
+      deleteNodeRecurs(root);
       this->tree_size = 0;
       // max extent of tree changed:
       this->size_changed = true;
     }
   }
 
-
   template <class NODE,class I>
   void OcTreeBaseImpl<NODE,I>::prune() {
     if (root == NULL)
@@ -628,6 +625,25 @@ namespace octomap {
     }
     return true;
   }
+
+  template <class NODE,class I>
+  void OcTreeBaseImpl<NODE,I>::deleteNodeRecurs(NODE* node){
+    assert(node);
+    // TODO: maintain tree size?
+
+    if (node->children != NULL) {
+      for (unsigned int i=0; i<8; i++) {
+        if (node->children[i] != NULL)
+          this->deleteNodeRecurs(static_cast<NODE*>(node->children[i])); 
+      }
+      delete[] node->children;
+      node->children = NULL;
+    } // else: node has no children
+
+    delete node;
+    node = NULL;
+  }
+
 
   template <class NODE,class I>
   bool OcTreeBaseImpl<NODE,I>::deleteNodeRecurs(NODE* node, unsigned int depth, unsigned int max_depth, const OcTreeKey& key){
@@ -655,6 +671,7 @@ namespace octomap {
     bool deleteChild = deleteNodeRecurs(getNodeChild(node, pos), depth+1, max_depth, key);
     if (deleteChild){
       // TODO: lazy eval?
+      // TODO delete check depth, what happens to inner nodes with children?
       this->deleteNodeChild(node, pos);
       this->tree_size-=1;
       this->size_changed = true;
@@ -666,7 +683,6 @@ namespace octomap {
     }
     // node did not lose a child, or still has other children
     return false;
-
   }
 
   template <class NODE,class I>

octomap/include/octomap/OcTreeDataNode.h

@@ -73,6 +73,8 @@ namespace octomap {
     /// including node data in "value"
     OcTreeDataNode(const OcTreeDataNode& rhs);
 
+    /// Delete only own members. 
+    /// OcTree maintains tree structure and must have deleted children already
     ~OcTreeDataNode();
 
     /// Copy the payload (data in "value") from rhs into this node
@@ -118,6 +120,7 @@ namespace octomap {
     void allocChildren();
 
     /// pointer to array of children, may be NULL
+    /// @note The tree class manages this pointer, the array, and the memory for it!
     void** children;
     /// stored data (payload)
     T value;

octomap/include/octomap/OcTreeDataNode.hxx

@@ -64,15 +64,9 @@ namespace octomap {
   template <typename T>
   OcTreeDataNode<T>::~OcTreeDataNode()
   {
-    if (children != NULL) {
-      for (unsigned int i=0; i<8; i++) {
-        if (children[i] != NULL)
-          delete static_cast<OcTreeDataNode<T>*>(children[i]); 
-      }
-      // TODO: ensure correct destructor is called for each derived node!
-      delete[] children;
-    }
-
+    // Delete only own members. OcTree maintains tree structure and must have deleted 
+    // children already
+    assert(!hasChildren());
   }
 
   template <typename T>

octomap/valgrind_memcheck.supp

@@ -1,3 +1,10 @@
+# Suppression file for valgrind memcheck on OctoMap. 
+# Suppresses the "leaks" by the static member initializer which cannot be avoided. 
+# 
+# You can run valgrind on an OctoMap binary (e.g. a unit test) with:
+# valgrind --tool=memcheck --leak-check=full --suppressions=valgrind_memcheck.supp bin/test_....
+#
+
 {
    static_member_init_std_string
    Memcheck:Leak