Thread safety

[pca006132]

• Added lock for the static variable meshID2Original_ to make manifold copy constructors thread safe, as users may copy manifolds concurrently (I guess gtest do run tests concurrently?). A allMeshID cache is added to cache all mesh IDs referenced in the manifold pImpl_, so we just need to iterate over this set while holding the lock to meshID2Original_, instead of having to iterate over the entire triBary vector while holding the lock.
• Added TBB backend and changed atomicAdd to use c++ atomics instead of OpenMP pragma.

Todo: Is there a way to wrap the vector with a mutex? So we can return meshID2Original_ protected by a mutex in the MeshID2Original function.

[elalish]

I'm hoping there's a simpler way to get thread safety for MeshID2Original. Let's not be too picky about breaking changes; I doubt anyone has used that feature yet anyway.

[pca006132]

Actually regarding MeshID2Original, I am thinking if it is possible to make it reference the old copy using weak pointers or something, and make the ID local to the objects (and their parents). This way we don't need a static variable for storing the IDs, but may be a bit more complicated. The benefit is that there will be no static buffer leaking memory (we never shrink the static buffer), and we don't need a lock for the buffer.

[elalish]

I suppose another option, which I think is in the same vein, is to just add an OriginalMeshID alongside meshID in BaryRef and just skip all of this entirely. Not sure why that didn't occur to me before; I think that would keep everything simpler. What do you think?

[elalish]

Then I think the only thing you'd need for thread safety is an atomic to increment the next meshID when you need one.

[pca006132]

Great, so when we copy A -> B -> C, C's OriginalMeshID should point to A right? Unless we do SetAsOriginal.

[pca006132]

Sorry I was busy with finals recently. I have a bit trouble understanding the purpose of meshID.

manifold/test/mesh_test.cpp

