search

nmwsharp / polyscope

A C++ & Python viewer for 3D data like meshes and point clouds
https://polyscope.run
MIT License
1.76k stars 190 forks source link

Update CMake dependencies to rely on proper targets #261

Closed jdumas closed 5 months ago

jdumas commented 6 months ago
jdumas commented 5 months ago

Done cleaning up this PR after the merge of #260. Should be much easier to review @nmwsharp. Note that the main reason to update to glm 0.9.9.8 was to use their CMake target directly. Updating to 1.x versions break compilation so I refrain from doing that.

nmwsharp commented 5 months ago

Thanks!

I'm a little wary about unnecessarily breaking things for people who might be e.g. using Polyscope's copy of json or something (even though this technically wasn't an intended use of the build setup). I'm also worried about what I'm always worried about with CMake, which is that it's not clear to me how to write clean proper CMake scripts, but also not break everyone's ancient dusty CMake setups in their own projects!

Regardless, I tried rebuilding so of my own projects with this and it all seems to work, so I say let's do it. The change to using targets and potentially pulling them from the outer project is quite useful.

nmwsharp commented 5 months ago

Merged! (Also we're about to get a bunch of issues about glm being broken, I need a keyboard macro that auto-replies "run git submodule update --init --recursive" 😄 )

nmwsharp commented 5 months ago

By the way, can you explain more about what glm 1.x breaks? As long as we're changing the build setup, that's a pretty good time to bite the bullet and update glm if the incompatibility isn't too bad.

jdumas commented 5 months ago

Thanks for merging this quickly!

which is that it's not clear to me how to write clean proper CMake scripts

Oh man, I could go on for a while on this topic... Very briefly, I'd break it down to two items:

  1. Treat CMake code as an object-oriented language, where CMake targets are your "objects", and rely heavily on target properties rather than CMake variables. CMake variables are very brittle (between various policies, cache variables, scoping to functions/directories, etc.), whereas target and their properties are much more reliable. This was one Daniel Pfeifer's point in his 2017 talk "Effective CMake" (the first 45min of this are great, after that it gets into the weeds of packaging, which is hard).
  2. Don't be afraid to use bump CMake version to benefit from their latest features. Because of their emphasis on strong backward compatibility (via CMake policies), updating the min required CMake version will never break an old project. But leveraging their latest feature can simplify a lot of things. See this list for a highlight of the new features in recent CMake versions. E.g. I now use CMake 3.25 so I can set CMAKE_MSVC_DEBUG_INFORMATION_FORMAT on Windows, which helps my project play nicely with ccache or address sanitizer on Windows.

Anyhow if you have more specific concerns I'm always happy to answer any CMake question :D

Also we're about to get a bunch of issues about glm being broken, I need a keyboard macro that auto-replies "run git submodule update --init --recursive" 😄 )

Yep, this is why I've switched to FetchContent/CPM for dependency management in CMake...

By the way, can you explain more about what glm 1.x breaks? As long as we're changing the build setup, that's a pretty good time to bite the bullet and update glm if the incompatibility isn't too bad.

Hmm I encountered some compilation errors at the time, so I bumped it down to 0.9.9.8. But now that I'm trying again it seems to work right away... the only thing I had to do was define GLM_ENABLE_EXPERIMENTAL more broadly (e.g. it's now needed in some of the functions used in the MarchingCube target). So I've opened https://github.com/nmwsharp/polyscope/pull/263 if you want to update GLM to the very latest version.