Improve documentation and implementation of SampleConsensusModelStick

[alexvorndran]

trafficstars

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.

[mvieth]

@alexvorndran Based on what I saw so far, I would suggest that we deprecate and at some point remove the stick model. Not only that the model coefficients are treated inconsistently (2 points vs 1 point+line direction), the last model parameter (width) is never used, so this is basically a line model. The width can't even be computed from the 2 sample points. Then there is also some strange logic in getDistancesToModel and countWithinDistances that introduces a special penalty for outliers, but selectWithinDistance does not have this, neither does any other samples consensus model (to my knowledge). I don't see a reason why someone would want to use the stick model instead of the line model or maybe the cylinder model.

We could start with a "soft" deprecation in sac_segmentation.hpp and in the documentation of sac_model_stick.h.

I also posted a question in the Discord community chat whether anyone has ever used the model, let's see whether someone answers.

[alexvorndran]

@mvieth I follow most of your observations, but would like to mention that in the cylinder model the radius is also determined based on only two sample points (and some normal magic - at least I think so :sweat_smile:). So I guess maybe one could come up with something based on the inlier distances. But to be honest: I'm not sure if that would add significant value :man_shrugging:

I originally went for "stick" because I had a very slim, pipe-like object (width ~3cm) I wanted to segment with not a lot of neighbors to compute normals for the cylinder model, so stick sounded like a great idea. Well... here we are :laughing:

[mvieth]

@mvieth I follow most of your observations, but would like to mention that in the cylinder model the radius is also determined based on only two sample points (and some normal magic - at least I think so sweat_smile). So I guess maybe one could come up with something based on the inlier distances. But to be honest: I'm not sure if that would add significant value man_shrugging

You are right that the cylinder model is also only computed with 2 samples, but there the samples have normals (as you mentioned). The stick model is a model that does not use normals (subclass of SampleConsensusModel, but not of SampleConsensusModelFromNormals). I don't see how we could compute a radius from just two samples without normals. We could make the stick model a model with normals like the cylinder model, but it is still unclear (at least to me :wink: ) what the stick model is supposed to be. For very thin objects, the estimated normals will be quite unstable.

I originally went for "stick" because I had a very slim, pipe-like object (width ~3cm) I wanted to segment with not a lot of neighbors to compute normals for the cylinder model, so stick sounded like a great idea. Well... here we are laughing

Ah, interesting. My guess would be that the line model with a bit of threshold-parameter-tuning would work for that scenario, no?

Another thing is, most if not all models could be computed from samples with normals and samples without normals. However when normals are available, fewer samples are necessary. E.g. without normals, 3 samples are necessary to compute a plane model, but with normals, only 1 sample would be necessary (not implemented in PCL, or only in the CUDA module, I think). 2 samples with normals are enough to compute a cylinder model. When no normals are available, there should still be a formula to compute a cylinder model, but with more samples (maybe 4 or 5? not sure. Currently not implemented in PCL)

[alexvorndran]

Ah, interesting. My guess would be that the line model with a bit of threshold-parameter-tuning would work for that scenario, no?

Yes, you are right. This is how I did it in the end and how it works till this day 😉 Therefore I won't shed a lot of tears if stick gets deprecated/removed. Given the model's state at the moment, I cannot imagine lots of people using it productively.