-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
avoid exits from MMG5_coltet() #257
Conversation
Let MMG5_boulesurfvolpNom() return different values for different failures, and let MMG5_coltet() take them into account by just lumbering on if the failure does not look fatal. This is currently necessary to make the larger or more difficult test cases in the MICROCARD project get through Mmg when the cell domains are preserved (so there can well be 4 or 5 references around a single vertex).
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.
Thanks for this improvement.
I just think that we should stop the remeshing process when the edge is not found in bouleSurfVolNomp
: if you agree and confirm that it doesn't happen too often I can make the change from my side.
Thanks again,
Best
src/mmg3d/boulep_3d.c
Outdated
* is not possible, while -2 and -3 just mean the job could not be done. | ||
* |
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.
I don't really remember when we discuss that but I was wrong: I think that we the -3 error (edge cannot be found) should not happend.
Does this happen frequently?
* | ||
*/ | ||
static int MMG5_coltet(MMG5_pMesh mesh,MMG5_pSol met,int8_t typchk) { | ||
static MMG5_int MMG5_coltet(MMG5_pMesh mesh,MMG5_pSol met,int8_t typchk) { |
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.
Well seen!
src/mmg3d/mmg3d1.c
Outdated
list,&ilist,lists,&ilists,&refmin,&refplus,p0->tag & MG_NOM); | ||
if(bsret==-1 || bsret==-4){ | ||
return -3; // fatal | ||
}else if(bsret==-2 || bsret==-3){ |
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.
I think that -3 is fatal too...
I agree; I have never seen the -3 error and I classified it as harmless only because you told me so. Fine if you handle it. Maybe you would also like to insert a warning message (above a certain verbosity level) for any negative return value from MMG5_boulesurfvolpNom. I have that in my personal branch and forgot to include it in the PR. Or maybe you have a better way to remember that there is something odd with the function. |
…eshing process in this case.
…boulesurfvolpNom to continue the remeshing process if more than 3 domains are found when trying to collapse a non-manifold point (it was already done and tested for the second call).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #257 +/- ##
==========================================
Coverage ? 49.85%
==========================================
Files ? 178
Lines ? 47523
Branches ? 10187
==========================================
Hits ? 23693
Misses ? 16158
Partials ? 7672 ☔ View full report in Codecov by Sentry. |
Complex meshes in the MICROCARD project currently have a 2/3 chance of remesher failure because MMG5_coltet() exit()s when MMG5_boulesurfvolpNom() returns a negative value, indicating that it cannot compute the ball of a given vertex. But extensive testing with a private branch has shown that we do not have to take all of these failures as critical. So I let the return values of MMG5_boulesurfvolpNom() distinguish between 4 different causes of failure, and let MMG5_coltet() continue instead of exit when the cause of failure is that (-2) there are more than two references in the ball (a common situation in the MICROCARD meshes) or (-3) an edge could not be found (only because I think Algiane said this is harmless too).
As a result of this change, edges involving a vertex with more than 2 references in its ball will simply not be collapsed, rather than causing an exit. Perhaps we never want such edges to be collapsed anyway.
What remains mysterious to me is that not all vertices with more than 2 references in their ball caused such exits: 3 references is common in all of our cases, and 4 or 5 also occurs.
I also don't understand why MMG5_boulesurfvolpNom() requires that there are just two references in the ball. Is this a leftover from before the introduction of multimaterial functionality - when the only surfaces were level sets? The "refmin" and "refmax" nomenclature in the function arguments suggests that.