libigl / libigl.github.io

Repository for the libigl website, online tutorial and documentation
http://libigl.github.io
4 stars 30 forks source link

Update docs/static-library.md #19

Closed BruegelN closed 4 years ago

BruegelN commented 5 years ago

I'm not sure about the embree part, because I don't use embree personaly. However I think it's not relevenat anymore. Embree always compiles as static lib when LIBIGL_WITH_EMBREE is set to true: https://github.com/libigl/libigl/blob/master/cmake/libigl.cmake#L269-L295

Shoudn't the Extra section better be integrated into External Dependencies?

What about the Compressed .h/.cpp pair part? This is outdated an can be deleted, right?

Just let me know what you think such that I can update this PR accordingly.

jdumas commented 5 years ago

Yeah this page definitely needs an update!

Some comments:

jdumas commented 5 years ago

Hmm so if I type make install it will indeed correctly install libigl into the right folder, but it will also install embree, even though we don't really support installing the igl::embree module at the moment. This is annoying, because there is no way to avoid Embree's CMake calling the install() on its own targets. However, it looks like add the EXCLUDE_FROM_ALL option to the line

add_subdirectory("${EMBREE_DIR}" "embree" EXCLUDE_FROM_ALL)

in libigl.cmake does the trick, and make install will only install the main libigl target!

BruegelN commented 5 years ago

Thank you for your detailed feedback @jdumas! I've included most of the changes proposed by you.

The "External Dependencies" page definitely needs a rework as well! But I don't want to include it into this PR to keep the scope somewhat limited and I'm not very experienced with most the external libs. But I'll add a second PR later for the page about the libigl example project

Regarding installation with make install to CMAKE_INSTALL_PREFIX: Do you want me to added ad PR to the main libigl-repo with your fix? I could test it on macOS and a Ubuntu machine beforehand. Or just leave it for now?


@alecjacobson I've deleted a few section which looked outdated to me:

jdumas commented 5 years ago

Awesome, thanks! Yes maybe you could also create a PR to add the EXCLUDE_FROM_ALL in the libigl repo, so we can update the doc accordingly.

jdumas commented 4 years ago

Merged! Sorry it took a while. Appreciate your effort in improving libigl :D