Facette scan#17
Conversation
edgar-bonet
left a comment
edgar-bonet
left a comment
There was a problem hiding this comment.
Thanks for the pull request! I still have to take a deep look at this code, but I quickly noticed a few issues, so I am requesting some changes that you can perform while I dig more deeply into the code.
First of all, there are quite a few changes here that have nothing to do with the purpose of the pull request: const correctness, moving a function from a header to a cpp file... These should ideally me moved to another pull request. At the very least, they should be in separate commits.
Other things that deserve fixing:
-
avoid introducing compiler warnings; for instance “comparison of integer expressions of different signedness”
-
avoid introducing trailing whitespace.
-
in English, “facette” is spelled “facet”
-
in English, there is no space before a colon (write “Error: …”)
-
messages output to the terminal should end in
'\n' -
also, please rebase on top of master
| double mesh::surface(std::vector<int> &facIndices) | ||
| void mesh::checkFacettes() const | ||
| { | ||
| std::vector<Facette::Fac> allFacCtnr; |
There was a problem hiding this comment.
The Facette::Fac class is too big for this usage: it has some vectors and matrices that are useful for mesh elements, but make no sense for the simple triangular faces of the tetrahedra. This is significant because we expect millions of such faces in the typical use case. A custom, leaner class/struc would be suitable here.
| tetrahedron.ind[(i + 1) % tetraN], | ||
| tetrahedron.ind[(i + 2) % tetraN]}); | ||
|
|
||
| // Search for an equal facette, insert at the end if not found. |
There was a problem hiding this comment.
Your comments are pretty nice: short, useful and to the point!
| int j = 0; | ||
| while (j < allFacCtnr.size() && !(allFacCtnr[j] == curFac)) | ||
| { j++; } |
There was a problem hiding this comment.
A for loop would be more idiomatic here.
More importantly, I am worried about the nesting of these loops. Looks to me like the algorithm is in O(n2), where n is the number of triangular faces. This can be annoying when n is in the millions. I do believe we can do way better.
2 participants
Add a function filtering some wrong mesh generations. It doesn't filter everything, but it can catch some poorly created mesh.