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

Use non-normalized normals in Mesh centroid #655

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

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Jun 25, 2024

This PR adds the fixes I discussed with @tibuch on Zulip (here), while trying to determine the difference in return between the geom.centroid Op operating on Meshes and the static algorithm MeshStats.centroid() from imglib2-mesh. This change does not need to be ported forward to SciJava Ops Image, as it is our plan to remove these Ops entirely, instead leaning upon imglib2-mesh (where we'd just use MeshStats.centroid())

@tim-oliver Buchholz I have finally figured out the difference - if you call mesh.triangles().add(idx1, idx2, idx3), the library will automatically normalize the normal vector. The geom.centroid Op expects the normal vector of each triangle to be non-normalized, as it uses those values directly in the computation (SciJava Ops Image does the same thing), however the algorithm in the paper requires non-normlized normal vectors. If you recompute the normal vector within the CentroidMesh Op, then you get the negative of MeshStats.centroid, because of the inversion here

The reason for the draft PR is that I am unsure how to fix the tests - there are three failures in Mesh features that depend upon the centroid Op which fail with these changes, and two suggest that the values were verified against MATLAB. I don't know which MATLAB code was used/how to verify.

@gselzer gselzer requested review from ctrueden and tibuch June 25, 2024 14:35
@gselzer gselzer self-assigned this Jun 25, 2024
@gselzer gselzer added the bug label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant