-
-
Notifications
You must be signed in to change notification settings - Fork 56
Use shoelace approach to compute normals #245
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
Conversation
@ffreyer how's this coming along? Let me know if I can help. |
I think this is good to go. It would be good if you checked again if everything is still working. Maybe also add some basic tests for the things codecov is flagging as missing (see files changed) Just fyi, I changed things around so that orthogonal_vector always returns a Vec3 now. |
Sure I'll have a look.
Great, that makes sense. |
@ffreyer I've done some tests and it seems fine. I think testing is needed for (or is there anything else?): However, I could not figure out how to test for these. I kept getting errors and was not sure how to proceed, specifically I tried something like: v = (Point{3,Float64}(0.0,0.0,0.0),Point{3,Float64}(1.0,0.0,0.0),Point{3,Float64}(2.0,0.0,0.0),Point{3,Float64}(2.0,1.0,0.0))
GeometryBasics.orthogonal_vector(v) I also wasn't sure how to trigger the "if vertices are something else" related line there |
@ffreyer @SimonDanisch This PR implements the "shoelace" method for normal computation:

The approach should reduce to the same thing for triangles but generalises properly for faces with more than 3 vertices. Previously 3 points were taken along the face to compute the normal. This assumes that the face/polygon is planar and that the chosen points are not for co-linear edges. However both can naturally occur for faces with more than 3 nodes. Previously such faces would have inaccurate normals computed or NaNs would be returned.
Here is a test case for a Quad with a co-linear edge, the above implemented method now returns the z-dir as the normal direction properly:
I have implemented other cases for testing too. Note that during testing for me this errored:
It seemed correct to withing 1e-8, So I changed it to use
≈
Check if that is indeed what we'd like.
Let me know if you are happy with this or if you need me to work on anything.