Skip to content

Extend BruteForce search to non-XYZ point types - #6457

Open
jeevan6996 wants to merge 6 commits into
PointCloudLibrary:masterfrom
jeevan6996:extend-bruteforce-nonxyz-points
Open

Extend BruteForce search to non-XYZ point types#6457
jeevan6996 wants to merge 6 commits into
PointCloudLibrary:masterfrom
jeevan6996:extend-bruteforce-nonxyz-points

Conversation

@jeevan6996

Copy link
Copy Markdown

Summary

  • add PointRepresentation support to BruteForce search
  • keep the existing fast XYZ distance path when the representation is trivial 3D
  • use full DefaultFeatureRepresentation dimensions for descriptor point types where available
  • allow autoSelectMethod to fall back to BruteForce when FLANN/Nanoflann are unavailable
  • add focused BruteForce tests for custom representations, FPFH descriptors, and sparse invalid descriptors

Fixes #6421.

Validation

  • configured a focused local build with search/tests enabled
  • built target: cmake --build /private/tmp/pcl-bruteforce-build-6 --target test_brute_force_search
  • ran: ctest --test-dir /private/tmp/pcl-bruteforce-build-6/test -R brute_force_search --output-on-failure
  • ran: git diff --check

@jeevan6996

Copy link
Copy Markdown
Author

The CI linker failures were caused by AutoSelectMethod being explicitly instantiated for every PCL_POINT_TYPES, while BruteForce was precompiled only for the XYZ and descriptor subsets. That left symbols undefined for types such as SHOT352, Normal, and PointUV when the fallback was selected.

Updated search/src/brute_force.cpp to instantiate BruteForce for PCL_POINT_TYPES, keeping both instantiation sets aligned. Verified locally with Homebrew LLVM: pcl_search builds, test_brute_force_search links, and all 3 focused tests pass.

Comment thread search/include/pcl/search/brute_force.h Outdated
@jeevan6996

Copy link
Copy Markdown
Author

Thanks. I checked the other search implementations and they default to DefaultPointRepresentation, which is limited to the first three float values unless a type-specific specialization exists.

This condition is intentional for BruteForce’s new non-XYZ descriptor support: types with descriptorSize need DefaultFeatureRepresentation so all descriptor dimensions are included in distance calculations. XYZ and ordinary point types retain the existing default behavior, and callers can still override the representation explicitly. Tests cover both descriptor and custom representations.

Comment thread search/include/pcl/search/impl/brute_force.hpp Outdated
@mvieth

mvieth commented Aug 14, 2026

Copy link
Copy Markdown
Member

Thanks. I checked the other search implementations and they default to DefaultPointRepresentation, which is limited to the first three float values unless a type-specific specialization exists.

This condition is intentional for BruteForce’s new non-XYZ descriptor support: types with descriptorSize need DefaultFeatureRepresentation so all descriptor dimensions are included in distance calculations. XYZ and ordinary point types retain the existing default behavior, and callers can still override the representation explicitly. Tests cover both descriptor and custom representations.

I just checked all types for which specializations of descriptorSize exist, and I identified several types where specializations of DefaultPointRepresentation exist and it is thus wrong to use DefaultFeatureRepresentation (e.g. ShapeContext1980). So I would suggest to just use DefaultPointRepresentation, not DefaultFeatureRepresentation, in BruteForce, to stay consistent with KdTree search and others.

But I have also identified four types which are missing a specialization of DefaultPointRepresentation in point_representation.h, so the following should be added to point_representation.h:

  template <>
  class DefaultPointRepresentation<GRSDSignature21> : public DefaultFeatureRepresentation <GRSDSignature21>
  {};

  template <>
  class DefaultPointRepresentation<BRISKSignature512> : public DefaultFeatureRepresentation <BRISKSignature512>
  {};

  template <>
  class DefaultPointRepresentation<ESFSignature640> : public DefaultFeatureRepresentation <ESFSignature640>
  {};

  template <>
  class DefaultPointRepresentation<GFPFHSignature16> : public DefaultFeatureRepresentation <GFPFHSignature16>
  {};

Can you do that? Thanks.

@mvieth

mvieth commented Aug 15, 2026

Copy link
Copy Markdown
Member

Please also change 512 to 66 here, then the tests should pass:

template<> struct descriptorSize<BRISKSignature512> { static constexpr const int value = 512; };

Seems like there has been a mistake when detail::traits::descriptorSize was added ( d39d3d3 ).

Comment thread search/include/pcl/search/brute_force.h Outdated
Comment thread common/include/pcl/impl/point_types.hpp Outdated
@mvieth

mvieth commented Aug 18, 2026

Copy link
Copy Markdown
Member

I have to close and reopen this pull request, to make all the checks on Azure pipelines run. Somehow they did not run for your latest commit, probably due to GitHub's outage yesterday evening

@mvieth mvieth closed this Aug 18, 2026
@mvieth mvieth reopened this Aug 18, 2026
@jeevan6996

Copy link
Copy Markdown
Author

Thanks for the detailed review. The latest revision now uses DefaultPointRepresentation consistently, adds the missing feature specializations, performs representation-based validity checks inline, and treats BRISK as a 64-value descriptor with a dedicated representation. The reopened Azure pipeline completed successfully across GCC, Clang, macOS, Windows, and formatting checks. I have resolved the outdated threads corresponding to these changes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Help wanted: Extend BruteForce search to point types without xyz

3 participants