[registration] Possible incorrect use of transformation_epsilon in ICP

[themightyoarfish]

I'm not 100% sure if this is a bug, considering the code has been there for 11 years, but I'm wondering if this code is wrong:

pcl/registration/include/pcl/registration/impl/icp.hpp

Line 163 in 6c23c85

convergence_criteria_->setRotationThreshold(1.0 - transformation_epsilon_);

It uses the transformation_epsilon_ value, which is otherwise used as convergence criterion for the translational part of the transform. But if transformation_rotation_epsilon_ is negative (default of -1), it is instead put into an expression for the rotational epsilon. How does this make sense? The translational epsilon may even exceed the possible value range of cos(), and even if it doesn't this seems to mix units.

Context

There is no concrete problem I have, but I stumbled upon this and it seems wrong, but I may be missing something.

The code was added here ca930cd and has been there since, so I suspect I just do not understand what it is supposed to do.

Expected behavior

n/a

Current Behavior

n/a

[mvieth]

I don't think this is a big problem, but I agree that it is strange. We could just delete the else case: then, if transformation_rotation_epsilon_ is unset, convergence_criteria_ will use its default value for the rotation threshold (0.99999). Or do you have another suggestion?

[themightyoarfish]

That would seem to be the intuitive change here, but there must be some reason why this code is here, as it seems nonsensical.

[mvieth]

@themightyoarfish Maybe this line was added before transformation_rotation_epsilon_ existed, so transformation_epsilon_ was used for both?
A previous PCL maintainer apparently also had doubts about that line: #1724 (comment)
I don't think there is a particular reason for that code. I am fine with removing the else case, feel free to open a pull request.

[themightyoarfish]

#5755

[mvieth]

Fixed by #5755