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

avoid exits from MMG5_coltet() #257

Merged
merged 4 commits into from
Apr 12, 2024
Merged

avoid exits from MMG5_coltet() #257

merged 4 commits into from
Apr 12, 2024

Conversation

mpotse
Copy link
Contributor

@mpotse mpotse commented Apr 1, 2024

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.

mpotse added 2 commits March 31, 2024 21:31
  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).
@Algiane Algiane self-assigned this Apr 4, 2024
@Algiane Algiane added kind: enhancement enhancement to an existing feature kind: cleanup should be clean part: mmg3d mmg3d specific labels Apr 4, 2024
Copy link
Member

@Algiane Algiane left a 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

Comment on lines 767 to 768
* is not possible, while -2 and -3 just mean the job could not be done.
*
Copy link
Member

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well seen!

list,&ilist,lists,&ilists,&refmin,&refplus,p0->tag & MG_NOM);
if(bsret==-1 || bsret==-4){
return -3; // fatal
}else if(bsret==-2 || bsret==-3){
Copy link
Member

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...

@mpotse
Copy link
Contributor Author

mpotse commented Apr 7, 2024

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.

Algiane added 2 commits April 12, 2024 15:32
…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).
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 20.83333% with 19 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (develop@a4a5ba3). Click here to learn what that means.

Files Patch % Lines
src/mmg3d/mmg3d1.c 21.05% 14 Missing and 1 partial ⚠️
src/mmg3d/boulep_3d.c 20.00% 4 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@Algiane Algiane merged commit 058f9a3 into MmgTools:develop Apr 12, 2024
19 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup should be clean kind: enhancement enhancement to an existing feature part: mmg3d mmg3d specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants