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

added keyword for positive=true from simplexify() for GMSH mesh #82

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

janmodderman
Copy link
Contributor

Hi everyone,

I'm working with GridapGMSH and GridapEmbedded and I want to extend my code to embedding .stl's via the STLCutters package. One requirement for STLCutters and triangular meshes appears to be the orientation of the cells.

In Gridap, this orientation is enforced by the kwarg positive=true when using the simplexify function. For some reason, it seems that the orientation is automatically selected for when using a GMSH mesh and therefore I added this keyword so the user can have control on the orientation of the elements in their mesh.

Best regards,

Jan

@JordiManyer
Copy link
Member

Hi @janmodderman , thanks for the contribution!

The issue I see is that the name seems to imply that it works for any type of mesh, which is not the case.

I think it would be more correct to have the kwarg be orient_if_simplex, so that your feature be triggered by orient_if_simplex=false with the logic

 if ( Dp == 3 && Dc == 2 ) || ( Dp == 2 && Dc == 1 )
    @assert isnothing(orient_if_simplex)
    orient_if_simplex = false
  else
    orient_if_simplex = isnothing(orient_if_simplex) || orient_if_simplex
  end

or something similar. I.e we disable the orientation instead of imposing positivity (which is what you are doing).

Let me know what you think.

@janmodderman
Copy link
Contributor Author

Hi @JordiManyer, you're right.

I think the suggestion you made, makes more sense. I replaced the positive keyword with the orient_if_simplex in the code and added your conditional statement instead of the one I suggested. With this implementation everything seems to be working for me, so I have no further points of discussion.

@JordiManyer JordiManyer merged commit 366eecd into gridap:master Jan 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants