Changed MeshRelation API (#184)

* cause repro

* added originalID to BaryRef

* make coverage check informational

* unique meshIDs for compose

* make AsOriginal decimate

* remove vscode properties from git

* update docs

* remove vertNormal handling

* Revert "remove vertNormal handling"

This reverts commit c92d590.

* alternative fix

.codecov.yml

@@ -0,0 +1,8 @@
+coverage:
+  status:
+    project:
+      default:
+        informational: true
+    patch:
+      default:
+        informational: true

.gitignore

@@ -107,3 +107,4 @@ Temporary Items
 
 build
 __pycache__
+.vscode/c_cpp_properties.json

samples/src/menger_sponge.cpp

@@ -54,7 +54,12 @@ Manifold MengerSponge(int n) {
   result -= hole;
   hole = hole.Rotate(0, 0, 90);
   result -= hole;
-  result = result.AsOriginal();
+
+  // Alternative order causes degenerate triangles
+  // Manifold tmp1 = hole.Rotate(90) + hole.Rotate(90).Rotate(0, 0, 90);
+  // Manifold tmp2 = hole + tmp1;
+  // result = result - tmp2;
+
   return result;
 }
 }  // namespace manifold

src/manifold/include/manifold.h

@@ -118,7 +118,7 @@ class Manifold {
    */
   ///@{
   MeshRelation GetMeshRelation() const;
-  std::vector<int> GetMeshIDs() const;
+  int OriginalID() const;
   Manifold AsOriginal() const;
   ///@}
 

src/manifold/src/boolean_result.cpp

@@ -19,10 +19,6 @@
 #include "par.h"
 #include "polygon.h"
 
-// Mark the meshID as coming from P by flipping the 30th bit.
-// The 30th bit is used to avoid flipping the sign.
-constexpr int kFromP = 1 << 30;
-
 using namespace manifold;
 using namespace thrust::placeholders;
 
@@ -421,6 +417,8 @@ struct CreateBarycentric {
   glm::vec3 *barycentricR;
   BaryRef *faceRef;
   int *idx;
+
+  const int offsetQ;
   const int firstNewVert;
   const glm::vec3 *vertPosR;
   const glm::vec3 *vertPosP;
@@ -446,9 +444,10 @@ struct CreateBarycentric {
     const BaryRef oldRef = halfedgeRef.PQ == 0 ? triBaryP[tri] : triBaryQ[tri];
 
     faceRef[halfedgeR.face] = oldRef;
-    if (halfedgeRef.PQ == 0) {
-      // Mark the meshID as coming from P
-      faceRef[halfedgeR.face].meshID |= kFromP;
+
+    if (halfedgeRef.PQ == 1) {
+      // Mark the meshID as coming from Q
+      faceRef[halfedgeR.face].meshID += offsetQ;
     }
 
     if (halfedgeR.startVert < firstNewVert) {  // retained vert
@@ -488,19 +487,22 @@ std::pair<VecDH<BaryRef>, VecDH<int>> CalculateMeshRelation(
   outR.meshRelation_.barycentric.resize(outR.halfedge_.size());
   VecDH<BaryRef> faceRef(numFaceR);
   VecDH<int> halfedgeBary(halfedgeRef.size());
+
+  const int offsetQ = Manifold::Impl::meshIDCounter_;
   VecDH<int> idx(1, 0);
   for_each_n(
       autoPolicy(halfedgeRef.size()),
       zip(halfedgeBary.begin(), halfedgeRef.begin(), outR.halfedge_.cbegin()),
       halfedgeRef.size(),
       CreateBarycentric(
           {outR.meshRelation_.barycentric.ptrD(), faceRef.ptrD(), idx.ptrD(),
-           firstNewVert, outR.vertPos_.cptrD(), inP.vertPos_.cptrD(),
+           offsetQ, firstNewVert, outR.vertPos_.cptrD(), inP.vertPos_.cptrD(),
            inQ.vertPos_.cptrD(), inP.halfedge_.cptrD(), inQ.halfedge_.cptrD(),
            inP.meshRelation_.triBary.cptrD(), inQ.meshRelation_.triBary.cptrD(),
            inP.meshRelation_.barycentric.cptrD(),
            inQ.meshRelation_.barycentric.cptrD(), invertQ, outR.precision_}));
   outR.meshRelation_.barycentric.resize(idx[0]);
+
   return std::make_pair(faceRef, halfedgeBary);
 }
 }  // namespace
@@ -674,19 +676,7 @@ Manifold::Impl Boolean3::Result(Manifold::OpType op) const {
   if (ManifoldParams().intermediateChecks)
     ASSERT(outR.Is2Manifold(), logicErr, "simplified mesh is not 2-manifold!");
 
-  // Index starting from 0 is chosen because I want the kernel part as simple as
-  // possible.
-  VecDH<int> meshIDs;
-  VecDH<int> original;
-  for (auto &entry : inP_.meshRelation_.originalID) {
-    meshIDs.push_back(entry.first | kFromP);
-    original.push_back(entry.second);
-  }
-  for (auto &entry : inQ_.meshRelation_.originalID) {
-    meshIDs.push_back(entry.first);
-    original.push_back(entry.second);
-  }
-  outR.UpdateMeshIDs(meshIDs, original);
+  outR.IncrementMeshIDs(0, outR.NumTri());
 
 #ifdef MANIFOLD_DEBUG
   simplify.Stop();

src/manifold/src/constructors.cpp

@@ -381,13 +381,6 @@ std::vector<Manifold> Manifold::Decompose() const {
     return meshes;
   }
   VecDH<int> vertLabel(components);
-  // meshID mapping for UpdateMeshIDs
-  VecDH<int> meshIDs;
-  VecDH<int> original;
-  for (auto& entry : pImpl_->meshRelation_.originalID) {
-    meshIDs.push_back(entry.first);
-    original.push_back(entry.second);
-  }
 
   std::vector<Manifold> meshes;
   for (int i = 0; i < numLabel; ++i) {
@@ -421,9 +414,6 @@ std::vector<Manifold> Manifold::Decompose() const {
 
     impl->Finish();
 
-    // meshIDs and original will only be sorted after successful updates, so we
-    // can keep using the old one.
-    impl->UpdateMeshIDs(meshIDs, original);
     meshes.push_back(Manifold(impl));
   }
   return meshes;

src/manifold/src/csg_tree.cpp

@@ -210,18 +210,10 @@ Manifold::Impl CsgLeafNode::Compose(
               combined.halfedge_.begin() + nextEdge,
               UpdateHalfedge({nextVert, nextEdge, nextTri}));
 
-    // Assign new IDs to triangles added in this iteration, to differentiate
-    // triangles coming from different manifolds.
-    VecDH<int> meshIDs;
-    VecDH<int> original;
-    for (auto &entry : node->pImpl_->meshRelation_.originalID) {
-      meshIDs.push_back(entry.first);
-      original.push_back(entry.second);
-    }
-    int meshIDStart = combined.meshRelation_.originalID.size();
-    combined.UpdateMeshIDs(meshIDs, original, nextTri,
-                           node->pImpl_->meshRelation_.triBary.size(),
-                           meshIDStart);
+    // Since the nodes may be copies containing the same meshIDs, it is
+    // important to increment them separately so that each node instance gets
+    // unique meshIDs.
+    combined.IncrementMeshIDs(nextTri, node->pImpl_->NumTri());
 
     nextVert += node->pImpl_->NumVert();
     nextEdge += 2 * node->pImpl_->NumEdge();

src/manifold/src/impl.cpp

@@ -146,11 +146,29 @@ struct InitializeBaryRef {
     int tri = thrust::get<1>(inOut);
 
     baryRef.meshID = meshID;
+    baryRef.originalID = meshID;
     baryRef.tri = tri;
     baryRef.vertBary = {-3, -2, -1};
   }
 };
 
+struct MarkMeshID {
+  int* includesMeshID;
+
+  __host__ __device__ void operator()(BaryRef& ref) {
+    includesMeshID[ref.meshID] = 1;
+  }
+};
+
+struct UpdateMeshID {
+  const int* meshIDold2new;
+  const int meshIDoffset;
+
+  __host__ __device__ void operator()(BaryRef& ref) {
+    ref.meshID = meshIDold2new[ref.meshID] + meshIDoffset;
+  }
+};
+
 struct CheckProperties {
   const int numSets;
 
@@ -349,9 +367,8 @@ void Manifold::Impl::ReinitializeReference(int meshID) {
   // 0 -> meshID, because the meshID after boolean operation also starts from 0.
   for_each_n(autoPolicy(NumTri()),
              zip(meshRelation_.triBary.begin(), countAt(0)), NumTri(),
-             InitializeBaryRef({0, halfedge_.cptrD()}));
-  meshRelation_.originalID.clear();
-  meshRelation_.originalID[0] = meshID;
+             InitializeBaryRef({meshID, halfedge_.cptrD()}));
+  meshRelation_.originalID = meshID;
 }
 
 int Manifold::Impl::InitializeNewReference(
@@ -591,53 +608,33 @@ void Manifold::Impl::CalculateNormals() {
 }
 
 /**
- * Update meshID and originalID in meshRelation_.triBary[startTri..startTri+n]
- * according to meshIDs -> originalIDs mapping. The updated meshID will start
- * from startID.
- * Will raise an exception if meshRelation_.triBary[startTri..startTri+n]
- * contains a meshID not in meshIDs.
- *
- * We remap them into indices starting from startID. The exact value value is
- * not important as long as
- * 1. They are distinct
- * 2. `originalID[meshID]` is the original mesh ID of the triangle
- *
- * Use this when the mesh is a combination of several meshes or a subset of a
- * larger mesh, e.g. after performing boolean operations, compose or decompose.
+ * Remaps all the contained meshIDs to new unique values to represent new
+ * instances of these meshes.
  */
-void Manifold::Impl::UpdateMeshIDs(VecDH<int>& meshIDs, VecDH<int>& originalIDs,
-                                   int startTri, int n, int startID) {
-  if (n == -1) n = meshRelation_.triBary.size();
-  sort_by_key(autoPolicy(n), meshIDs.begin(), meshIDs.end(),
-              originalIDs.begin());
-  constexpr int kOccurred = 1 << 30;
-  VecDH<int> error(1, -1);
-  const int numMesh = meshIDs.size();
-  const int* meshIDsPtr = meshIDs.cptrD();
-  int* originalPtr = originalIDs.ptrD();
-  int* errorPtr = error.ptrD();
-  for_each(autoPolicy(n), meshRelation_.triBary.begin() + startTri,
-           meshRelation_.triBary.begin() + startTri + n,
-           [=] __host__ __device__(BaryRef & b) {
-             int index = thrust::lower_bound(meshIDsPtr, meshIDsPtr + numMesh,
-                                             b.meshID) -
-                         meshIDsPtr;
-             if (index >= numMesh || meshIDsPtr[index] != b.meshID) {
-               *errorPtr = b.meshID;
-             }
-             b.meshID = index + startID;
-             originalPtr[index] |= kOccurred;
-           });
-
-  ASSERT(error[0] == -1, logicErr,
-         "Manifold::UpdateMeshIDs: meshID " + std::to_string(error[0]) +
-             " not found in meshIDs.");
-  for (int i = 0; i < numMesh; ++i) {
-    if (originalIDs[i] & kOccurred) {
-      originalIDs[i] &= ~kOccurred;
-      meshRelation_.originalID[i + startID] = originalIDs[i];
-    }
-  }
+void Manifold::Impl::IncrementMeshIDs(int start, int length) {
+  VecDH<BaryRef>& triBary = meshRelation_.triBary;
+  ASSERT(start >= 0 && length >= 0 && start + length <= triBary.size(),
+         logicErr, "out of bounds");
+  const auto policy = autoPolicy(length);
+  // Use double the space since the Boolean has P and Q instances.
+  VecDH<int> includesMeshID(2 * Manifold::Impl::meshIDCounter_, 0);
+
+  auto begin = triBary.begin() + start;
+  auto end = begin + length;
+
+  for_each(policy, begin, end, MarkMeshID({includesMeshID.ptrD()}));
+
+  inclusive_scan(autoPolicy(includesMeshID.size()), includesMeshID.begin(),
+                 includesMeshID.end(), includesMeshID.begin());
+
+  const int numMeshIDs = includesMeshID[includesMeshID.size() - 1];
+  const int meshIDstart = Manifold::Impl::meshIDCounter_.fetch_add(
+      numMeshIDs, std::memory_order_relaxed);
+
+  // We do start - 1 because the inclusive scan makes our first index 1 instead
+  // of 0.
+  for_each(policy, begin, end,
+           UpdateMeshID({includesMeshID.cptrD(), meshIDstart - 1}));
 }
 
 /**

src/manifold/src/impl.h

@@ -27,17 +27,9 @@ namespace manifold {
 struct Manifold::Impl {
   struct MeshRelationD {
     VecDH<glm::vec3> barycentric;
-    /// meshID in BaryRef has different meaning in MeshRelation and
-    /// MeshRelationD:
-    /// - In `MeshRelation`: The original mesh triangle index.
-    /// - In `MeshRelationD`: The original mesh triangle index =
-    /// `originalID[meshID]`
-    ///
-    /// @note Triangles coming from different manifolds should have different
-    /// mesh ID, otherwise `SimplifyTopology` will not work properly.
     VecDH<BaryRef> triBary;
-    /// meshID to originalID mapping.
-    std::unordered_map<int, int> originalID;
+    /// The meshID of this Manifold if it is an original; -1 otherwise.
+    int originalID = -1;
   };
 
   Box bBox_;
@@ -70,8 +62,7 @@ struct Manifold::Impl {
   void ReinitializeReference(int meshID);
   void CreateHalfedges(const VecDH<glm::ivec3>& triVerts);
   void CalculateNormals();
-  void UpdateMeshIDs(VecDH<int>& meshIDs, VecDH<int>& originalIDs,
-                     int startTri = 0, int n = -1, int startID = 0);
+  void IncrementMeshIDs(int start, int length);
 
   void Update();
   void MarkFailure(Error status);