Pcl: Improve documentation and implementation of SampleConsensusModelStick

[alexvorndran]

Your Environment

• Operating System and version: Linux Mint 18.3
• Compiler: gcc 5.4
• PCL Version: 1.8.1 built from GitHub releases page

Context

I was using pcl::SACSegmentation with setMethodType(pcl::SACMODEL_STICK) and looked for an interpretation of the model coefficients. The documentation of SampleConsensusModelStick tells us to interpret them as origin and direction of the line. However, the usage of the model coefficients in parts of the implementation as well as the commit message of 5f0c934736a614bbe2e3b2843d5929f6faec8a06 indicate that the model coefficients indeed seem to represent two points describing the line.

Current Behavior

The "public" documentation is not consistent with the implementation. There are even inconsistencies in how the actual implementation treats the model parameters. At the moment computeModelCoefficients, selectWithinDistance and some other methods seem to use the model coefficients in the way described in 5f0c934736a614bbe2e3b2843d5929f6faec8a06. Others like getDistanceToModel or projectPoints were left untouched and still assume the old behavior.

Expected Behavior

The documentation should be consistent with the implementation.

Furthermore, the implementation should use the same behavior at all points.

Code to Reproduce

Some relevant code pieces for this issue are linked in the description above. However, the linked code pieces do not cover all places in the implementation dealing with the model coefficients. I'm a little short on time at the moment so I'm not able to provide an exhaustive list for all methods affected by this issue.

Possible Solution

The code and the implementation should be fixed to a coherent state.

[SergioRAgostinho]

Thanks for reporting. Since you're going in depth into SampleConsensusModelStick at the moment please consider submitting a Pull Request with the appropriate corrections.

[alexvorndran]

Before working on the issue there should be consens which of the two variants is supposed to be used in the implementation and as a consequence be documented.

@SergioRAgostinho What's your oppinion on this?

[taketwo]

Generally, I would say keep the behavior and adjust the documentation, otherwise someone's code may be broken. In this case, however, if I understand you correctly, this class has been broken for many years, so it's hard to imagine someone relies on it. So I think the road is clear for using either of the implementations.

I should admit I've never used this model. How is it different from the cylinder model?

[alexvorndran]

@taketwo Yeah it seems like the class is inconsistent since that commit in 2011.

To be honest, I'm not sure how the model is different from the cylinder model. I mainly chose the stick model because the concept of a "stick" (height >> diameter) seemed to be a better fit for the object I was trying to segment from the point cloud. So I will have to look into that.

Reverting it to the older line and direction coefficients would bring the implementation in line with the cylinder model (impl) you mentioned as well as with the line model (impl), I think.

[alexvorndran]

Broken uses of the model coefficients

Broken in the sense that their use of the model coefficients contradicts computeModelCoefficients.

• getDistancesToModel (used e.g in LMEDS, MLESAC, MSAC, RMSAC)
• projectPoints (used e.g. pcl::ProjectInliers<PointT>::applyFilter)
• doSamplesVerifyModel (used e.g. in RMSAC, RRANSAC)
• probably also optimizeModelCoefficients (correctly interpreting an eigenvector as point seems very unlikely)
• (the documentation)

Usage in line with computeModelCoefficients

• computeModelCoefficients (of course)
• selectWithinDistance
• countWithinDistance

[alexvorndran]

@taketwo I think the main difference is supposed to be that the stick model can work without normals.

But the implementation also does not seem to compute a radius/width value for the stick (at the moment the radius model coefficient value is [probably] uninitalized after resize) nor does is validate the value against the radius limits given by the user.

[taketwo]

In the old SVN times devs were committing their in-progress work directly to master. Seems like this stick model is such a half-baked feature. What would you propose: remove entirely not to mislead future users, or somehow fix?

[alexvorndran]

I had time to fix the model coefficients in my fork so that they work (mostly) consistent to the documentation and all the other similar models again. At the moment there is just a negative signaling value for the radius telling you that something is not quite right here, ~so in essence, the model boils down to the line model~.

[alexvorndran]

As a follow up to previous comment I would like to add that there are indeed some subtle differences between the stick and the line model. In my application scenario the stick model seems to be more stable and produce "better" results.

This definitively needs a closer look.

[taketwo]

I guess you are now the leading stick model expert. Please update us with your further findings so we can decide what to do with it.

[stale[bot]]

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