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

Refactoring MMG5_chkBdryTria #259

Merged

Conversation

corentin-prigent
Copy link
Contributor

@corentin-prigent corentin-prigent commented Apr 15, 2024

This update refactorizes the function MMG_chkBdryTria. It is now written as a sequence of several sub-functions. It does not modify the behaviour or features of mmg. However, this update is necessary for parmmg, in order to take into account the fact that we pack the mesh->tria array when extra triangles are deleted from it.
This update primarily ensures the maintainability of the code and avoids redundancy in mmg and parmmg.

Comment on lines 1876 to 1879
if ( k!=nbl ) {
pttnew = &mesh->tria[nbl];
memcpy(pttnew,ptt,sizeof(MMG5_Tria));
}
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 we can remove / extract this from the loop and just delete extra triangles inside the loop.
Then, after the loop (so outside the MMG5_chkbdryTria_deleteExtraBoundaries() func), it should be possible to :

  • travel triangles
  • detect the deleted tria
  • pack the array accordingly
  • update the perm array for ParMmg only

Copy link
Member

Choose a reason for hiding this comment

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

something like that :

nt=0; nbl=1;
for (k=1; k<=mesh->nt; k++) {
    ptt = &mesh->tria[k];
    
    if ( !MG_EOK(ptt) ) continue;
    
    ++nt;
    if ( k!=nbl ) {
      pttnew = &mesh->tria[nbl];
      memcpy(pttnew,ptt,sizeof(MMG5_Tria));
      if ( permtria ) {
         permtria[k] = nbl;
       }
    }
}
    

Copy link
Member

Choose a reason for hiding this comment

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

Or more elegant but also more error-prone (I am not sure that it works ;-) )

    k = 1;
    do {
      ptt = &mesh->tria[k];
      if ( !MG_EOK(ptt) ) {
        pttnew = &mesh->tria[mesh->nt];
        assert( ptt && pttnew && MG_EOK(pttnew) );
        memcpy(ptt,pttnew,sizeof(MMG5_Tria));

       if ( permtria ) permtria[mesh->nt] = k;

        ptt->v[0] = 0;
        mesh->nt--;
      }
    }
    while ( ++k < mesh->nt );

Comment on lines 1876 to 1879
if ( k!=nbl ) {
pttnew = &mesh->tria[nbl];
memcpy(pttnew,ptt,sizeof(MMG5_Tria));
}
Copy link
Member

Choose a reason for hiding this comment

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

something like that :

nt=0; nbl=1;
for (k=1; k<=mesh->nt; k++) {
    ptt = &mesh->tria[k];
    
    if ( !MG_EOK(ptt) ) continue;
    
    ++nt;
    if ( k!=nbl ) {
      pttnew = &mesh->tria[nbl];
      memcpy(pttnew,ptt,sizeof(MMG5_Tria));
      if ( permtria ) {
         permtria[k] = nbl;
       }
    }
}
    

src/mmg3d/hash_3d.c Show resolved Hide resolved
}
else if ( j > 0 ) {
/* the face already exists in the tria table */
continue;
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 here we can insert the tria deletion in order to perform the compression of triangle array outside the loop (see comment where the array is currently packed).

With something like : ptt->v[0] = 0; MG_EOK(ptt) should be false.

Comment on lines 1876 to 1879
if ( k!=nbl ) {
pttnew = &mesh->tria[nbl];
memcpy(pttnew,ptt,sizeof(MMG5_Tria));
}
Copy link
Member

Choose a reason for hiding this comment

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

Or more elegant but also more error-prone (I am not sure that it works ;-) )

    k = 1;
    do {
      ptt = &mesh->tria[k];
      if ( !MG_EOK(ptt) ) {
        pttnew = &mesh->tria[mesh->nt];
        assert( ptt && pttnew && MG_EOK(pttnew) );
        memcpy(ptt,pttnew,sizeof(MMG5_Tria));

       if ( permtria ) permtria[mesh->nt] = k;

        ptt->v[0] = 0;
        mesh->nt--;
      }
    }
    while ( ++k < mesh->nt );

@corentin-prigent corentin-prigent marked this pull request as ready for review April 16, 2024 14:08
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

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

Project coverage is 43.98%. Comparing base (058f9a3) to head (cf436e4).

Files Patch % Lines
src/mmg3d/hash_3d.c 65.48% 17 Missing and 22 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #259      +/-   ##
===========================================
+ Coverage    43.96%   43.98%   +0.01%     
===========================================
  Files          179      179              
  Lines        54102    54124      +22     
  Branches     10249    10252       +3     
===========================================
+ Hits         23786    23805      +19     
  Misses       22599    22599              
- Partials      7717     7720       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@corentin-prigent corentin-prigent changed the title Creation of skeleton of refactored MMG5_chkBdryTria; for draft PR pur… Creation of skeleton of refactored MMG5_chkBdryTria; for draft PR purposes Apr 16, 2024
@corentin-prigent corentin-prigent changed the title Creation of skeleton of refactored MMG5_chkBdryTria; for draft PR purposes Refactoring MMG5_chkBdryTria Apr 16, 2024
@Algiane Algiane added kind: refactoring needs refactoring part: mmg3d mmg3d specific labels Apr 19, 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!

@Algiane Algiane merged commit b338fee into MmgTools:develop Apr 19, 2024
25 checks passed
@corentin-prigent corentin-prigent deleted the feature/factorize-chkbdrytria branch April 22, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: refactoring needs refactoring part: mmg3d mmg3d specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants