Skip to content

Conversation

@rishabhbhutani013
Copy link

No description provided.

@DavidJourdan
Copy link
Contributor

Hi, I think a bit of context would help the devs who maintain this library :)

I assume this is meant to fix the same bug that I encountered: on Windows the symlink creation fails with an error like so:

file Failed to create link
'C:/.../build/deps/include/nanoflann/nanoflann.hpp'
because existing path cannot be removed: No error

Your solution to make a copy instead could work, but I think you forgot the DESTINATION word, should be something like that:

file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/nanoflann/include/nanoflann.hpp
     DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/include/nanoflann/nanoflann.hpp)

@nmwsharp
Copy link
Owner

Hi all, I'm clearing out old PRs, pardon for not addressing this sooner.

I don't quite understand what's going on here. I guess this code was added in case users depend on nanoflann.hpp living at that location, so after moving it we added a copy step at CMake-time, but that copy is failing on some Windows builds? Can you confirm? Maybe it would be better to just delete this and bump the version?

CC @qnzhou who it seems wrote this line originally, in case you have any opinions on this.

@qnzhou
Copy link
Contributor

qnzhou commented Sep 16, 2025

This is related to #157

The issue is about the nanoflann include path. In geometry central, it is sometimes included as the following:

#include "nanoflann/nanoflann.hpp"

which assumes nanoflann.hpp is in a nanoflann folder. This is not the case in the vanilla nanoflann repo: https://github.com/jlblancoc/nanoflann/tree/master/include

If an external project (e.g. Lagrange) uses both nanoflann and geometry-central, it will lead to a header file not found error.

After thinking it a bit more, I feel it is better to consistently use the following include path in geometry central:

#include "nanoflann.hpp"

This way, we do not need to do any cmake magic to have compatible include path.

@qnzhou
Copy link
Contributor

qnzhou commented Sep 16, 2025

Just to add, #208 is in the same boat, just for nanort instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants