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

Flip triangles in csg_tree #404

Merged
merged 2 commits into from
Apr 7, 2023
Merged

Conversation

pca006132
Copy link
Collaborator

Although we already have related logic in impl.cpp, the compose code will skip performing transformation on the original mesh, and apply transformation on the combined mesh directly, skipping the previous fix. The logic is now added to the compose function to fix the bug without much performance penalty. This also factors out some functions used in both impl.cpp and csg_tree.cpp (although we should probably give it a better name).

Fixes #403

@pca006132 pca006132 requested a review from elalish April 6, 2023 10:23
@pca006132
Copy link
Collaborator Author

@elalish is there a way to check if triangle orientations are correct? I think we should add this to our tests as this bug occurred at least twice.

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 94.87% and project coverage change: -0.16 ⚠️

Comparison is base (f25d960) 86.04% compared to head (f4e1686) 85.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
- Coverage   86.04%   85.88%   -0.16%     
==========================================
  Files          36       37       +1     
  Lines        4485     4500      +15     
==========================================
+ Hits         3859     3865       +6     
- Misses        626      635       +9     
Impacted Files Coverage Δ
src/manifold/src/impl.cpp 94.97% <ø> (-0.29%) ⬇️
src/manifold/src/mesh_fixes.h 91.30% <91.30%> (ø)
src/manifold/src/csg_tree.cpp 92.56% <100.00%> (+0.22%) ⬆️

... and 1 file with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! As for tests, the best check of triangle orientation is the sign of the volume. For compose, you might want to decompose and check each volume's sign, or make sure the total is correct. There's also our testing hook, MatchesTriNormals(), but that's more of an internal consistency check.

@pca006132 pca006132 merged commit 85d74b4 into elalish:master Apr 7, 2023
22 checks passed
@pca006132 pca006132 deleted the fix-triangle branch August 15, 2023 12:53
@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* flip triangles in csg_tree

* added test
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.

mirror inverts triangles
2 participants