Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a manifold test case for custom "revolve". #1093

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fire
Copy link
Contributor

@fire fire commented Dec 3, 2024

Add test.

@fire
Copy link
Contributor Author

fire commented Dec 3, 2024

This might be as expected since it's not a crash and may or may not be manifold (probably non-manifold).

test/manifold_test.cpp Show resolved Hide resolved
test/manifold_test.cpp Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-test branch 2 times, most recently from c423dd2 to 4aa91fd Compare December 3, 2024 23:31
@elalish
Copy link
Owner

elalish commented Dec 3, 2024

@pca006132 is it expected that about half our CI jobs (the ones with green checkmarks) just build but don't actually run any tests?

@elalish
Copy link
Owner

elalish commented Dec 3, 2024

@fire - so it's working except that we're getting 16 triangles instead of your expected 48. Can you show the difference in renders so we can get an idea of what's wrong with the shape?

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

Do you know if I can export GLTF2 from here? I can't preview the mesh in Godot Engine because the condition is that the manifold is manifold.

We could do empty mesh for (merge and intersection) and identity for subtraction.

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

Yeah, you can add this in like a bunch of our other tests have:

#ifdef MANIFOLD_EXPORT
  if (options.exportModels) {
    ExportOptions options2;
    ExportMesh("complex.glb", manifold.GetMeshGL(), options2);
  }
#endif

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

Then just run ./manifold_test --gtest_filter=Manifold.MeshWithCustomProps -e to export.

@pca006132
Copy link
Collaborator

@pca006132 is it expected that about half our CI jobs (the ones with green checkmarks) just build but don't actually run any tests?

Hmmm, weird. Will check.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

complex.glb.zip

Here's the complex glb.

Screenshot 2024-12-03 at 4 28 10 pm
Screenshot 2024-12-03 at 4 28 21 pm

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

The failure points seem to be at the top and bottom.

image

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.90%. Comparing base (3603542) to head (2c992b8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
+ Coverage   91.87%   91.90%   +0.03%     
==========================================
  Files          30       30              
  Lines        5883     5883              
==========================================
+ Hits         5405     5407       +2     
+ Misses        478      476       -2     

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

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

@fire I don't see a failure - can you highlight the difference? Also, your test passes, so....

And don't get me started on USDZ! Here you can drag and drop GLBs into my web viewer here: https://modelviewer.dev/editor/

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

With this change, I cannot export.

% ./test/./manifold_test --gtest_filter=Manifold.MeshWithCustomProps -e
Note: Google Test filter = Manifold.MeshWithCustomProps
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Manifold
[ RUN      ] Manifold.MeshWithCustomProps
manifold_test(58041,0x202c23840) malloc: Incorrect checksum for freed object 0x1537044b8: probably modified after being freed.
Corrupt value: 0x1000101000100
manifold_test(58041,0x202c23840) malloc: *** set a breakpoint in malloc_error_break to debug
zsh: abort      ./test/./manifold_test --gtest_filter=Manifold.MeshWithCustomProps -e

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

Screenshot 2024-12-03 at 4 47 35 pm

These are non-manifold, according to Blender. See also the right side 3D-print fixer.

Edited:

Also, the custom mesh revolve's vertex are non-manifold at the far back.

Edited:

image

I think this is an input error, but I would like to know how to fix it.

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

These are non-manifold, according to Blender. See also the right side 3D-print fixer.

Um, I disagree? Your test is checking manifoldness and it passes. I trust my manifold checker better than anyone else's, but I especially don't trust file importers and exporters. Particularly since every mesh you've shown me doesn't even have shared verts, I have the feeling manifoldness is getting lost somewhere in Godot.

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

Remember - you cannot have a manifold without shared verts.

@pca006132
Copy link
Collaborator

Wonder if we can do an export-import round-trip in this case. If not, maybe the exporter is to blame here?

@elalish
Copy link
Owner

elalish commented Dec 4, 2024

To export, you'd also need to compile Manifold with MANIFOLD_EXPORT=ON, which requires Assimp, so you may not be set up for that.

@pca006132
Copy link
Collaborator

@fire can you merge the current master branch? It seems that you do not have #1092 in your branch. I guess github somehow automatically merge stuff when running the CI...

And I tried a round-trip of export-import, it works fine.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

I suspect what is happening is that the listed mesh edge is not connected in Godot Engine's input Mesh. I will try to explore this hypothesis.

The CSGPolygon3D is relatively untested with hellish/manifold, but people depend on it, so we're trying to avoid crashing or empty scenes.

Also, these are failure cases, so I am a bit frustrated, but the MeshGL dumping tool has been a great way to communicate the failures and gives me hope.

This test should pass.

@fire fire force-pushed the vsk-csg-test branch 2 times, most recently from 0568fc8 to 128a747 Compare December 4, 2024 01:49
@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

I did some sleuthing, I tried printing after the mesh.Merge() operation and it gives this test result.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

!

If you force add a MeshExport64 it fails even with Merge();

@fire fire force-pushed the vsk-csg-test branch 3 times, most recently from ef37f7b to 8060e57 Compare December 4, 2024 02:27
@pca006132
Copy link
Collaborator

Merge is only best-effort and cannot guarantee the result to be manifold, so this is not really an issue?

@pca006132
Copy link
Collaborator

Try a larger tolerance value, this is probably because the tolerance value becomes too small when using MeshGL64.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

Thanks for the hint, I expect the test to pass.

@pca006132
Copy link
Collaborator

Btw, do note that if you need the merge operation to make the custom revolve operation produce a manifold mesh, it probably means you may want to modify the revolve operation to make sure it always outputs a manifold mesh. There is no guarantee that the merge operation can work in general.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

I did a test manifold's CSG revolve: godotengine/godot#99977, but I wanted to try using the existing godot engine CSG code first.

@fire
Copy link
Contributor Author

fire commented Dec 4, 2024

@elalish Should I close this pull request? Any revisions I can make?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants