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

Adding angle and uniform weights in per_vertex_normals #141

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

NIcolasp14
Copy link

Adding angle and uniform weights in per_vertex_normals, testing with armadillo using the files:
test_uniform_per_vertex_normals.py
test_angle_per_vertex_normals.py

@odedstein odedstein requested a review from sgsellan August 4, 2024 07:56
@odedstein
Copy link
Collaborator

Hi Nicolas,

Thanks for your contribution. We'll review it and then give you feedback.

Things I noticed from skimming your PR:

  • You have created new files for the unit tests. Usually, we have one unit test file (test_function) for each file in gpytoolbox (function). Can you change this?
  • You make sure, in your new added code, that a division by zero can't happen when normalizing. This is good (and something that the existing code does not do). However, it can happen that the result of your normalization procedure does not have unit norm. Can we ensure that the per-vertex normals that this function outputs always have norm 1? For example, by picking some back-up normal when normalization fails.
  • This normalization procedure should be the same no matter which triangle weighting algorithm I choose. It should be moved out of the big "if" statement, and apply to all weighting algorithms (to avoid code duplication).

@sgsellan we should take this as an opportunity to maybe change the unit tests for per_vertex_normal... there are no examples testing on a very basic input, and the test file is over a MB.

@NIcolasp14
Copy link
Author

Hello professor Oded,

Thank you for the prompt reply and your feedback. I adjusted the code. I chose the back up normal to be the nearest valid normal. Also, the user has the ability to either correct or omit invalid normals. In the extreme case that there are no valid normals at all, I assign a default normal. Let me know if anything else needs refining.

@odedstein
Copy link
Collaborator

This looks ok to me.

@NIcolasp14
Copy link
Author

Awesome!

@sgsellan
Copy link
Owner

This looks great to me! One question: shouldn't compute_angles be swapped by the gpytoolbox.tip_angles we already have in the codebase, to avoid code replication? Or is there a difference between these two angles that I'm missing.

@sgsellan
Copy link
Owner

Also noticed that there's no test for angle weights, probably worth writing one

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