Property output

[elalish]

The upshot of discussions from #274, #196, and #91 is that the existing MeshRelation isn't the best API for reapplying mesh properties (UVs, normals, etc). Part of that is because it requires users to write interpolation functions, but a more fundamental issue is that the barycentric data structure actually loses some information: when an edge with shared property verts is cut by a Boolean op, the new vert no longer shares its properties on each side of the retained edge. Over time, this can lead to a lot of unnecessary duplication of property verts for rendering.

Instead, I'm now looking to make the new MeshGL struct focus on handling the property input and output. By adding a bit of merge information, it's able to take the non-manifold GL structures and allow exact reconstruction of the manifold (no geometric checks needed). This can also be saved in a glTF as a sparse accessor on the indices, so this could easily become a manifoldness extension to glTF.

[hrgdavor]

This sounds amazing. Looking fwd to some kind of blog post or explainer when it is implemented :)

[elalish]

This is the core of the new API, but it's still incomplete. For now the mesh/material/triangle reference still lives in the MeshRelation; I'm planning to build a different version of that information here and probably deprecate MeshRelation entirely.

[elalish]

@pca006132 I could use a hand if you can repro: our CI is consistently erroring out with Floating point exception, but everything is running fine for me locally on my mac. It sounds like the most likely culprit might be an integer divide by zero, but I can't find any. It's probably a simple error, but I'm baffled as to why it would work for me and not the CI.

[pca006132]

[ RUN      ] Boolean.Split

Thread 1 "manifold_test" received signal SIGFPE, Arithmetic exception.
0x00000000004cee78 in operator() (__closure=0x7fffffff5540) at /home/pca006132/code/manifold/src/manifold/src/edge_op.cpp:471
471	     const int newProp = prop.size() / numProp;
(gdb) p numProp
$1 = 0

Not sure why it works on your mac.

[elalish]

Perfect, thanks @pca006132!

[elalish]

@pca006132 I looked at the code, and that / numProp is inside a check of triProperties.size() > 0, so having numProp == 0 shouldn't be possible in there. I switched it to properties.size() > 0 and now Linux is happy, but mac and windows are segfaulting. I'm pretty sure there must be some uninitialized data or writing outside of bounds somewhere. Sadly valgrind doesn't exist on the mac and leaks isn't showing anything. If you have a moment for code review / debug, that'd be awesome, since I still can't repro locally.

[pca006132]

Yeah there are some invalid access. I tried adding some assertions and got some failures:

