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
Refactor silhouette from add_mesh #1689
Refactor silhouette from add_mesh #1689
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1689 +/- ##
==========================================
- Coverage 92.59% 92.59% -0.01%
==========================================
Files 71 71
Lines 13864 13870 +6
==========================================
+ Hits 12838 12843 +5
- Misses 1026 1027 +1 |
Still failing due to:
+100 for cleaning up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I made some minor changes to get the links to build correctly, but otherwise LGTM.
Overview
add_mesh
is a monster and the logic is very complex depending on combinations of input parameters. I think it is best if we can extract out as much logic as possible into separable pieces. Silhouette functionality is completely separate from all the other pieces. This PR separates out the silhouette functionality into it's own method, that can be called on it's own if desired.add_mesh
calls this new method.Details
My personal style preference would be to have the below for any functionality like this:
This makes it more clear that we have two separate actors being added to the scene. I'm not sure if the current state has problems. If the mesh is updated, will the silhouette also update? In any case, separating out the logic is the first step.
TODO: