Fix build against nanoflann >= 1.10 and SYCL+GCC libomp linkage - #2151
Fix build against nanoflann >= 1.10 and SYCL+GCC libomp linkage#2151MGRAFF2006 wants to merge 2 commits into
Conversation
nanoflann 1.10.0 casts the accessor to typename RESULTSET::IndexType in searchLevel(), so a result set class must publicly expose an IndexType typedef. The custom result sets SmallerPixSizeInRadius (fuseCut) and BestPointInRadius (importE57) only carried IndexType as a template parameter; publish it as a member typedef so AliceVision builds against nanoflann >= 1.10.
When AliceVision is compiled with GCC but the SYCL depthmap sources are compiled by AdaptiveCpp/clang (-fopenmp), the resulting library depends on LLVM's OpenMP runtime (libomp: __kmpc_* symbols). A bare '-lomp' link option does not reliably propagate to GCC-linked consumers, causing undefined references to __kmpc_fork_call / __kmpc_global_thread_num when linking executables (e.g. aliceVision_depthMapEstimation). Resolve libomp to a full path (searching the AdaptiveCpp/ROCm LLVM lib dirs when known) and link it explicitly via target_link_libraries so it propagates transitively; keep '-lomp' as a fallback.
There was a problem hiding this comment.
Code Review
This pull request updates nanoflann-related classes in Kdtree.hpp and main_importE57.cpp to support nanoflann 1.10.0 by exposing the IndexType typedef. It also enhances the CMake build system to locate and explicitly link LLVM's OpenMP runtime (libomp) when compiling with GCC and SYCL. The review feedback suggests avoiding reserved C++ identifiers starting with an underscore and an uppercase letter, utilizing the exposed IndexType directly in member function signatures, and properly initializing the CMake hints list to prevent empty string entries from affecting find_library.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| template<typename _DistanceType, typename _IndexType = size_t> | ||
| class SmallerPixSizeInRadius | ||
| { | ||
| public: | ||
| // Necessary since nanoflann 1.7.0: https://github.com/jlblancoc/nanoflann/commit/9c84c4c32c2bfa7077cc8873dd3b5bcbb557da90 | ||
| using DistanceType = _DistanceType; | ||
| // Necessary since nanoflann 1.10.0: result sets must expose an IndexType typedef | ||
| using IndexType = _IndexType; |
There was a problem hiding this comment.
In C++, identifiers starting with an underscore followed by an uppercase letter (such as _DistanceType and _IndexType) are reserved for the implementation in any scope. To avoid potential collisions with compiler internals or standard library headers, it is highly recommended to use standard names like DistanceT and IndexT for template parameters.
| template<typename _DistanceType, typename _IndexType = size_t> | |
| class SmallerPixSizeInRadius | |
| { | |
| public: | |
| // Necessary since nanoflann 1.7.0: https://github.com/jlblancoc/nanoflann/commit/9c84c4c32c2bfa7077cc8873dd3b5bcbb557da90 | |
| using DistanceType = _DistanceType; | |
| // Necessary since nanoflann 1.10.0: result sets must expose an IndexType typedef | |
| using IndexType = _IndexType; | |
| template<typename DistanceT, typename IndexT = size_t> | |
| class SmallerPixSizeInRadius | |
| { | |
| public: | |
| // Necessary since nanoflann 1.7.0: https://github.com/jlblancoc/nanoflann/commit/9c84c4c32c2bfa7077cc8873dd3b5bcbb557da90 | |
| using DistanceType = DistanceT; | |
| // Necessary since nanoflann 1.10.0: result sets must expose an IndexType typedef | |
| using IndexType = IndexT; |
| * @return true if the search should be continued, false if the results are sufficient | ||
| */ | ||
| inline bool addPoint(DistanceType dist, IndexType index) | ||
| inline bool addPoint(DistanceType dist, _IndexType index) |
There was a problem hiding this comment.
Since IndexType is now a public member typedef of the class, we can use it directly in the member function signature instead of the template parameter _IndexType. This is cleaner and avoids using reserved names with leading underscores.
| inline bool addPoint(DistanceType dist, _IndexType index) | |
| inline bool addPoint(DistanceType dist, IndexType index) |
| template<typename _DistanceType, typename _IndexType = size_t> | ||
| class BestPointInRadius | ||
| { | ||
| public: | ||
| // Necessary since nanoflann 1.7.0: https://github.com/jlblancoc/nanoflann/commit/9c84c4c32c2bfa7077cc8873dd3b5bcbb557da90 | ||
| using DistanceType = _DistanceType; | ||
| // Necessary since nanoflann 1.10.0: result sets must expose an IndexType typedef | ||
| using IndexType = _IndexType; |
There was a problem hiding this comment.
In C++, identifiers starting with an underscore followed by an uppercase letter (such as _DistanceType and _IndexType) are reserved for the implementation in any scope. To avoid potential collisions with compiler internals or standard library headers, it is highly recommended to use standard names like DistanceT and IndexT for template parameters.
template<typename DistanceT, typename IndexT = size_t>
class BestPointInRadius
{
public:
// Necessary since nanoflann 1.7.0: https://github.com/jlblancoc/nanoflann/commit/9c84c4c32c2bfa7077cc8873dd3b5bcbb557da90
using DistanceType = DistanceT;
// Necessary since nanoflann 1.10.0: result sets must expose an IndexType typedef
using IndexType = IndexT;| * @return true if the search should be continued, false if the results are sufficient | ||
| */ | ||
| inline bool addPoint(DistanceType dist, IndexType index) | ||
| inline bool addPoint(DistanceType dist, _IndexType index) |
There was a problem hiding this comment.
| # final link time. Resolve libomp to a full path (searching the | ||
| # AdaptiveCpp/ROCm LLVM lib dirs when known) and link it explicitly so it | ||
| # propagates to consumers; fall back to "-lomp" if it cannot be located. | ||
| set(_acpp_omp_hints "") |
There was a problem hiding this comment.
In CMake, set(VAR "") initializes the variable as a list containing a single empty string element "". When you later append elements using list(APPEND VAR ...), the list will start with an empty string, which can cause issues with commands like find_library that interpret empty strings as paths (often resolving to the current working directory). Using set(_acpp_omp_hints) properly initializes it as an empty list.
set(_acpp_omp_hints)
Description
Two small, independent build fixes needed to compile AliceVision (
develop) against current toolchains/dependencies. Both are isolated and behavior-preserving.1.
fix(deps): exposeIndexTypein custom nanoflann result setsnanoflann 1.10.0 casts the accessor to
typename RESULTSET::IndexTypeinKDTreeBaseClass::searchLevel():Two custom result-set classes only carried
IndexTypeas a template parameter and did not publish it as a member typedef, so building against nanoflann ≥ 1.10 fails with:Fix: rename the template parameter to
_IndexTypeand exposeusing IndexType = _IndexType;(mirroring the existingDistanceTypetypedef added for nanoflann 1.7). No behavior change.Affected files:
src/aliceVision/fuseCut/Kdtree.hpp(SmallerPixSizeInRadius)src/software/convert/main_importE57.cpp(BestPointInRadius)2.
fix(cmake): linklibompby full path for SYCL libraries built with GCCWhen AliceVision is built with GCC but the SYCL depthmap sources are compiled by AdaptiveCpp/clang (
-fopenmp), the resultingaliceVision_depthMap_sycllibrary depends on LLVM's OpenMP runtime (libomp,__kmpc_*symbols). The existingtarget_link_options(... "-lomp")does not reliably propagate to GCC-linked consumers, so linking executables fails:Fix: resolve
libompto a full path (searching the AdaptiveCpp/ROCm LLVM lib dirs viaAdaptiveCpp_DIR/ROCM_PATHwhen available) and link it withtarget_link_libraries(... PUBLIC ...)so it propagates transitively. Falls back to-lompif not found.Affected file:
src/cmake/Helpers.cmakeTesting
Built
developend-to-end on Arch Linux (GCC 16, nanoflann 1.10.1, AdaptiveCpp 25.10 + ROCm 7.2, SYCL depthmap backend) — full build now completes and produces workingaliceVision_*binaries includingaliceVision_depthMapEstimation.Notes
Marking as draft for maintainer review; happy to adjust the libomp discovery logic if there's a preferred pattern already used elsewhere in the CMake.