Breaking change in uniform sampling

[paulhilbert]

Is there a specific reason to break the uniform sampling? I understand it has been moved to filters, yet it does not implement the filter interface (it completely ignores the interface for retrieving sampled indices - in fact it even doesn't call the filter constructor).

To be clear: Getting the indices is the point of subsampling (at least in all my use cases).

Your Environment

• Operating System and version: Irrelevant.
• Compiler: Irrelevant.
• PCL Version: Master branch.

Expected Behavior

Please unbreak the uniform sampling.

Changing the uniform sampling interface broke existing functionality without apparent benefits. You should maybe consider more closely evaluating breaking pull requests.

Context

The interface change breaks roughly 6-7 of my point cloud processing libraries and my entire toolchain.

[taketwo]

Hi, sorry for the late reply. Can you please be more specific, which functionality (i.e. which function) worked before and does not work anymore? I'm not very familiar with the class in question, but looking at the commit diff I don't see any functionality being removed.

For reference, the PR in question is #1411 and the respective author is @fran6co (ping).

[fran6co]

The main change is that UniformSampling inherits from Filter instead of Keypoint. That makes it use filter instead of compute to actually apply the filter. Maybe an easy fix would be to add a compute method that forwards to filter.

[SergioRAgostinho]

The main change is that UniformSampling inherits from Filter instead of Keypoint. That makes it use filter instead of compute to actually apply the filter. Maybe an easy fix would be to add a compute method that forwards to filter.

If I understood correctly, I thing the biggest critique @paulhilbert was mentioning is that the removed indices are not being properly maintained in removed_indices_ so that he can use the index related methods in the filter base class.

Adding a method (compute) just to declare it deprecated and later remove it, is not worthy at this point. We already broke the interface and this is settled. @paulhilbert point is valid though. It needs to properly provide the indexes. We overlooked it.

[fran6co]

Ah, sorry. I overlooked the issue. I'll submit a fix for it.

[SergioRAgostinho]

@fran6co Thanks for taking the time to do it.

(Albeit probably a redundant request) Please be sure to fork from the most recent version of master. We just merged something to the CI pipeline roughly 40 minutes ago 😅

[taketwo]

Agree, no need to add compute.

As for the indices, before it was Keypoint::getKeypointsIndices that allowed to retrieve remaining subsampled indices, whereas the Filter interface has Filter::getRemovedIndices, which is the opposite. Implementing this will sure be better than nothing, but the codebases willing to work with either version of PCL will need to implement some sort of conversion. I wonder if we should rather incorporate this into the class and add something like getFilteredIndices.

[SergioRAgostinho]

I wonder if it wouldn't make more sense to simply overload the filter method for that. One of the versions just outputs the indices and another the a new pointcloud based on those indices. Maybe even one with both as well.

[taketwo]

This is an interesting idea. Would make sense to put these overloads in the base Filter class? Deriving classes will just need to implement protected applyFilter(PointIndices& output); all Filter::filter() variations can be implemented using its output.

Downsides: a lot of work, plus we will break custom user classes deriving from Filter.

[SergioRAgostinho]

Would make sense to put these overloads in the base Filter class? Deriving classes will just need to implement protected applyFilter(PointIndices& output); all Filter::filter() variations can be implemented using its output.

That was exactly the idea 👍 .

[taketwo]

Hm, another problem here will be that for many filters we have a specialization for PCLPointCloud2, which are typically some sort of copy/paste of the templated versions. I think getting everything straight will be a lot of work :(