nmwsharp / polyscope

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

Clarify directory structure of installed headers #225

Closed phaedon closed 1 year ago

phaedon commented 1 year ago

This question came up on https://groups.google.com/g/bazel-discuss/c/e8wUEQrDNoE/m/P4tQdO23AgAJ

I am running into a problem wherein the Polyscope headers reference the "render/" subdirectory when including certain headers, for example:

#include "polyscope/render/managed_buffer.h"
#include "polyscope/render/color_maps.h"
#include "polyscope/render/engine.h"

However, the installed headers (in /usr/local/include/polyscope, via cmake) are all in a flattened directory, there is no "render/" subdirectory, and this causes errors when trying to build this minimal/toy example.

I'm not very familiar with cmake and am wondering how the compiler is supposed to resolve a path like

include "polyscope/render/engine.h"

referenced in structure.h, when the render/ subdir has been eliminated.

nmwsharp commented 1 year ago

Hi! Thanks for flagging this.

I suspect this is actually a bug with our CMake INSTALL target. It should not be flattening the directory, it should be preserving the directory structure. (I always use Polyscope by naively adding it as a subproject, rather than installing it at the system-level, to explain how such a blatant bug could go unnoticed...)

It looks like we may need to use different CMake commands to get the INSTALL target to preserve the directory structure (see e.g. https://stackoverflow.com/questions/48212771/cmake-install-header-files-and-maintain-directory-hierarchy).

I'll look it to this soon, sounds like an annoying but easy-to-fix bug.

nmwsharp commented 1 year ago

And to clarify a little more, I don't know anything about Bazel unfortunately, or how one ordinarily adds a CMake project to a Bazel build. But this directory structure issue definitely sounds like a bug in our CMake script.

nmwsharp commented 1 year ago

This should be fixed in 6c1c490. Please reopen if not.