GEOS-DEV / GEOS

GEOS Simulation Framework
GNU Lesser General Public License v2.1
203 stars 80 forks source link

Force face-based ordering for node list of FaceElements #3133

Closed CusiniM closed 3 weeks ago

CusiniM commented 1 month ago

Currently the nodes of FaceElementSubRegions are ordered differently when the subRegion is filled from an external mesh and when it is filled by the SurfaceGenerator.

Specifically, the SurfaceGenerator orders the nodes of the FaceElementSubRegion as if it was an hexahedra whereas when reading the nodes from an external mesh the local odering il CCW for each face. This inconsistency creates an issue in the vtk output. In this PR we modify the odering of the nodes imposed by these two approaches to be consistent by always storing the nodes related to face 0 CCW, followed by the nodes related to face 1 also stored CCW.

However, using the 3D element map for plotting could be interesting coz it would allow a 3D view of the facelemetn which has some advantages. To do so, we could probably plot faceElements as general polyhedra though, for which I assume that the CCW ordering by face will work.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.

Project coverage is 56.45%. Comparing base (d0a4088) to head (f346099).

Files Patch % Lines
...mponents/fileIO/vtk/VTKPolyDataWriterInterface.cpp 0.00% 11 Missing :warning:
src/coreComponents/fileIO/Outputs/VTKOutput.cpp 0.00% 6 Missing :warning:
...mponents/fileIO/vtk/VTKPolyDataWriterInterface.hpp 0.00% 2 Missing :warning:
...sicsSolvers/surfaceGeneration/SurfaceGenerator.cpp 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #3133 +/- ## =========================================== - Coverage 56.45% 56.45% -0.01% =========================================== Files 1022 1022 Lines 86421 86431 +10 =========================================== + Hits 48792 48794 +2 - Misses 37629 37637 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CusiniM commented 1 month ago

So we're now in sync with

static std::vector< int > getVtkConnectivity( ElementType const elementType, localIndex const numNodes )
{
  switch( elementType )
  {
    ...
    case ElementType::Triangle:      return { 0, 1, 2 };
    case ElementType::Quadrilateral: return { 0, 1, 2, 3 };
    ...

?

we should be. HOwever, the better fix would be to use the faceToNodeMap for plotting a faceElement as a 2D cell instead of using the elemToNodesMap or to plot it as a 3D polyhedral element using the 'elemToNodesMap`.

rrsettgast commented 1 month ago

Face elements should be plotted as volume elements to allow for inspection in deformed meshes. I.e. when looking at a crack from the side with the displacement amplified, it is nice to see the contents of the crack and have variable coloring available.

CusiniM commented 1 month ago

Face elements should be plotted as volume elements to allow for inspection in deformed meshes. I.e. when looking at a crack from the side with the displacement amplified, it is nice to see the contents of the crack and have variable coloring available.

I have made this optional for now. It's a nice tool for us but I have seen people being already quite confused about how fault elements are modeled that I think it's better if they are not induced to think that they are 3D elements.

CusiniM commented 3 weeks ago

Face elements should be plotted as volume elements to allow for inspection in deformed meshes. I.e. when looking at a crack from the side with the displacement amplified, it is nice to see the contents of the crack and have variable coloring available.

I have made this optional for now. It's a nice tool for us but I have seen people being already quite confused about how fault elements are modeled that I think it's better if they are not induced to think that they are 3D elements.

Actually, to make this work properly for all shapes I will need to do a bit of extra work. I ll do it in a follow up PR so that I can merge this one and fix the current bug.