Enable CGAL on Travis + AppVeyor.#996
Merged
alecjacobson merged 19 commits intolibigl:devfrom Dec 24, 2018
Merged
Conversation
Collaborator
Author
|
And obviously it fails on both Travis and AppVeyor, argh. |
Collaborator
Author
|
Any template expert in the room? I'm dumbfounded by the last 3 missing template instantiations, as the offending signatures are literally present in their respective .cpp files... |
Collaborator
Author
|
Ok now it compiles on Windows and Linux! (b7db5a8) @qnzhou do you mind taking a look at the unit test that is failing on macOS? And also maybe check that I didn't mess up anything else in this PR. I've made some slight modifications to make it compile on Windows with CGAL + unit tests:
|
Contributor
|
@jdumas Is that normal that some unit tests are missing opening brackets? I could not compile them without adding them. |
Collaborator
Author
|
Which one are you talking about? Are you sure you are checking my branch for this PR? I've fixed those missing brackets and it compiles on AppVeyor. |
Contributor
|
@jdumas You are right. I was on a wrong branch. Sorry! |
Contributor
|
Related to this is compilation also fails when using |
Collaborator
Author
|
Is it a missing template instantiation? In which case it should be easily fixed. |
Contributor
|
There are many other places [Side note: I see |
Collaborator
Author
|
Right. I believe we should do some SFINAE magic then. But I'd rather do it as a separate issue/PR. First goal is to get the test to pass in static mode (right now they randomly fail for some reason). |
Contributor
|
@jdumas I do not know the solution for the issue but I have noticed something that IMHO does not look like a good practice with unit testing. One of the rules of TDD and unit testing says that your tests should not depend on external resources, while you have in CGTree Would not that be better to keep such data as small as possible and in a compilation unit file? Then this can be compiled-in and served via, let's say, a UnitTestData class, and accessed without any need for data storage, etc. Some people keep fonts in such way for GUI without font files. |
Contributor
|
Anyway, what I wrote above about external deps. is not super important. I will try to find a machine with os x to check the test failing issue. |
Collaborator
Author
|
What's wrong with loading a mesh? True this implies that the mesh loading function has no bugs but... well you need to start somewhere. There's no need to over-engineer any extra class, and it makes it easy to add new test data if need be. Otherwise, you'd still have to trust that you did the conversion correctly from the .obj to the embedded format anyway... |
Contributor
|
The problem is that your test may fail due to storage failure. If you have a class that is responsible for serving that you will first have tests to test it. But of course this is not the best solution. As long as your data are simple you can verfiry them, this is why simplifying these testing data to minimum would be a good thing to do anyway. |
Collaborator
Author
|
Unless we start putting gigantic meshes in the testing data I'm not convinced this is worth the effort (and the problem still stands if you cannot preload the embedded data in memory). |
This was referenced Dec 22, 2018
Contributor
|
There's a lot here. What's the upshot? What's blocking this from being merged? |
5 tasks
Collaborator
Author
|
Bottom line is that some unit tests are randomly failing on Travis due to what I suspect may be uninitialized memory issue, or something similar. See for example 985ad5d that passes the tests on Travis, but a3f2406 doesn't, and changes in a3f2406 are unlikely to be the cause of the failure. If you compile this branch and run |
Contributor
|
I fixed the conflicting files in 85b9382 but I don't know how to append it to this PR. The commands I tried pushing it to jdumas's fork said permission denied. Is there a special command? |
Collaborator
Author
|
Did you try pushing directly on my |
Contributor
|
For posterity: this was merged despite seemingly breaking the continuous integration compilation and unit tests. In fact, those tests were disabled, so this merge now just accurately displays the (poor) state of affairs. |
Contributor
|
f186bed fixed the compilation issues. There's some kind of memory issue happening that causes the CGAL test(s) to occasionally fail. If I run it 1000 times I can usually get the |
Collaborator
|
Looking into it. |
Collaborator
|
Here is a stand alone code that will reproduce the issue on MacOS. #include <iostream>
#include <Eigen/Core>
#include <igl/read_triangle_mesh.h>
#include <igl/copyleft/cgal/CSGTree.h>
int main() {
constexpr size_t N=1e6;
Eigen::MatrixXd V, VC;
Eigen::MatrixXi F, FC;
//igl::read_triangle_mesh("extrusion.obj", V, F);
igl::read_triangle_mesh("ball.obj", V, F);
for (size_t i=0; i<N; i++) {
igl::copyleft::cgal::CSGTree tree(V, F);
igl::copyleft::cgal::CSGTree inter(tree, tree, "i"); // returns error
Eigen::MatrixXd V2 = inter.cast_V<Eigen::MatrixXd>();
Eigen::MatrixXi F2 = inter.F();
if (V2.rows() != V.rows()) {
throw "V different";
}
if (F2.rows() != F.rows()) {
throw "V different";
}
}
return 0;
}The issue is quite hard to trace (Thanks to cgal and boost). It has nothing to do with the mesh used. A simple ball or box can also be used to reproduce the problem. However, here are some interesting observations: Instead of igl::copyleft::cgal::CSGTree tree(V, F);
igl::copyleft::cgal::CSGTree inter(tree, tree, "i");Replacing it with a simple call to igl::copyleft::cgal::mesh_boolean(V, F, V, F, igl::MESH_BOOLEAN_TYPE_INTERSECT, VC, FC);seems to never fail. Thus, the problem may be related to Here is another variation that also seems to work: igl::copyleft::cgal::CSGTree tree1(V, F);
igl::copyleft::cgal::CSGTree tree2(V, F);
igl::copyleft::cgal::CSGTree inter(tree1, tree2, "i");Thus, the crash disappears by creating two identical trees instead of reusing the same tree. My current suspicion is that the memory deallocation mechanism of |
Contributor
|
This would make sense. I'm not sure many are using the CSGTree interface.
Or at least I rarely am. So it's probably less tested
|
Contributor
|
@qnzhou Your code has seemed to work fine on Linux until I ran it in valgrind. Then I got this exception: I am not exacty sure what valgrind changes that the code suddenly brakes, and to be 100% I am not saying that the above issue has anything to od with the OS X problem but it may. Just my 3 cents, take it easy. PS valgrind options: |
Collaborator
Author
|
The valgrind crash is a known issue, orthogonal to this pb. You can find it
discussed at length in an issue thread I opened on CGAL’s github (on the
phone right now so can’t find it).
…On Sat, Dec 29, 2018 at 11:23 AM Kacper Pluta ***@***.***> wrote:
@qnzhou <https://github.com/qnzhou> Your code has seemed to work fine on
Linux until I ran it in valgrind. Then I got this exception:
terminate called after throwing an instance of 'CGAL::Assertion_exception'
what(): CGAL ERROR: assertion violation!
Expr: -CGAL_IA_MUL(-1.1, 10.1) != CGAL_IA_MUL(1.1, 10.1)
File: /home/kacper/Projects/libigl/external/cgal/Installation/lib/cmake/CGAL/../../../../Number_types/include/CGAL/Interval_nt.h
Line: 210
Explanation: Wrong rounding: did you forget the -frounding-math option if you use GCC (or -fp-model strict for Intel)?
Process finished with exit code 134 (interrupted by signal 6: SIGABRT)
I am not exacty sure what valgrind changes that the code suddenly brakes,
and to be 100% I am not saying that the above issue has anything to od with
the OS X problem.
Just my 3 cents, take it easy.
PS valgrind options: /usr/bin/valgrind --tool=memcheck --xml=yes
--xml-file=/tmp/valgrind --gen-suppressions=all --leak-check=full
--leak-resolution=med --track-origins=yes --vgdb=no <path>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#996 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAjUjhHy2hn73GfoKGFNS9gj32DU1wAzks5u95cNgaJpZM4YNYVu>
.
|
Collaborator
|
Following @jdumas' suggestion, here is the output from address sanitizer: |
Collaborator
|
Another interesting observation: The code also seems to be working in single thread mode (achieved by setting the following environment variable). export LIBIGL_NUM_THREADS=1We already have locks for each vertex and each triangle... Maybe CGAL is caching some computed quantity (e.g. |
Collaborator
|
Ah, I know! 😎 We are locking each triangle and each vertex assuming all triangles and vertices are separate entities in memory! However, it is not true for the following line: igl::copyleft::cgal::CSGTree inter(tree, tree, "i");where two different triangles could be mapped to the same memory location... So, when we are locking triangle @alecjacobson Any thought on this? |
Contributor
|
Interesting. Admittedly it will be rare to do exactly this operation. But it is conceivably very possible to merge CSG trees that reference the same primitives... Ultimately this comes down to the non-read safe nature of CGAL's Epeck type. Do we have an easy way to turn multi-threading on or off? |
Collaborator
|
The easiest way is to set the environment variable |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It just occurred to me that since CGAL is now disabled by default, we need to explicitly enable it on Travis and AppVeyor...
Check all that apply (change to
[x])