diff --git a/src/manifold/src/edge_op.cpp b/src/manifold/src/edge_op.cpp
index e046a9f..6516eca 100644
--- a/src/manifold/src/edge_op.cpp
+++ b/src/manifold/src/edge_op.cpp
@@ -143,6 +143,11 @@ void Manifold::Impl::SimplifyTopology() {
   sequence(policy, idx.begin(), idx.end());
   sort_by_key(policy, halfedge.begin(), halfedge.end(), idx.begin());
 
+  int numtri = NumTri();
+  ASSERT(all_of(policy, meshRelation_.triBary.begin(), meshRelation_.triBary.end(),
+        [=]__host__ __device__(const BaryRef& ref) {return ref.tri < numtri;}),
+      logicErr, "tri out of range");
+
   VecDH<int> flaggedEdges(halfedge_.size());
 
   int numFlagged =
@@ -461,7 +466,7 @@ void Manifold::Impl::RecursiveEdgeSwap(const int edge) {
     triBary[tri1].vertBary[perm1[0]] = newBary;
     triBary[tri0].vertBary[perm0[2]] = newBary;
     // Update properties if applicable
-    if (meshRelation_.triProperties.size() > 0) {
+    if (NumProp() > 0) {
       VecDH<glm::ivec3>& triProp = meshRelation_.triProperties;
       VecDH<float>& prop = meshRelation_.properties;
       triProp[tri0] = triProp[tri1];
diff --git a/src/manifold/src/impl.cpp b/src/manifold/src/impl.cpp
index 92b6ba7..a7c12ef 100644
--- a/src/manifold/src/impl.cpp
+++ b/src/manifold/src/impl.cpp
@@ -336,6 +336,7 @@ Manifold::Impl::Impl(const MeshGL& meshGL,
       mesh.halfedgeTangent[i][j] = meshGL.halfedgeTangent[4 * i + j];
   }
 
+  ASSERT(propertyTolerance.size() == numProp, userErr, "invalid number of properties");
   *this = Impl(mesh, triProperties, properties, propertyTolerance);
 }

Both of the assertions are sometimes invalid. For the first one regarding BaryRef::tri, I think that is triggering the segmentation fault you observed. The second assertion is based on the initialization of numProp = propertyTolerance.size() so I think the two should match. Here are the list of failures:

[ RUN      ] Manifold.MeshRelationRefine
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/impl.cpp (339): 'propertyTolerance.size() == numProp' is false: invalid number of properties" thrown in the test body.
[  FAILED  ] Manifold.MeshRelationRefine (0 ms)
[ RUN      ] Boolean.MeshRelation
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/impl.cpp (339): 'propertyTolerance.size() == numProp' is false: invalid number of properties" thrown in the test body.
[ RUN      ] SDF.Surface
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/edge_op.cpp (147): 'all_of(policy, meshRelation_.triBary.begin(), meshRelation_.triBary.end(), [=]__host__ __device__(const BaryRef& ref) {return ref.tri < numtri;})' is false: tri out of range" thrown in the test body.
[ RUN      ] Samples.TetPuzzle
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/edge_op.cpp (147): 'all_of(policy, meshRelation_.triBary.begin(), meshRelation_.triBary.end(), [=]__host__ __device__(const BaryRef& ref) {return ref.tri < numtri;})' is false: tri out of range" thrown in the test body.
[ RUN      ] Samples.Bracelet
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/edge_op.cpp (147): 'all_of(policy, meshRelation_.triBary.begin(), meshRelation_.triBary.end(), [=]__host__ __device__(const BaryRef& ref) {return ref.tri < numtri;})' is false: tri out of range" thrown in the test body.
[ RUN      ] Samples.GyroidModule
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/src/manifold/src/edge_op.cpp (147): 'all_of(policy, meshRelation_.triBary.begin(), meshRelation_.triBary.end(), [=]__host__ __device__(const BaryRef& ref) {return ref.tri < numtri;})' is false: tri out of range" thrown in the test body.

[elalish]

@pca006132 Perfect, thanks! Fixed that issue and now my tests are actually running the code I care about - and seg-faulting, so now I can debug locally. I thought it was a bit fishy they passed so easily before...

[fire]

How far is this from user testing?

[elalish]

@fire I think it's pretty close; I'm down to one test failure that I think I know how to address. Then I need to update the API for WASM. I'm also going to have to remove the barycentric stuff from MeshRelation; part of why this took so much longer than I thought it would is that this process led to me realize how broken my barycentric solution actually was, so I couldn't just reuse its logic.

[elalish]

Okay, I think I finally have this working, just need to update the WASM bindings for the new API.

[codecov-commenter]

Codecov Report

Base: 91.31% // Head: 91.54% // Increases project coverage by +0.23% 🎉

Coverage data is based on head (283190e) compared to base (a701d2b).
Patch coverage: 90.68% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   91.31%   91.54%   +0.23%     
==========================================
  Files          33       33              
  Lines        3533     3608      +75     
==========================================
+ Hits         3226     3303      +77     
+ Misses        307      305       -2     

Impacted Files	Coverage Δ
src/manifold/include/manifold.h	100.00% <ø> (ø)
src/manifold/src/constructors.cpp	95.90% <0.00%> (-2.30%)	⬇️
src/sdf/include/sdf.h	95.39% <ø> (-0.18%)	⬇️
src/manifold/src/impl.h	70.00% <50.00%> (-13.34%)	⬇️
src/utilities/include/utils.h	56.52% <66.66%> (+3.58%)	⬆️
src/manifold/src/shared.h	58.90% <73.33%> (-17.10%)	⬇️
src/utilities/include/public.h	73.80% <78.57%> (+17.49%)	⬆️
src/manifold/src/sort.cpp	87.42% <80.00%> (-2.43%)	⬇️
src/manifold/src/impl.cpp	95.68% <92.04%> (+1.72%)	⬆️
src/manifold/src/edge_op.cpp	98.94% <94.44%> (-0.70%)	⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pca006132]

Should we return NumVert() when there is no property or 0 as none of the vertices have any property? Actually I prefer having something like Error(NoProperty)/Ok(n), although this isn't really popular in C++..

[elalish]

This isn't public API, but this is correct because the (extra) properties are optional, while the positions are not. They get combined in the output MeshGL so that distinction isn't present in the public API.

[elalish]

I use NumProp() to determine if there aren't any properties, rather than NumPropVert(). I recall it simplified some code somewhere.

[pca006132]

OK thanks. Btw I will be a bit slow to review recently, so feel free to merge it first if you want, I will try to go over it later when I have more time. (plus this PR is huge and I am unfamiliar with the usecases, so will take more time to understand it)

[elalish]

Thanks, and no problem. Yeah, this did turn out huge, since it involved a wholesale replacement of the barycentric part of MeshRelation when I realized how broken it was. The good news is I feel like the testing is a lot tighter now - this took forever because of how many errors were caught in those sanity checks. So I feel pretty confident about it algorithmically, though it could probably use further cleanup. As a bonus it seems to run a bit faster now too.

[elalish]

@geoffder I see a cbind compile error on the CI I don't understand. Would you mind taking a look?

[geoffder]

manifold/bindings/c/include/manifoldc.h

Line 190 in 6a59065

int *manifold_meshgl_merge_from_vert(void *mem, ManifoldMeshGL *m);

from_ and to_vert declarations should return uint32_t instead of int to match the definitions.

Not sure why it was compiling for me when I made the update PR, I've hit other examples of that error before.

[elalish]

@geoffder Ah, thank you! I updated those but somehow missed manifoldc.h. Fixed.