-
Notifications
You must be signed in to change notification settings - Fork 75
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
Added fail-if-invalid option - resolves issue #45. #53
base: master
Are you sure you want to change the base?
Conversation
src/util.c
Outdated
if( | ||
stl->stats.facets_w_1_bad_edge != 0 || | ||
stl->stats.facets_w_2_bad_edge != 0 || | ||
stl->stats.facets_w_3_bad_edge != 0 || | ||
stl->stats.degenerate_facets != 0 || | ||
stl->stats.edges_fixed != 0 || | ||
stl->stats.facets_removed != 0 || | ||
stl->stats.facets_added != 0 || | ||
stl->stats.backwards_edges != 0 || | ||
stl->stats.normals_fixed != 0 || | ||
( | ||
!reverse_all_flag && | ||
stl->stats.facets_reversed != 0 | ||
) | ||
) | ||
return 1; | ||
else | ||
return 0; |
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.
Two questions:
- Can the
if conditional: return 1, else return 0
construct be replaced withreturn conditional
in C? - What does the function return? It returns 1 (aka true) when the STL was invalid? In that case should the name reflect that?
stl_check_results
doe snot really tell which way it works. I would expect it to return 1 when the STl was valid. We could either flip the values or rnema it, e.g. tostl_was_invalid
.
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.
First question: yes, good point. I actually don't know why I thought it would me more readable inside the if statement.
Second question: Alright, renamed.
src/admesh.c
Outdated
@@ -450,7 +456,8 @@ redistribute it under certain conditions. See the file COPYING for details.\n") | |||
} | |||
|
|||
stl_stats_out(&stl_in, stdout, input_file); | |||
|
|||
if(fail_if_invalid_flag && stl_check_results(&stl_in, reverse_all_flag)) |
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.
If we do the reverse_all_flag if test outside of stl_check_results, then the function is more reusable outside of this main. WDYT?
Somethign like:
if(fail_if_invalid_flag && stl_check_results(&stl_in, reverse_all_flag)) | |
if(fail_if_invalid_flag && !reverse_all_flag && stl_check_results(&stl_in)) |
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.
The reason is we still want to check other stats for validity even if we explicitly reverse facets. So (just reporting our IRL discussion) we will pass a number of facets that should be reversed and check that instead.
Added a new CLI option to set a non-zero exit code if there are any errors detected in the file as in issue #45.