[sample_consensus] `Sac::getInliers` should return the results as a const reference instead of to a parameter

[SunBlack]

Just found this

https://github.com/PointCloudLibrary/pcl/blob/20464ce026619a04d91eb2e1cc7797bc284d4fe5/sample_consensus/include/pcl/sample_consensus/sac.h#L309-L310

Compared to this, the GPU module is already fine (apart from the slightly different type between both modules)

https://github.com/PointCloudLibrary/pcl/blob/20464ce026619a04d91eb2e1cc7797bc284d4fe5/cuda/sample_consensus/include/pcl/cuda/sample_consensus/sac.h#L154-L155

Note: Sames goes for getModelCoefficients

[mvieth]

@SunBlack Hi, could you elaborate on this? Why should the functions return a const reference?

[SunBlack]

With the current method, a copy is forcibly created (inliers_ is copied to inliers). However, if inliers_ is returned as a const reference, you can work directly on inliers_ unless you want to modify it. This leaves it up to the user of the method to decide whether a copy is necessary or not.

[mvieth]

Ah okay. that makes sense. Feel free to create a pull request to add the new functions, if you like. I can also do that sometime, but can't promise when. I would suggest to only add the additional functions but not deprecate or remove the existing getInliers/getModelCoefficients. It is good to give users the option to avoid the copy, but I think that is not enough reason to force users to switch. However, we could replace all usages within PCL.

[SunBlack]

This is not a priority for us. I just noticed that as we are currently removing the last remnants of the PCL from our code (as usage has fallen sharply in recent years), I haven't touched my open PRs and issues for a long time (which may already be obsolete).

[jose-n9]

Is this still available to create a pull request for getInliers?

[mvieth]

@jose-n9 Hi! Yes, it is still available. If you would like to create a pull request, please also add new functions for getModel and getModelCoefficients.

[jose-n9]

@mvieth I created a pull request, thank you

[mvieth]

@jose-n9 I requested some changes on the pull request, please address them. Thanks.

[mvieth]

I closed https://github.com/PointCloudLibrary/pcl/pull/6251 due to inactivity and failing tests. But this issue is still open, if anyone else wants to take this up. (if anyone does, please make sure to not replace the existing functions, just add the new, additional functions)