Check and fix tet orientations in TetMeshClipper#1800
Conversation
gunney1
requested review from
Arlie-Capps,
BradWhitlock,
bmhan12,
kennyweiss,
publixsubfan and
rhornung67
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks @gunney1
I had a question about the approach (see internal comments).
If (a) we keep the current approach and (b) the code always checks the orientations, and errors out if there's a negative tet (that we don't fix), then it seems like the default should be to have fixOrientation == true.
| constexpr double eps = 1e-12; | ||
| axom::for_all<ExecSpace>( | ||
| tetCount, | ||
| AXOM_LAMBDA(IndexType iTet) { | ||
| Point3DType verts[4]; | ||
| for(int vi = 0; vi < 4; ++vi) | ||
| { | ||
| int iPoint = connArray(iTet, vi); | ||
| verts[vi] = Point3DType {axom::NumericArray<double, 3> {x[iPoint], y[iPoint], z[iPoint]}}; | ||
| } | ||
|
|
||
| TetrahedronType tet(verts[0], verts[1], verts[2], verts[3]); | ||
| const auto signedVol = tet.signedVolume(); | ||
| if(signedVol < -eps) | ||
| { | ||
| if(fixOrientation) | ||
| { | ||
| std::swap(connArray(iTet, 2), connArray(iTet, 3)); | ||
| } | ||
| else | ||
| { | ||
| SLIC_ERROR(axom::fmt::format( | ||
| "TetMeshClipper: input tet[{}] is inverted, with volume {}. Please fix or add " | ||
| "'fixOrientation: 1' to the input klee::Geometry's hierarchical data.", | ||
| iTet, | ||
| signedVol)); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This approach, modifies the underlying connectivity arrays, and runs even when the user does not request fixOrientation. It also always throws an error if the user does not modify the connectivity if there is a negatively oriented tetrahedron.
Another approach might be to optionally add a pass that calls Tetrahedron::checkAndFixOrientation() on each tet in the array before running the clipping algorithm (without modifying the connectivity array).
And if the user doesn't ask for fixOrientation, they can get negative volumes.
Is there a reason to prefer the former to the latter?
There was a problem hiding this comment.
What I do is the same as checkAndFixOrientation. I just do it manually because I want to error out if the user sets fixOrientation to false, effectively saying that they know what they're doing. I can't count on negative volumes working out in any logical way.
Negative volumes don't behave the way one may think it should because of the various checks we have. It actually could yield zero instead of a negative number. To avoid such unexpected behavior, I thought it best to fix or reject the input based on fixOrientation right when we get it. So that negative volumes are avoided.
There was a problem hiding this comment.
I was suggesting to (a) modify the primal::Tetrahedra after reading them in, but before clipping, rather than modifying the conduit connectivity array, which would allow you to use the checkAndFix function (b) default to fixOrientation = true since the code won't work without it (but, maybe let the user override that
There was a problem hiding this comment.
Yeah, either would work. Note that the user's connectivity array isn't changed. It's the internal one that gets fixed.
There was a problem hiding this comment.
Working on the tetrahedra and calling tet.checkAndFixOrientation() seems cleaner to me
There was a problem hiding this comment.
Also, if we're always checking the orientation (to fix or warn/error) and the code won't work with bad orientations, we might as well always fix it and skip the extra option, right?
There was a problem hiding this comment.
Oh, another reason to fix the connectivity is to do it right when we get the input. The array of tets isn't generated until later, when we know what memory space to put it in. Not a huge reason, but that's how my brain sorted out the steps.
4 participants
Summary
TetMeshClipperclass comments, but I had forgotten to implement it.TetMeshClipperneeds thefixOrientationimplemented #1797