From 49aeab8d3aded681e6c60e8473a95a9749384948 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20M=C3=B6bius?= <moebius@cs.rwth-aachen.de>
Date: Thu, 22 Feb 2024 14:54:20 +0100
Subject: [PATCH] add_vertex and new_vertex parameters changed to by value

---
 Doc/changelog.docu                    |  5 ++
 src/OpenMesh/Core/Mesh/PolyMeshT.hh   | 17 ++---
 src/Unittests/CMakeLists.txt          |  1 +
 src/Unittests/unittests_new_vertex.cc | 94 +++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 12 deletions(-)
 create mode 100644 src/Unittests/unittests_new_vertex.cc

diff --git a/Doc/changelog.docu b/Doc/changelog.docu
index 55ff765d..266011a1 100644
--- a/Doc/changelog.docu
+++ b/Doc/changelog.docu
@@ -8,6 +8,11 @@
 
 <tr valign=top><td><b>11.0</b> (?/?/?)</td><td>
 
+<b>Core</b>
+<ul>
+<li>Changed add_vertex and new_vertex to copy the given position by value instead of by reference. Avoids crash when copying directly inside mesh on reallocation and most compilers optimize that to be faster.</li>
+</ul>
+
 <b>Documentation</b>
 <ul>
 <li>Update Doxygen config format</li>
diff --git a/src/OpenMesh/Core/Mesh/PolyMeshT.hh b/src/OpenMesh/Core/Mesh/PolyMeshT.hh
index eec74a99..b99c12ed 100644
--- a/src/OpenMesh/Core/Mesh/PolyMeshT.hh
+++ b/src/OpenMesh/Core/Mesh/PolyMeshT.hh
@@ -206,11 +206,8 @@ public:
    *
    * \sa new_vertex(), new_vertex_dirty()
    *
-   * \attention Be careful to not use a reference to a point in the mesh itself here.
-   *            as a resize of the underlying vector might invalidate this reference
-   *            and cause a segfault.  
    */
-  inline SmartVertexHandle new_vertex(const Point& _p)
+  inline SmartVertexHandle new_vertex(const Point _p)
   {
     VertexHandle vh(Kernel::new_vertex());
     this->set_point(vh, _p);
@@ -226,9 +223,9 @@ public:
    * new_vertex(const Point &) saves reallocation and reinitialization of
    * property memory.
    *
-   * \sa new_vertex(const Point &)
+   * \sa new_vertex(const Point )
    */
-  inline SmartVertexHandle new_vertex_dirty(const Point& _p)
+  inline SmartVertexHandle new_vertex_dirty(const Point _p)
   {
     VertexHandle vh(Kernel::new_vertex_dirty());
     this->set_point(vh, _p);
@@ -237,16 +234,12 @@ public:
 
   /** Alias for new_vertex(const Point&).
   *
-  *    \attention Be careful to not use a reference to a point in the mesh itself here.
-  *               as a resize of the underlying vector might invalidate this reference
-  *               and cause a segfault.           
-  *
   */
-  inline SmartVertexHandle add_vertex(const Point& _p)
+  inline SmartVertexHandle add_vertex(const Point _p)
   { return new_vertex(_p); }
 
   /// Alias for new_vertex_dirty().
-  inline SmartVertexHandle add_vertex_dirty(const Point& _p)
+  inline SmartVertexHandle add_vertex_dirty(const Point _p)
   { return make_smart(new_vertex_dirty(_p), this); }
 
   // --- normal vectors ---
diff --git a/src/Unittests/CMakeLists.txt b/src/Unittests/CMakeLists.txt
index 0376baab..0d7fb92d 100644
--- a/src/Unittests/CMakeLists.txt
+++ b/src/Unittests/CMakeLists.txt
@@ -17,6 +17,7 @@ set(UNITTEST_SRC
   unittests_mesh_dual.cc
   unittests_mesh_type.cc
   unittests_mixed_decimater.cc
+  unittests_new_vertex.cc
   unittests_normal_calculations.cc
   unittests_polymesh_collapse.cc
   unittests_polymesh_vec2i.cc
diff --git a/src/Unittests/unittests_new_vertex.cc b/src/Unittests/unittests_new_vertex.cc
new file mode 100644
index 00000000..c8899fe6
--- /dev/null
+++ b/src/Unittests/unittests_new_vertex.cc
@@ -0,0 +1,94 @@
+#include <gtest/gtest.h>
+#include <Unittests/unittests_common.hh>
+#include <iostream>
+
+namespace {
+    
+class OpenMeshNewVertexTriangleMesh : public OpenMeshBase {
+
+    protected:
+
+        // This function is called before each test is run
+        virtual void SetUp() {
+            
+            // Do some initial stuff with the member data here...
+        }
+
+        // This function is called after all tests are through
+        virtual void TearDown() {
+
+            // Do some final stuff with the member data here...
+        }
+
+    // Member already defined in OpenMeshBase
+    //Mesh mesh_;  
+};
+
+class OpenMeshNewVertexPolyMesh : public OpenMeshBasePoly {
+
+    protected:
+
+        // This function is called before each test is run
+        virtual void SetUp() {
+            
+            // Do some initial stuff with the member data here...
+        }
+
+        // This function is called after all tests are through
+        virtual void TearDown() {
+
+            // Do some final stuff with the member data here...
+        }
+
+    // Member already defined in OpenMeshBase
+    //Mesh mesh_;  
+};
+
+/*
+ * ====================================================================
+ * Define tests below
+ * ====================================================================
+ */
+
+/* Takes a vertex position directly from the mesh and readds it as a new vertex position
+ */
+TEST_F(OpenMeshNewVertexTriangleMesh, CopyVertexinsideMeshTriangle) {
+
+  mesh_.clear();
+
+  // Add some vertices
+  Mesh::VertexHandle vhandle[4];
+
+  vhandle[0] = mesh_.add_vertex(Mesh::Point(0, 0, 0));
+  vhandle[1] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(0)));
+  vhandle[2] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(1)));
+  vhandle[3] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(2)));
+
+
+  // Check setup
+  EXPECT_EQ(4u, mesh_.n_vertices() ) << "Wrong number of vertices";
+  EXPECT_EQ(0u, mesh_.n_faces() )    << "Wrong number of faces";
+
+}
+
+/* Takes a vertex position directly from the mesh and readds it as a new vertex position
+ */
+TEST_F(OpenMeshNewVertexPolyMesh, CopyVertexinsideMeshPoly) {
+
+  mesh_.clear();
+
+  // Add some vertices
+  Mesh::VertexHandle vhandle[4];
+
+  vhandle[0] = mesh_.add_vertex(Mesh::Point(0, 0, 0));
+  vhandle[1] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(0)));
+  vhandle[2] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(1)));
+  vhandle[3] = mesh_.add_vertex(mesh_.point(Mesh::VertexHandle(2)));
+
+  // Check setup
+  EXPECT_EQ(4u, mesh_.n_vertices() ) << "Wrong number of vertices";
+  EXPECT_EQ(0u, mesh_.n_faces() )    << "Wrong number of faces";
+
+}
+
+}