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

Fix CoquilFace warning #234

Merged
merged 2 commits into from
Jan 19, 2024
Merged

Conversation

coprigent
Copy link
Contributor

@coprigent coprigent commented Dec 15, 2023

This update fixes the triggering of a warning relative to the check of the number of boundary faces in the shell of a manifold edge.

Issue description

Collapses would sometimes merge two boundary manifold edges (that would each have 2 boundary faces in their shells) into one edge with 4 faces in the shell.

It happens when a non-boundary triangle has 3 boundary edges and we collapse one of the boundary edge (the two other edges being merged together).
(See attached picture were the edge p-q is collapsed, the 3 colored edges are boundary edges and the triangle p-p-q is an internal triangle).
non-bdy-tri

To avoid this situation, when collapsing an edge, we travel the shell of the edge and we check the presence of an "internal triangle with 3 boundary edges" situation. It requires to be able to identify if an edge is a boundary edge.

The bug solved by this PR is due to the non-consistency of boundary tags between faces and edges.

Tag management in Mmg

Currently, the Mmg convention seems to be the following one:

  • an edge related to a boundary face inside a given tetra is:
    • marked as MG_BDY+another tag if it is related to a given feature (ridge, ref edge, nosurf...);
    • marked MG_BDY or not marked (0 tag) if it is a regular surface edge (regular surface edges without tags are created by the pattern splittings, see here.
  • a boundary edge not related to a boundary face may have aMG_BDY tag or not (it is due to the fact that it is expensive to update the edge tag informations for each edge of a tetra when a xtetra is added to the tetra during a collapse, thus, only edges of the new boundary face are updated).
  • a tetra with a boundary edge but no boundary face may have or not have a xtetra: in any case, we must not treat a boundary edge from a non-boundary face.
  • If an edge, related to a boundary face or not, has a non-nul tag, the tag has to contain all the needed informations (an edge has either no tag at all, or all the needed tags). It means that matching edges should have no tags or consistent tags inside the tetra to which they belong.

Note that it seems that we have few tests that seems erroneous regarding this convention:

  • in lagrangian motion
  • during the computation of anisotropic edge lengths
  • at anisotropic metric interpolation

Resolution

As the MG_BDY tag is not always present along regular boundary edges, instead of checking for xt->tag[i] & MG_BDY , we now check the edge tag from a boundary face (we test xt->ftag & MG_BDY for both faces sharing edge i): if the edge has no tag at all, it is a regular edge so we now that the edge is exactly MG_BDY, if it has a non-nul tag, we recover the tag stored in the xtetra .

…r a edge is boundary or not : replaced check of tag & MG_BDY by ftag & MG_BDY
@Algiane Algiane self-requested a review December 15, 2023 15:39
@Algiane Algiane added kind: bug error or fault part: mmg3d mmg3d specific priority: high linked to blocking behaviour or to a close deadline labels Dec 15, 2023
Comment on lines 396 to 397
if ((pxt->ftag[MMG5_ifar[i][0]] & MG_BDY) || (pxt->ftag[MMG5_ifar[i][1]] & MG_BDY)) {
*tag |= MG_BDY;
Copy link
Member

Choose a reason for hiding this comment

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

  • The if ( pxt->tag[i] & MG_BDY ) can be kept as it may allow to exit the loop faster;
  • You can assign both pxt->tag[i] and MG_BDY tag using the bitewise OR operator pxt->tag[i] | MG_BDY

Finally, the test must be something like that:

if ( pxt->tag[i] & MG_BDY ) {
  *tag |= pxt->tag[i];
  *filled =1;
   return adj;
}  else if ((pxt->ftag[MMG5_ifar[i][0]] & MG_BDY) || (pxt->ftag[MMG5_ifar[i][1]] & MG_BDY)) {
  *tag |= (pxt->tag[i] | MG_BDY);
  *filled = 1;
   return adj;
}

Comment on lines 449 to 450
if ((pxt->ftag[MMG5_ifar[ia][0]] & MG_BDY) || (pxt->ftag[MMG5_ifar[ia][1]] & MG_BDY)) {
*tag |= MG_BDY;
Copy link
Member

Choose a reason for hiding this comment

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

Same remarks

@Algiane Algiane mentioned this pull request Dec 19, 2023
…nditions to check wether an edge has to be considered boundary
@Algiane Algiane self-assigned this Jan 19, 2024
@Algiane Algiane merged commit 64e4ce9 into MmgTools:develop Jan 19, 2024
23 checks passed
@Algiane
Copy link
Member

Algiane commented Jan 19, 2024

Thanks for this bugfix.

@coprigent coprigent deleted the feature/fix_coquilface2 branch January 19, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug error or fault part: mmg3d mmg3d specific priority: high linked to blocking behaviour or to a close deadline
Projects
Development

Successfully merging this pull request may close these issues.

2 participants