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

Vectorize MeshData vertex normals computation #2434

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

asnt
Copy link
Member

@asnt asnt commented Nov 27, 2022

Suggestion to extend #2433 with tests, clarification of documentation and some formatting.

@asnt asnt changed the title Vectorize meshdata vertex normals Vectorize MeshData vertex normals Nov 27, 2022
@asnt asnt changed the title Vectorize MeshData vertex normals Vectorize MeshData vertex normals computation Nov 27, 2022
@djhoese
Copy link
Member

djhoese commented Nov 28, 2022

Ok so this is #2433 with tests?

@soraxas how does this look to you?

@soraxas
Copy link
Contributor

soraxas commented Nov 28, 2022

Yep the code LGTM.

Unit test seems straightforward although it didn't quite test much of the edge cases. Should perform sufficient sanity check though.

@djhoese
Copy link
Member

djhoese commented Nov 28, 2022

Great. @asnt do you think these need any other tests?

@asnt
Copy link
Member Author

asnt commented Nov 28, 2022

Great. @asnt do you think these need any other tests?

Not sure.
What are the possible edge cases? What comes to mind:

  • Degenerate triangles (i.e. collinear or coinciding vertices) having undefined normals.
  • Distance between vertices close to machine epsilon creating numerical errors: inaccurate or wrong normals.

These are not handled in the current code. Not sure we want this handled at this level of abstraction.

EDIT: Fix typos.

@asnt
Copy link
Member Author

asnt commented Nov 30, 2022

Duplicate: #2069

@djhoese
Copy link
Member

djhoese commented Jan 13, 2023

I want this PR and all the related ones to stop weighing on me so I'm going to merge this and anything that breaks we can fix later. We've spent too much time trying to get a perfect PR for this optimization and I think this is the closest we've gotten. I think after this then @asnt can work on #2098 to clean up the code a bit (if you want).

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

Successfully merging this pull request may close these issues.

None yet

3 participants