RadiusOutlierRemoval<PCLPointCloud2> implementation is slow and confusing

[1r0b1n0]

Hi,

Context

I had some troubles with the implementation of the RadiusOutlierRemoval<PCLPointCloud2> filter :
it doesn't have the same behavior as the templated RadiusOutlierRemoval<PointT> implementation.
I wrote the following unit-test to show the problem :

Code to Reproduce

GTEST_TEST(OutlierRemovalTest, SimpleTest)
{
    using namespace pcl;

    PointCloud<PointXYZ>::Ptr pc_in(new PointCloud<PointXYZ>());
    pc_in->push_back(PointXYZ(1.f, 0.f, 0.f));
    pc_in->push_back(PointXYZ(2.f, 0.f, 0.f));
    pc_in->push_back(PointXYZ(3.f, 0.f, 0.f));

    PCLPointCloud2::Ptr pc2_in(new PCLPointCloud2());
    toPCLPointCloud2(*pc_in, *pc2_in);

    {
        RadiusOutlierRemoval<PointXYZ> filter;
        filter.setInputCloud(pc_in);
        filter.setRadiusSearch(1.1);
        filter.setMinNeighborsInRadius(2);
        PointCloud<PointXYZ> pc_out;
        filter.filter(pc_out);
        EXPECT_EQ(pc_out.size(), 1);
        EXPECT_NEAR(pc_out.at(0).x, 2.f, 1e-4f);
        EXPECT_NEAR(pc_out.at(0).y, 0.f, 1e-4f);
        EXPECT_NEAR(pc_out.at(0).z, 0.f, 1e-4f);
    }

    {
        RadiusOutlierRemoval<PCLPointCloud2> filter;
        filter.setInputCloud(pc2_in);
        filter.setRadiusSearch(1.1);
        // the implementation seems to consider itself as a neigbour, the test only works if
        // we change the next line to filter.setMinNeighborsInRadius(3);
        filter.setMinNeighborsInRadius(2);
        PCLPointCloud2 pc2_out;
        PointCloud<PointXYZ> pc_out;
        filter.filter(pc2_out);
        fromPCLPointCloud2(pc2_out, pc_out);
        EXPECT_EQ(pc_out.size(), 1); // this will fail
        EXPECT_NEAR(pc_out.at(0).x, 2.f, 1e-4f);
        EXPECT_NEAR(pc_out.at(0).y, 0.f, 1e-4f);
        EXPECT_NEAR(pc_out.at(0).z, 0.f, 1e-4f);
    }
}

Current Behavior

This test fails for the RadiusOutlierRemoval<PCLPointCloud2> filter.

Moreover the RadiusOutlierRemoval<PCLPointCloud2> implementation is a lot slower in my test cases :
For a dataset of ~12000 points from a 3D Lidar I measured the following timings :

BM_RadiusOutlierRemoval<PointXYZI>             18.8 ms
BM_RadiusOutlierRemoval<PCLPointCloud2>        143 ms

Possible Solution

It is confusing to have two different implementations for the same filter.
I think the best solution that doesn't break the API for existing projects would be marking the RadiusOutlierRemoval<PCLPointCloud2> implementation as deprecated, explaining RadiusOutlierRemoval<PointT> should be used instead.
If you agree with this change I can make a pull request.

[SergioRAgostinho]

Thank you for exposing the case. It looks indeed like a bug. Please include the unit test you wrote in the resulting PR from this issue.

It is confusing to have two different implementations for the same filter.

Agreed and I believe the general guideline now is to not extend support to algorithm specializations of PCLPointCloud2. @PointCloudLibrary/maintainers should we deprecate and replace and wrap the buggy implementation on top of the working one?

I'm ok in doing so with the objective of reducing maintenance.

[taketwo]

I'm all in favor of reducing code duplication, confusion, and maintenance burden. I'm not sure, however, that these specializations should be deprecated. For example, @stefanbuettner commented here:

Hey all, recently I was very happy to see that there are specializations of many algorithms for PCLPointCloud2

Stefan, could you perhaps explain why these were important for you?

[stefanbuettner]

Hey there,

it's been a long time but I think I was using PCL in a ROS node where I received a PCLPointCloud2. With this specialization no conversion (copy) to PointCloud<PointT> and back to PCLPointCloud2 was required. And I guess I was glad that the whole processing chain I needed was available for PCLPointCloud2.

[stale]

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

[kunaltyagi]

The PointCloud2 impl uses PointXYZ overload for the RadiusOutlierRemoval<PointT> and so, IMO, the internals of specialization should be rewritten to use the original case.

[kunaltyagi]

With this specialization no conversion (copy) to PointCloud and back to PCLPointCloud2 was required.

Sadly, this isn't true wrt the actual impl. There's a copy inside the implementation, but the overall class provides good value in a processing pipeline