add MeshGL64 #927

pca006132 · 2024-09-13T13:35:48Z

Added MeshGL64 and removed usages of Mesh inside our library. Mesh is not yet removed because we use it extensively in tests, we should soon remove it soon though.

@elalish we have two failures related to refine and smooth though, they are Smooth.ToLength and Samples.Scallop. The stats are different significantly, so perhaps I did something wrong in my refactoring.

Note that we haven't yet exposed MeshGL64 to C and python bindings, I think that can be done later. I did remove Mesh from the C binding.

pca006132 · 2024-09-13T14:45:28Z

And it seems that we miss a vertex normal calculation function that write normals to property channels? If I understand it correctly, CalculateNormals calculate face normals.

pca006132 · 2024-09-13T16:54:53Z

and I am thinking maybe we should add some simple iterators to MeshGL so we can easily iterate over vertex positions and tri verts as triples, without changing the memory representation.

elalish · 2024-09-13T17:06:02Z

and I am thinking maybe we should add some simple iterators to MeshGL so we can easily iterate over vertex positions and tri verts as triples, without changing the memory representation.

Yeah, good idea. That fits with the helper functions we put in the WASM bindings too.

elalish

This is looking great!

elalish · 2024-09-13T22:01:30Z

samples/src/scallop.cpp

-    scallop.triVerts.push_back({0, 2 + i, 2 + j});
+    scallop.triVerts.insert(
+        scallop.triVerts.end(),
+        {0, static_cast<uint32_t>(2 + i), static_cast<uint32_t>(2 + j)});


Can we skip the casting if we just define i and j as uint32_t?

It depends if we need signed results for other things that depend on i and j, iirc there are results that can be negative before this line?

src/manifold/include/manifold/manifold.h

elalish · 2024-09-13T22:27:23Z

src/manifold/src/impl.h

