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
Cgal surface to deal.II triangulation #13664
Conversation
ec6c629
to
720012f
Compare
0dce7af
to
210c832
Compare
/rebuild |
210c832
to
35784f6
Compare
When the triangulation is a manifold (dim < spacedim) and made of | ||
quadrilaterals, the normal field provided from the map class depends on |
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.
Why is this not an issue in other cases?
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.
It is an issue. But this loop won't work in that case (and it doesn't work also in the case of quadrilaterals for some perfectly valid grids, so I'm not sure what the loops actually does).
whether the orientation are the same (true) or opposite (false). | ||
|
||
Even though there may be a combinatorial/graph theory argument to get this | ||
table in any dimension, I tested by hand all the different possible cases |
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.
Who is "I"?
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 have no idea. In this whole comment, I only added "and made of quadrilaterals", to clarify that the check is skipped in the case of triangular meshes.
|
||
We store this data using an n_faces x n_faces full matrix, which is | ||
actually much bigger than the minimal data required, but it makes the code | ||
more readable. | ||
|
||
*/ | ||
if (dim < spacedim) | ||
if (dim < spacedim && all_reference_cells_are_hyper_cube()) |
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.
Maybe you can skip this for triangulation generation but are the results obtained on such meshes correct?
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.
If we check somewhere else that the orientation is correct in the case of triangles, then we should not have problems here. CGAL generates consistently oriented meshes. We should definitely check that they are consistently oriented for tringle meshes. How is this done in two dimensions + simplices now? If what we do when we create the grid is enough in 2d+simplices, then it should be ok also in this case. I think the issues with quads is that we have a highly non-standard orientation in our triangulation class (i.e., we do not base our triangulation on the half-edge datastructure, but on lexycographical ordering, and consistent orientation of edges across neighbors). This gives issues when extracting boundary triangulation (for example), since some faces will have the right orientation, and some other won't. I believe this is the reason for this check to be here in the first place.
This is not the case for triangles. Triangles follow the same orientation of most other libraries (anti-clockwise), and so does our tetra-hedron classes, so each face is oriented consistently w.r.t. to each cell, and you are guaranteed that even if you extract the bundary of a tet-only grid, you get a consistently oriented grid. I believe the check for triangles (and tetra) should be that, on each edge cell->face(f)->vertex_index(0) == cell->neighbor(f)->face(cell->neighbor(f)->neighbor_of_neighbor(f))->vertex_index(1)
is true, but I have to double check this.
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.
We should definitely check that they are consistently oriented for tringle meshes. How is this done in two dimensions + simplices now?
I assumed that the meshes coming from a CAD tool are correctly oriented. But unfortunately, this is not always given as the issue #13346 showed. It was fixed in #13579. For the normal case dim==spacedim
, we have the checks:
Lines 2309 to 2323 in 5cdf7bd
if (dim == spacedim) | |
for (unsigned int cell_no = 0; cell_no < cells.size(); ++cell_no) | |
{ | |
// If we should check for distorted cells, then we permit them | |
// to exist. If a cell has negative measure, then it must be | |
// distorted (the converse is not necessarily true); hence | |
// throw an exception if no such cells should exist. | |
if (tria.check_for_distorted_cells) | |
{ | |
const double cell_measure = GridTools::cell_measure<spacedim>( | |
vertices, | |
ArrayView<const unsigned int>(cells[cell_no].vertices)); | |
AssertThrow(cell_measure > 0, ExcGridHasInvalidCell(cell_no)); | |
} | |
} |
and
dealii/source/fe/mapping_fe.cc
Lines 1233 to 1240 in 5cdf7bd
// TODO: this allows for anisotropies of up to 1e6 in 3D and | |
// 1e12 in 2D. might want to find a finer | |
// (dimension-independent) criterion | |
Assert(det > | |
1e-12 * Utilities::fixed_power<dim>( | |
cell->diameter() / std::sqrt(double(dim))), | |
(typename Mapping<dim, spacedim>::ExcDistortedMappedCell( | |
cell->center(), det, point))); |
that will definitely be triggered if the mesh is not correctly orientated.
I believe the check for triangles (and tetra) should be that, on each edge cell->face(f)->vertex_index(0) == cell->neighbor(f)->face(cell->neighbor(f)->neighbor_of_neighbor(f))->vertex_index(1) is true, but I have to double check this.
Do you want to open an issue? I guess one can come up with a minimal test with a hand full of cells?
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.
It is unclear to me what we want to do. For the codimension zero case, it is clear that if a mesh has negative determinant, it should not be allowed. For co-dimension one grids, however, the J in JxW is defined as sqrt(det(DF^T.DF)), and it is positive regardless of the orientation of the normal (i.e, the previous expression is the same as computing the norm of the cross product of the tangent vectors, or the columns of the Jacobian). The orientation of the mesh in the codimension one case does not influence the sign of the determinant but just the orientation of the normal to the cell. Do we want to enforce a consistent orientation, or should we allow discontinuous normals? A moebious strip is not orientable, but it is a perfectly valid co-dimension one mesh... It won't make sense to ask for a continuous normal in many cases. I think we should remove the check, and let the user be responsible for the orientation.
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 know my previous comments partly contradicts what I said earlier, but I did not have the moebious strip example in mind when I wrota that
If we check somewhere else that the orientation is correct in the case of triangles, then we should not have problems here. CGAL generates consistently oriented meshes.
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 am not sure what the influence of discontinuous normals are so I cannot really tell when/where it is the best to deal with them. Maybe you could post a simple example?
35784f6
to
65984f2
Compare
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.
Fine from my side to merge! @luca-heltai You are blocking the PR (changes requested) ;)
Thanks @fdrmrc ! I really like where we are going... :) |
This allows to go from a CGAL::Surface_mesh or a CGAL::Polyhedron_3 to a Triangulation<2,3>.
Currently this is supporting only surfaces made by quads.Depends on #13661. Relevant changes to this PR can be seen at: luca-heltai/dealii@c965b7f...0dce7af