Lines 67 to 72 in a7791d0

	for (int tri = 0; tri < out.NumTri(); ++tri) {
	int meshID = relation.triBary[tri].meshID;
	int meshIdx = meshID2idx.find(meshID) != meshID2idx.end()
	? meshID2idx.at(meshID)
	: meshID2idx.at(meshID2Original[meshID]);
	ASSERT_LT(meshIdx, input.size());

It seems that the original mesh ID used here is the immediate predecessor of the copy? i.e. when we copy A -> B -> C, meshID2Original[meshID] of C would point to B rather than A. Is this intended?

If we follow this behavior, we should make C's OriginalMeshID point to B instead of A. However, if we don't have a static mapping and if B is just an intermediate object that is not accessible by the user, the user will not be able to find out that C comes from A, because the mapping A -> B is only stored in B and is lost. And if we set C's OriginalMeshID to A's mesh ID, we will lose the mapping B -> C.

Also, with the lazy boolean operation feature, the underlying copy constructor of the Manifold::Impl may not be invoked when we copy the manifold, so that will change the behavior of meshID as well.

[elalish]

No problem at all! I got halfway through fixing the bug you found and then got COVID. Hoping to get back to it soon.

You actually had it right before, original points all three way back to the first mesh, unless set as original is called. You get a new mesh id any time the mesh is copied, which includes booleans. It's like an instance id. Perhaps I should have named it that.

[pca006132]

Sorry to hear that, hope that you can recover soon.

Regarding the mesh ID problem, I'm still not sure what is the purpose for unique ID for each copy. It seems to me that the major usage of this ID is to find the original mesh and apply the properties of the original mesh to the copy, which does not need a unique ID for each copy.

[elalish]

Honestly, that's a good point. I had some reasons, but they may have been superceded by the set as original function. I was thinking it would help with finding the intersection between copies, but that can probably be deduced from which verts are original. It's been hard to explain on the documentation, which is probably reason enough to remove it.

[pca006132]

Great, I will just rename meshID to originalMeshID and remove the DuplicateMeshIDs function. This way the code will be simpler, and lazy evaluation now makes more sense as different copies may share the same underlying mesh, it is 'lazier' to avoid having to create a new set of mesh IDs.

[pca006132]

Is the meshID is also used to avoid collapsing edges from different meshes? I tried removing the DuplicateMeshIDs, setting flipping the 30th bit to indicate whether the triangle comes from P or Q and then reset it when we finish the boolean operation, which works for the boolean tests but failed the sample tests.

It seems that if we have to keep the meshID from P and Q distinct even after a boolean operation, because subsequent operations may try to simplify the mesh and will corrupt it if the distinct meshID property is lost. Not sure if this behavior is intended or not. Here is my patch that you can try:

diff --git a/manifold/include/manifold.h b/manifold/include/manifold.h
index 16e0e0b..b101c34 100644
--- a/manifold/include/manifold.h
+++ b/manifold/include/manifold.h
@@ -105,7 +105,6 @@ class Manifold {
   MeshRelation GetMeshRelation() const;
   std::vector<int> GetMeshIDs() const;
   int SetAsOriginal();
-  static std::vector<int> MeshID2Original();
   ///@}
 
   /** @name Modification
@@ -164,4 +163,4 @@ class Manifold {
   static float circularEdgeLength_;
 };
 /** @} */
-}  // namespace manifold
\ No newline at end of file
+}  // namespace manifold
diff --git a/manifold/src/boolean_result.cpp b/manifold/src/boolean_result.cpp
index 807727f..6d302df 100644
--- a/manifold/src/boolean_result.cpp
+++ b/manifold/src/boolean_result.cpp
@@ -442,6 +442,9 @@ struct CreateBarycentric {
     const BaryRef oldRef = halfedgeRef.PQ == 0 ? triBaryP[tri] : triBaryQ[tri];
 
     faceRef[halfedgeR.face] = oldRef;
+    if (halfedgeRef.PQ != 0) {
+      faceRef[halfedgeR.face].meshID |= 1 << 30;
+    }
 
     if (halfedgeR.startVert < firstNewVert) {  // retained vert
       int i = halfedgeRef.vert;
@@ -674,10 +677,18 @@ Manifold::Impl Boolean3::Result(Manifold::OpType op) const {
   Timer simplify;
   simplify.Start();
 
-  outR.DuplicateMeshIDs();
+  outR.meshRelation_.triBary.Dump();
 
   outR.SimplifyTopology();
 
+  thrust::for_each(
+      thrust::device,
+      outR.meshRelation_.triBary.begin(),
+      outR.meshRelation_.triBary.end(),
+      []__host__ __device__(BaryRef& b) {
+        b.meshID &= ~(1 << 30);
+      });
+
   simplify.Stop();
   Timer sort;
   sort.Start();
diff --git a/manifold/src/constructors.cpp b/manifold/src/constructors.cpp
index e0be155..c71c339 100644
--- a/manifold/src/constructors.cpp
+++ b/manifold/src/constructors.cpp
@@ -426,7 +426,6 @@ Manifold Manifold::Compose(const std::vector<Manifold>& manifolds) {
     nextBary += impl.meshRelation_.barycentric.size();
   }
 
-  combined.DuplicateMeshIDs();
   combined.Finish();
   return out;
 }
diff --git a/manifold/src/impl.cpp b/manifold/src/impl.cpp
index d6ca915..a188e52 100644
--- a/manifold/src/impl.cpp
+++ b/manifold/src/impl.cpp
@@ -281,7 +281,7 @@ struct EdgeBox {
 
 namespace manifold {
 
-std::vector<int> Manifold::Impl::meshID2Original_;
+std::atomic<int> Manifold::Impl::meshIDCounter_(1);
 
 /**
  * Create a manifold from an input triangle Mesh. Will throw if the Mesh is not
@@ -355,22 +355,6 @@ Manifold::Impl::Impl(Shape shape) {
   InitializeNewReference();
 }
 
-/**
- * When a manifold is copied, it is given a new unique set of mesh relation IDs,
- * identifying a particular instance of a copied input mesh. The original mesh
- * ID can be found using the meshID2Original mapping.
- */
-void Manifold::Impl::DuplicateMeshIDs() {
-  std::map<int, int> old2new;
-  for (BaryRef& ref : meshRelation_.triBary) {
-    if (old2new.find(ref.meshID) == old2new.end()) {
-      old2new[ref.meshID] = meshID2Original_.size();
-      meshID2Original_.push_back(meshID2Original_[ref.meshID]);
-    }
-    ref.meshID = old2new[ref.meshID];
-  }
-}
-
 void Manifold::Impl::ReinitializeReference(int meshID) {
   thrust::for_each_n(thrust::device, zip(meshRelation_.triBary.begin(), countAt(0)), NumTri(),
                      InitializeBaryRef({meshID, halfedge_.cptrD()}));
@@ -381,8 +365,7 @@ int Manifold::Impl::InitializeNewReference(
     const std::vector<float>& properties,
     const std::vector<float>& propertyTolerance) {
   meshRelation_.triBary.resize(NumTri());
-  const int nextMeshID = meshID2Original_.size();
-  meshID2Original_.push_back(nextMeshID);
+  const int nextMeshID = meshIDCounter_.fetch_add(1);
   ReinitializeReference(nextMeshID);
 
   const int numProps = propertyTolerance.size();
diff --git a/manifold/src/impl.h b/manifold/src/impl.h
index 27c21b1..def5e66 100644
--- a/manifold/src/impl.h
+++ b/manifold/src/impl.h
@@ -40,7 +40,7 @@ struct Manifold::Impl {
   Collider collider_;
   glm::mat4x3 transform_ = glm::mat4x3(1.0f);
 
-  static std::vector<int> meshID2Original_;
+  static std::atomic<int> meshIDCounter_;
 
   Impl() {}
   enum class Shape { TETRAHEDRON, CUBE, OCTAHEDRON };
@@ -56,7 +56,6 @@ struct Manifold::Impl {
       const std::vector<float>& properties = std::vector<float>(),
       const std::vector<float>& propertyTolerance = std::vector<float>());
 
-  void DuplicateMeshIDs();
   void ReinitializeReference(int meshID = -1);
   void CreateHalfedges(const VecDH<glm::ivec3>& triVerts);
   void CreateAndFixHalfedges(const VecDH<glm::ivec3>& triVerts);
diff --git a/manifold/src/manifold.cpp b/manifold/src/manifold.cpp
index 642d2ac..dc04011 100644
--- a/manifold/src/manifold.cpp
+++ b/manifold/src/manifold.cpp
@@ -62,14 +62,11 @@ Manifold::~Manifold() = default;
 Manifold::Manifold(Manifold&&) noexcept = default;
 Manifold& Manifold::operator=(Manifold&&) noexcept = default;
 
-Manifold::Manifold(const Manifold& other) : pImpl_(new Impl(*other.pImpl_)) {
-  pImpl_->DuplicateMeshIDs();
-}
+Manifold::Manifold(const Manifold& other) : pImpl_(new Impl(*other.pImpl_)) {}
 
 Manifold& Manifold::operator=(const Manifold& other) {
   if (this != &other) {
     pImpl_.reset(new Impl(*other.pImpl_));
-    pImpl_->DuplicateMeshIDs();
   }
   return *this;
 }
@@ -320,15 +317,6 @@ int Manifold::SetAsOriginal() {
   return meshID;
 }
 
-/**
- * Returns a vector that maps a given unique MeshID to the MeshID of the
- * original Mesh it came from, to easily identify separate copies of the same
- * thing.
- */
-std::vector<int> Manifold::MeshID2Original() {
-  return Manifold::Impl::meshID2Original_;
-}
-
 /**
  * Should always be true. Also checks saneness of the internal data structures.
  */
diff --git a/manifold/src/sort.cpp b/manifold/src/sort.cpp
index 7a0a67a..69541c6 100644
--- a/manifold/src/sort.cpp
+++ b/manifold/src/sort.cpp
@@ -315,7 +315,6 @@ void Manifold::Impl::GatherFaces(const Impl& old,
                  old.meshRelation_.triBary.begin(),
                  meshRelation_.triBary.begin());
   meshRelation_.barycentric = old.meshRelation_.barycentric;
-  DuplicateMeshIDs();
 
   if (old.faceNormal_.size() == old.NumTri()) {
     faceNormal_.resize(numTri);
diff --git a/test/mesh_test.cpp b/test/mesh_test.cpp
index 7fc0b7e..8213488 100644
--- a/test/mesh_test.cpp
+++ b/test/mesh_test.cpp
@@ -63,12 +63,9 @@ void Related(const Manifold& out, const std::vector<Mesh>& input,
              const std::map<int, int>& meshID2idx) {
   Mesh output = out.GetMesh();
   MeshRelation relation = out.GetMeshRelation();
-  std::vector<int> meshID2Original = Manifold::MeshID2Original();
   for (int tri = 0; tri < out.NumTri(); ++tri) {
     int meshID = relation.triBary[tri].meshID;
-    int meshIdx = meshID2idx.find(meshID) != meshID2idx.end()
-                      ? meshID2idx.at(meshID)
-                      : meshID2idx.at(meshID2Original[meshID]);
+    int meshIdx = meshID2idx.at(meshID);
     ASSERT_LT(meshIdx, input.size());
     const Mesh& inMesh = input[meshIdx];
     int inTri = relation.triBary[tri].tri;
@@ -869,4 +866,4 @@ TEST(Boolean, DISABLED_Cylinders) {
   EXPECT_TRUE(m1.IsManifold());
   EXPECT_TRUE(m1.MatchesTriNormals());
   EXPECT_LE(m1.NumDegenerateTris(), 12);
-}
\ No newline at end of file
+}

[pca006132]

Final changes:

The meshID in the internal API is purely internal, usually starting from
0, which is used for differentiating between triangles coming from
different meshes. originalID can map this internal meshID to the
external meshID, and such mapping is automatically done when calling
GetMeshRelation, so users don't have to care about this mapping.

As a bonus, we don't need to deal with atomics when manipulating meshIDs
internally: we can use whatever IDs we want as long as the mapping is
preserved and distinct for triangles coming from different meshes.
I also made the meshID modification code run in the device, which should
be faster for larger meshes.

[elalish]

Yes, you are correct, sorry I forgot to mention that issue regarding edge collapse. And yes, I like your idea of making that an internal detail to simplify the API. I'm still feeling not great, so I can't promise when I'll do a proper code review. Hopefully soon. Thanks!

[elalish]

Looking great, thanks!

[elalish]

👍

[elalish]

I need to get used to using these device lambdas, at least for simple functions like this. Would probably clean up the code a lot. When I started, I couldn't get these to work.

[elalish]

👍

[elalish]

Simplifying the test is a good indication we're moving in the right direction on API shape. 👍

[elalish]

This looks great, thanks for helping to simplify the API here!

[pca006132]

Revisiting this after (mostly) finished the lazy boolean implementation. Keeping the meshIDs from different meshes distinct can fix the collapseEdge problem mentioned above, but it also prohibits reducing the set of vertices after boolean operations (e.g. when you union n cubes together as a larger cube, the number of vertices will grow to n*8).

Perhaps we should try to use the originalID instead and see how to fix the issue regarding edge collapse. Do you have any documentation about the simplify topology code that I can have a look at?