+
+    if (meshGL.runOriginalID.empty()) {
+      // FIXME: should we do this?
+      meshRelation_.originalID = Impl::ReserveIDs(1);


Huh, it does seem to conflict with the statement at the end of the function. Does anything break if you remove it? I should probably think through the cases here...

elalish · 2024-09-13T22:31:50Z

src/manifold/src/impl.h

+        }
+        tri[j] = prop2vert[vert];
+        // FIXME: do we need this check?
+        if (tri[j] < 0 || tri[j] >= static_cast<int>(numVert)) {


Agreed, looks like that's already checked above.

elalish · 2024-09-13T22:32:46Z

src/manifold/src/impl.h

+      ivec3 tri;
+      for (const size_t j : {0, 1, 2}) {
+        const int vert = meshGL.triVerts[3 * i + j];
+        if (vert < 0 || vert >= static_cast<int>(numVert)) {


Can we avoid the cast by declaring vert as size_t?

probably yes

pca006132 · 2024-09-14T00:58:32Z

Btw, MeshGLP is just a random naming, feel free to suggest another if you think of something better.

pca006132 · 2024-09-14T15:45:07Z

And it seems that we miss a vertex normal calculation function that write normals to property channels? If I understand it correctly, CalculateNormals calculate face normals.

@elalish btw what about this? Am I overlooking some API or we should add this?

pca006132 · 2024-09-14T17:01:43Z

For Smooth.ToLength, if we switch to using Smooth with a Mesh, it works:

diff --git a/test/smooth_test.cpp b/test/smooth_test.cpp
index 8d11dd89..88efcba1 100644
--- a/test/smooth_test.cpp
+++ b/test/smooth_test.cpp
@@ -109,7 +109,7 @@ TEST(Smooth, ToLength) {
       CrossSection::Circle(10, 10).Translate({10, 0}).ToPolygons(), 2, 0, 0,
       {0, 0});
   cone += cone.Scale({1, 1, -5});
-  Manifold smooth = Manifold::Smooth(cone.GetMeshGL64());
+  Manifold smooth = Manifold::Smooth(cone.GetMesh());
   smooth = smooth.RefineToLength(0.1);
   ExpectMeshes(smooth, {{85250, 170496}});
   auto prop = smooth.GetProperties();

So maybe the mesh is not being simplified due to not being original, and it is somehow giving a different volume?
The weird thing though is that the volume difference is pretty large:

The difference between prop.volume and 4604 is 77.845313791107401, which exceeds 1, where
prop.volume evaluates to 4526.1546862088926,
4604 evaluates to 4604, and
1 evaluates to 1.

pca006132 · 2024-09-14T17:13:25Z

OK looking at the outputs it seems that they are indeed very different:

The expected one:

The current one with MeshGL:

It is like getting punched in the face XD

elalish

As for CalculateNormals, I realize now it's a bit confusing since we have both a Manifold and an Impl method with that name that don't do the same thing. We should rename the Impl one - it calculates both face and vertex normals, but just the simple internal versions. The public method does much more complicated vertex normal calculations that are put into the public property channels.

So, I don't think we're missing anything. Certainly some tests should fail if we are.

src/manifold/src/impl.h

src/manifold/src/manifold.cpp

test/manifold_test.cpp

elalish · 2024-09-14T18:43:23Z

I'm pretty certain the issue with Smooth.ToLength is that certain triangle pairs used to be found to be coplanar and treated as quads for smoothing, but now they are not. Would be good to know if it's some kind of precision error or something else. AsOriginal may fix it, but I'd like to understand why it's behaving differently in the first place. Take a look at the faceIDs right after CreateFaces runs to see the difference.

Should we mark the test as broken for now? I think this issue is not related to MeshGL/MeshGL64 in general?

pca006132 · 2024-09-15T05:36:18Z

So, I don't think we're missing anything. Certainly some tests should fail if we are.

Oh, I don't mean we miss some calculation, I just mean there is no simple way to get vertex normal with MeshGL and it makes it hard to get rid of Mesh in our tests.

codecov · 2024-09-15T06:00:20Z

Codecov Report

Attention: Patch coverage is 92.04152% with 23 lines in your changes missing coverage. Please review.

Project coverage is 88.33%. Comparing base (d437097) to head (6cdbd95).
Report is 93 commits behind head on master.

Files with missing lines	Patch %	Lines
src/manifold/src/impl.h	91.48%	8 Missing ⚠️
src/manifold/src/manifold.cpp	86.88%	8 Missing ⚠️
src/manifold/src/sort.cpp	91.66%	5 Missing ⚠️
src/manifold/include/manifold/manifold.h	80.00%	1 Missing ⚠️
test/test_main.cpp	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
- Coverage   91.84%   88.33%   -3.51%     
==========================================
  Files          37       62      +25     
  Lines        4976     8670    +3694     
  Branches        0     1049    +1049     
==========================================
+ Hits         4570     7659    +3089     
- Misses        406     1011     +605

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elalish

I just mean there is no simple way to get vertex normal with MeshGL and it makes it hard to get rid of Mesh in our tests.

Interesting - which tests is this making difficult? I can take a look at refactoring them.

elalish · 2024-09-15T06:40:15Z

src/manifold/src/impl.h

+
+#define IGNORE_DEPRECATED_BEGIN  \
+  _Pragma("GCC diagnostic push") \
+      _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"")


This is cool, but should we just go ahead and remove Mesh for v3.0? It would be a lot less maintenance burden.

test/manifold_test.cpp

pca006132 · 2024-09-15T06:51:43Z

Interesting - which tests is this making difficult? I can take a look at refactoring them.

In general, WithNormals, ExpectMeshes, CheckNormals in test_main.cpp, as well as Manifold.Normals.

And I also reverted the deprecated commit, that is a bit hard to get right on all platforms (e.g. clang 18 has a bug that breaks pragma diagnostic push.

pca006132 · 2024-09-16T03:01:07Z

@elalish so should I just disable the failed test xase and merge this?

elalish · 2024-09-16T04:12:35Z

test/smooth_test.cpp

@@ -111,8 +109,7 @@ TEST(Smooth, ToLength) {
      CrossSection::Circle(10, 10).Translate({10, 0}).ToPolygons(), 2, 0, 0,
      {0, 0});
  cone += cone.Scale({1, 1, -5});
-  Manifold smooth = Manifold::Smooth(cone.GetMesh());
-  smooth = smooth.RefineToLength(0.1);
+  Manifold smooth = cone.AsOriginal().SmoothOut(180).RefineToLength(0.1);


Okay, I fixed it with AsOriginal and I think I understand why - our old Mesh didn't keep track of IDs, which allowed those faces to become quads, but MeshGL keeps more info. I think this is fine.

elalish

Looks great, thanks!

elalish · 2024-09-16T04:39:25Z

Hmm, I should have asked - did you add any tests of MeshGL64? Not that it's likely to behave much differently, but at least one that tests, say, the difference of round-trip precision between MeshGL and MeshGL64? Fodder for a follow-on.

pca006132 · 2024-09-16T04:42:12Z

Yeah, we should definitely add some tests for MeshGL64. I think I used it sparingly for some tests, but it is kind of random (depends on my mood) and only a few of them uses MeshGL64.

add MeshGL64

c41a918

pca006132 linked an issue Sep 13, 2024 that may be closed by this pull request

Add MeshGL64 #916

Closed

pca006132 added this to the v3.0 milestone Sep 13, 2024

pca006132 requested a review from elalish September 13, 2024 13:38

elalish reviewed Sep 13, 2024

View reviewed changes

pca006132 added 4 commits September 14, 2024 23:52

allow SetProperties to zero property channel

1ed00f8

address comments

0a49002

minor test updates

8d0877e

fixed scallop

ba7eec4

elalish reviewed Sep 14, 2024

View reviewed changes

src/manifold/src/impl.h Show resolved Hide resolved

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved

test/manifold_test.cpp Show resolved Hide resolved

test/manifold_test.cpp Outdated Show resolved Hide resolved

address comments

fb7a6ae

elalish reviewed Sep 15, 2024

View reviewed changes

pca006132 force-pushed the meshgl64 branch 2 times, most recently from edaaece to fb7a6ae Compare September 15, 2024 10:02

fix test

6cdbd95

elalish reviewed Sep 16, 2024

View reviewed changes

elalish approved these changes Sep 16, 2024

View reviewed changes

pca006132 merged commit 22c6605 into elalish:master Sep 16, 2024
22 checks passed

pca006132 deleted the meshgl64 branch September 16, 2024 04:36

elalish mentioned this pull request Nov 5, 2024

v3.0 #1024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add MeshGL64 #927

add MeshGL64 #927

pca006132 commented Sep 13, 2024

pca006132 commented Sep 13, 2024

pca006132 commented Sep 13, 2024

elalish commented Sep 13, 2024

elalish left a comment

elalish Sep 13, 2024

pca006132 Sep 14, 2024

elalish Sep 13, 2024

elalish Sep 13, 2024

elalish Sep 13, 2024

pca006132 Sep 14, 2024

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

elalish left a comment

elalish commented Sep 14, 2024 •

edited by pca006132

Loading

pca006132 commented Sep 15, 2024

codecov bot commented Sep 15, 2024 •

edited

Loading

elalish left a comment

elalish Sep 15, 2024

pca006132 Sep 15, 2024

pca006132 commented Sep 15, 2024

pca006132 commented Sep 16, 2024

elalish Sep 16, 2024 •

edited

Loading

elalish left a comment

elalish commented Sep 16, 2024

pca006132 commented Sep 16, 2024

add MeshGL64 #927

add MeshGL64 #927

Conversation

pca006132 commented Sep 13, 2024

pca006132 commented Sep 13, 2024

pca006132 commented Sep 13, 2024

elalish commented Sep 13, 2024

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

pca006132 commented Sep 14, 2024

elalish left a comment

Choose a reason for hiding this comment

elalish commented Sep 14, 2024 • edited by pca006132 Loading

pca006132 commented Sep 15, 2024

codecov bot commented Sep 15, 2024 • edited Loading

Codecov Report

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Sep 15, 2024

pca006132 commented Sep 16, 2024

elalish Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

elalish commented Sep 16, 2024

pca006132 commented Sep 16, 2024

elalish commented Sep 14, 2024 •

edited by pca006132

Loading

codecov bot commented Sep 15, 2024 •

edited

Loading

elalish Sep 16, 2024 •

edited

Loading