PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.86k stars 4.61k forks source link

Adjust code to match current FLANN API #3234

Open SunBlack opened 5 years ago

SunBlack commented 5 years ago

With FLANN 1.8 they switched from int to size_t. PCL is still using int, so there is always an implicit conversion.

I tried to fix it in context of CLang-Tidy warning performance-implicit-conversion-in-loop here: https://github.com/PointCloudLibrary/pcl/blob/816391762918f3a026b1822fe42c74bb83129eb4/registration/include/pcl/registration/impl/ppf_registration.hpp#L108

But after this I had to change a lot of further PCL code, so I stopped my work after I had already ~100 lines adjusted, because I don't know if you want such a big change before release of 1.10.

Necessary adjustments:

Some things you should notice about this:

And in general: Increase required FLANN version in CMake from 1.7 to 1.8.

taketwo commented 5 years ago

I don't think we want to change vector<int> to vector<size_t> at all. This will break incredible amount of user code.

And in general: Increase required FLANN version in CMake from 1.7 to 1.8.

Why is that? Are they incompatible?

SunBlack commented 5 years ago

Why is that? Are they incompatible?

FLANN 1.8 was released at end of 2012, so it is already really old. And if we switch to size_t user may not notice, that there are still function, which can't handle big data, because they are still using int internally. To prevent such issue, I would drop support of FLANN 1.7.

stale[bot] commented 4 years ago

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

kunaltyagi commented 4 years ago

Reviving (relatively) old discussion. FLANN 1.7 is no longer considered supported.

@sunblack Are there changes that still need to be made? Will our migration from int to index_t cover all of them?

stale[bot] commented 4 years ago

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.