Apply code review changes to make_watertight files

news/PR-0666.rst

@@ -0,0 +1,21 @@
+**Added:** None
+
+**Changed:** None
+
+- Applied changes from PullRequest to make_watertight files
+
+  - remove commented code blocks that are either outdated or are debug statements
+  - improvements to some logic for clarity
+  - use of standard library containers to avoid potential memory leaks in Arc.cpp/Gen.cpp
+  - improvements to struct/variable names
+  - declared variables for "magic numbers"
+  - passing by const reference where possible to avoid unnecessary memory allocation
+  - removed an unused function (Arc::create_loops_from_oriented_edges_fast)
+
+**Deprecated:** None
+
+**Removed:** None
+
+**Fixed:** None
+
+**Security:** None

src/make_watertight/Arc.cpp

@@ -125,7 +125,8 @@ moab::ErrorCode Arc::remove_opposite_pairs_of_edges_fast(moab::Range& edges, con
 
   // populate edge array, used only for searching
   unsigned n_orig_edges = edges.size();
-  edge* my_edges = new edge[n_orig_edges];
+  std::vector<Edge> my_edges(n_orig_edges);
+  // edge* my_edges = new edge[n_orig_edges];
   unsigned j = 0;
   for (moab::Range::const_iterator i = edges.begin(); i != edges.end(); ++i) {
     // get the endpoints of the edge
@@ -151,7 +152,7 @@ moab::ErrorCode Arc::remove_opposite_pairs_of_edges_fast(moab::Range& edges, con
   }
 
   // sort edge array
-  qsort(my_edges, n_orig_edges, sizeof(struct edge), compare_edge);
+  qsort(my_edges.data(), n_orig_edges, sizeof(struct Edge), compare_edge);
 
   // find duplicate edges
   j = 0;
@@ -171,7 +172,6 @@ moab::ErrorCode Arc::remove_opposite_pairs_of_edges_fast(moab::Range& edges, con
       edges = subtract(edges, duplicate_edges);
       rval = MBI()->delete_entities(duplicate_edges);
       if (moab::MB_SUCCESS != rval) {
-        delete[] my_edges;
         MB_CHK_SET_ERR(moab::MB_FAILURE, "cannot delete edge");
       }
       if (debug) {
@@ -183,7 +183,6 @@ moab::ErrorCode Arc::remove_opposite_pairs_of_edges_fast(moab::Range& edges, con
     j = i;
   }
 
-  delete[] my_edges;
   return moab::MB_SUCCESS;
 }
 
@@ -218,7 +217,6 @@ moab::ErrorCode Arc::get_next_oriented_edge(const moab::Range edges,
       std::cout << "result=" << result
                 << " could not get connectivity of edge" << std::endl;
       return result;
-      //print_edge( *i );
     }
     assert(moab::MB_SUCCESS == result);
     assert(2 == n_verts);
@@ -246,37 +244,11 @@ moab::ErrorCode Arc::get_next_oriented_edge(const moab::Range edges,
     std::cout << "get_next_oriented_edge: " << adj_edges.size() <<
               " possible edges indicates a pinch point." << std::endl;
     result = MBI()->list_entity(end_verts[1]);
-    //assert(moab::MB_SUCCESS == result);
-    //return moab::MB_MULTIPLE_ENTITIES_FOUND;
-    next_edge = adj_edges.front();
-  }
-  return moab::MB_SUCCESS;
-}
-
-moab::ErrorCode Arc::create_loops_from_oriented_edges_fast(moab::Range edges,
-                                                           std::vector< std::vector<moab::EntityHandle> >& loops_of_edges,
-                                                           const bool debug) {
-  // place all edges in map
-  std::multimap<moab::EntityHandle, edge> my_edges;
-  moab::ErrorCode rval;
-  for (moab::Range::const_iterator i = edges.begin(); i != edges.end(); ++i) {
-    // get the endpoints of the edge
-    const moab::EntityHandle* endpts;
-    int n_verts;
-    rval = MBI()->get_connectivity(*i, endpts, n_verts);
-    if (moab::MB_SUCCESS != rval || 2 != n_verts) {
-      MB_CHK_SET_ERR(moab::MB_FAILURE, "could not get connectivity");
+    if (result != moab::MB_SUCCESS) {
+      return moab::MB_MULTIPLE_ENTITIES_FOUND;
     }
-    // store the edges
-    edge temp;
-    temp.edge = *i;
-    temp.v0   = endpts[0];
-    temp.v1   = endpts[1];
-    my_edges.insert(std::pair<moab::EntityHandle, edge>(temp.v0, temp));
+    next_edge = adj_edges.front();
   }
-  std::cout << "error: function not complete" << std::endl;
-  return moab::MB_FAILURE;
-
   return moab::MB_SUCCESS;
 }
 
@@ -344,25 +316,22 @@ moab::ErrorCode Arc::create_loops_from_oriented_edges(moab::Range edges,
         return result;
       }
 
-      // if the next edge was found
-      if (0 != next_edge) {
-        // add it to the loop
-        loop_of_edges.push_back(next_edge);
-        if (debug)
-          std::cout << "push_back: " << next_edge << std::endl;
-        n_edges_out++;
-
-        // remove the edge from the possible edges
-        edges.erase(next_edge);
+      // if the next edge was not found, we're done
+      if (0 == next_edge) {
+        break;
+      }
 
-        // set the new reference vertex
-        edge = next_edge;
+      // add it to the loop
+      loop_of_edges.push_back(next_edge);
+      if (debug)
+        std::cout << "push_back: " << next_edge << std::endl;
+      n_edges_out++;
 
-        // if another edge was not found
-      } else {
-        break;
+      // remove the edge from the possible edges
+      edges.erase(next_edge);
 
-      }
+      // set the new reference vertex
+      edge = next_edge;
     }
 
     // check to ensure the arc is closed
@@ -515,9 +484,13 @@ moab::ErrorCode Arc::merge_curves(moab::Range curve_sets, const double facet_tol
     moab::EntityHandle back_endpt  = curve[curve.size() - 1];
     // ADD CODE TO HANDLE SPECIAL CASES!!
     if (front_endpt == back_endpt) { // special case
+      std::cerr << "Warning: Special case hit in merge_curves: ";
       if (0 == gen->length(curve)) { // point curve
+        std::cerr << "point curve";
       } else {                      // circle
+        std::cerr << "circle";
       }
+      std::cerr << std::endl;
     } else {                        // normal curve
       endpoints.insert(front_endpt);
       endpoints.insert(back_endpt);
@@ -527,7 +500,7 @@ moab::ErrorCode Arc::merge_curves(moab::Range curve_sets, const double facet_tol
   // add endpoints to kd-tree. Tree must track ownership to know when verts are
   // merged away (deleted).
 
-  moab::AdaptiveKDTree kdtree(MBI()); //, 0, MESHSET_TRACK_OWNER);
+  moab::AdaptiveKDTree kdtree(MBI());
   moab::EntityHandle root;
 
   //set tree options
@@ -664,7 +637,7 @@ moab::ErrorCode Arc::merge_curves(moab::Range curve_sets, const double facet_tol
         result = zip->merge_verts(curve_i_verts.back(), curve_j_verts.back(),
                                   curve_i_verts, curve_j_verts);
         if (moab::MB_SUCCESS != result)
-          std::cout << result << std::endl;
+          std::cout << "Failed to merge vertices with error: " << result << std::endl;
         assert(moab::MB_SUCCESS == result);
       }
 

src/make_watertight/Arc.hpp

@@ -50,9 +50,6 @@ class Arc {
                                                  moab::EntityHandle& edge_out,
                                                  moab::EntityHandle& vertex_out);
 
-  moab::ErrorCode create_loops_from_oriented_edges_fast(moab::Range edges,
-                                                        std::vector< std::vector<moab::EntityHandle> >& loops_of_edges,
-                                                        const bool debug);
   moab::ErrorCode create_loops_from_oriented_edges(moab::Range edges,
                                                    std::vector< std::vector<moab::EntityHandle> >& loops_of_edges,
                                                    const bool debug);
@@ -73,7 +70,7 @@ class Arc {
 
   /// goes through curve_sets and finds any curves with coincident ( dist. apart <= FACET_TOL) front and back points.
   /// it then merges the curves topologically. Any merged curves aren't deleted until prepare surfaces.
-  moab::ErrorCode merge_curves(moab::Range curve_sets, const double FACET_TOL,
+  moab::ErrorCode merge_curves(moab::Range curve_sets, const double facet_tol,
                                moab::Tag idTag, moab::Tag merge_tag, const bool debug);
 };
 

src/make_watertight/CMakeLists.txt

@@ -1,4 +1,3 @@
-message("")
 
 file(GLOB SRC_FILES "*.cpp")
 file(GLOB PUB_HEADERS "*.hpp")

src/make_watertight/CheckWatertight.cpp

@@ -81,16 +81,6 @@ moab::ErrorCode CheckWatertight::check_mesh_for_watertightness(moab::EntityHandl
   if (moab::MB_SUCCESS != result) { // failed to get edge data
     return result; // failed
   }
-//10/11/13
-//removed as a result of the change from the gen::find_skin function to the moab::Skinner:find_skin function
-  /*
-    result = MBI()->delete_entities( edges ); //otherwise delete all edge
-
-    if(moab::MB_SUCCESS != result) // failed to delete edge data
-      {
-        return result; // failed
-      }
-  */
   // loop over each volume meshset
   int vol_counter = 0;
   for (moab::Range::iterator i = vol_sets.begin(); i != vol_sets.end(); ++i) {
@@ -128,7 +118,6 @@ moab::ErrorCode CheckWatertight::check_mesh_for_watertightness(moab::EntityHandl
     }
 
     // save the edges in a vector that is large enough to avoid resizing
-    // presumably some kind of efficiency thing?? ad ??
     std::vector<coords_and_id> the_coords_and_id;
     the_coords_and_id.reserve(n_tris);
 
@@ -224,20 +213,6 @@ moab::ErrorCode CheckWatertight::check_mesh_for_watertightness(moab::EntityHandl
         temp.matched = false;
         the_coords_and_id.push_back(temp);
       }
-      //10/10/13
-      // Removed the following to avoid altering the data set at all
-      // -No need to delete skin_edges with the moab:Skinner find_skin function
-      // -skin_edges size will be reset to zero upon new moab::Range skin_edges; call
-      // clean up the edges for the next find_skin call
-      //result = MBI()->delete_entities( skin_edges );
-      //if(moab::MB_SUCCESS != result) return result;
-
-      //10/10/13
-      // - No ned to ensure edges aren't in the meshset with moab::Skinner find_skin function
-      //int n_edges;
-      //result = MBI()->get_number_entities_by_type(0, moab::MBEDGE, n_edges );
-      //if(moab::MB_SUCCESS != result) return result;
-      //if(0 != n_edges) MB_CHK_SET_ERR(moab::MB_MULTIPLE_ENTITIES_FOUND,"n_edges not equal to zero");
     }
 
     // sort the edges by the first vert. The first vert has a lower handle than the second.
@@ -271,7 +246,6 @@ moab::ErrorCode CheckWatertight::check_mesh_for_watertightness(moab::EntityHandl
               the_coords_and_id[j].vert2 == the_coords_and_id[k].vert2) {
             the_coords_and_id[j].matched = true;
             the_coords_and_id[k].matched = true;
-            //std::cout<< "matched by handle" << std::endl;
             break;
           }
         } else {
@@ -292,7 +266,6 @@ moab::ErrorCode CheckWatertight::check_mesh_for_watertightness(moab::EntityHandl
           if (d0 < tol * tol && d1 < tol * tol) {
             the_coords_and_id[j].matched = true;
             the_coords_and_id[k].matched = true;
-            //std::cout<< "matched by proximity" << std::endl;
             break;
           }
           // Due to the sort, once the x-coords are out of tolerance, a match

src/make_watertight/CheckWatertight.hpp

@@ -78,26 +78,23 @@ inline int compare_by_coords(const void* a, const void* b) {
             if (ia->z2 == ib->z2) {
               return ia->surf_id - ib->surf_id;
             } else {
-              return (ia->z2 > ib->z2) - (ia->z2 < ib->z2);
+              return (ia->z2 > ib->z2) ? 1 : -1;
             }
           } else {
-            return (ia->y2 > ib->y2) - (ia->y2 < ib->y2);
+            return (ia->y2 > ib->y2) ? 1 : -1;
           }
         } else {
-          return (ia->x2 > ib->x2) - (ia->x2 < ib->x2);
+          return (ia->x2 > ib->x2) ? 1 : -1;
         }
       } else {
-        return (ia->z1 > ib->z1) - (ia->z1 < ib->z1);
+        return (ia->z1 > ib->z1) ? 1 : -1;
       }
     } else {
-      return (ia->y1 > ib->y1) - (ia->y1 < ib->y1);;
+      return (ia->y1 > ib->y1) ? 1 : -1;
     }
   } else {
-    return (ia->x1 > ib->x1) - (ia->x1 < ib->x1);
+    return (ia->x1 > ib->x1) ? 1 : -1;
   }
-  /* float comparison: returns negative if b > a
-     and positive if a > b. We multiplied result by 100.0
-     to preserve decimal fraction */
 }
 
 #endif

src/make_watertight/Cleanup.cpp

@@ -115,7 +115,6 @@ moab::ErrorCode Cleanup::delete_small_edges(const moab::Range& surfaces, const d
 
     // get the skin first, because my find_skin does not check before creating edges.
     moab::Range skin_edges;
-    //result = gen->find_skin( tris, 1, skin_edges, false );
     moab::Skinner tool(MBI());
     result = tool.find_skin(0, tris, 1, skin_edges, false);
     assert(moab::MB_SUCCESS == result);
@@ -152,7 +151,7 @@ moab::ErrorCode Cleanup::delete_small_edges(const moab::Range& surfaces, const d
         MBI()->list_entities(duplicate_edges);
       assert(1 == duplicate_edges.size());
 
-      // if the edge length is less than MERGE_TOL do nothing
+      // if the edge length is less than FACET_TOL do nothing
       if (FACET_TOL < gen->dist_between_verts(endpts[0], endpts[1]))
         continue;
 
@@ -208,11 +207,6 @@ moab::ErrorCode Cleanup::delete_small_edges(const moab::Range& surfaces, const d
       }
       assert(3 <= adj_edges0.size());
       moab::Range adj_skin_edges0 = intersect(adj_edges0, skin_edges);
-      bool endpt0_is_skin;
-      if (adj_skin_edges0.empty())
-        endpt0_is_skin = false;
-      else
-        endpt0_is_skin = true;
 
       moab::Range adj_edges1;
       result = MBI()->get_adjacencies(&endpts[1], 1, 1, true, adj_edges1);
@@ -225,13 +219,13 @@ moab::ErrorCode Cleanup::delete_small_edges(const moab::Range& surfaces, const d
       }
       assert(3 <= adj_edges1.size());
       moab::Range adj_skin_edges1 = intersect(adj_edges1, skin_edges);
-      bool endpt1_is_skin;
-      if (adj_skin_edges1.empty())
-        endpt1_is_skin = false;
-      else
-        endpt1_is_skin = true;
-      if (endpt0_is_skin && endpt1_is_skin)
+
+      // check to see if the endpoints part of the skin
+      bool endpt0_is_skin = !adj_skin_edges0.empty();
+      bool endpt1_is_skin = !adj_skin_edges1.empty();
+      if (endpt0_is_skin && endpt1_is_skin) {
         continue;
+      }
 
       // Keep the skin endpt, and delete the other endpt
       moab::EntityHandle keep_endpt, delete_endpt;