Extend BruteForce search to non-XYZ point types - #6457
Conversation
Signed-off-by: jeevan6996 <jeevanpawar5890@gmail.com>
|
The CI linker failures were caused by Updated |
| if constexpr (!pcl::traits::has_xyz_v<PointT> && has_descriptor_size<PointT>::value) | ||
| return (pcl::make_shared<DefaultFeatureRepresentation<PointT>> ()); | ||
| else | ||
| return (pcl::make_shared<DefaultPointRepresentation<PointT>> ()); |
There was a problem hiding this comment.
Thanks for this pull request. Could you please explain why you did this condition, since none of the other search methods do it like this?
|
Thanks. I checked the other search implementations and they default to This condition is intentional for BruteForce’s new non-XYZ descriptor support: types with |
| template <typename PointT> bool | ||
| pcl::search::BruteForce<PointT>::isValidPoint (const PointT& point) const | ||
| { | ||
| if constexpr (pcl::traits::has_xyz_v<PointT>) |
There was a problem hiding this comment.
The new custom-representation path is not used by isValidPoint() for point types with XYZ traits:
if constexpr (pcl::traits::has_xyz_v<PointT>)
return (pcl::isFinite (point));This still validates x/y/z even when setPointRepresentation() installs a representation that intentionally uses a different subset of fields. For example, a representation that uses only x/y should be able to search a point whose z is NaN, but this causes the query assertion to fail (and causes sparse searches to skip the point). This also differs from the representation-based validity behavior used by the other search implementations.
Could this use point_representation_->isValid(point) consistently, with a regression test using a custom representation that ignores an invalid XYZ field?
There was a problem hiding this comment.
Sounds good to me, then there is even no point in having the isValidPoint function, instead point_representation_->isValid(point) can be checked inline.
I just checked all types for which specializations of But I have also identified four types which are missing a specialization of 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. |
|
Please also change 512 to 66 here, then the tests should pass: pcl/common/include/pcl/impl/point_types.hpp Line 169 in 4dd5652 Seems like there has been a mistake when |
| public: | ||
| BruteForce (bool sorted_results = false) | ||
| : Search<PointT> ("BruteForce", sorted_results) | ||
| , point_representation_ (detail::makeDefaultPointRepresentation<PointT> ()) |
There was a problem hiding this comment.
| , point_representation_ (detail::makeDefaultPointRepresentation<PointT> ()) | |
| , point_representation_ (new DefaultPointRepresentation<PointT>) |
I think there is not really a point in having the makeDefaultPointRepresentation function any more, so I would suggest removing it.
Summary
Fixes #6421.
Validation
cmake --build /private/tmp/pcl-bruteforce-build-6 --target test_brute_force_searchctest --test-dir /private/tmp/pcl-bruteforce-build-6/test -R brute_force_search --output-on-failuregit diff --check