Added option to specify translation and rotation convergence deltas in ICP and NDT algorithms. #1724
Conversation
ICP and NDT algorithms.
|
👍 Also, I wonder if we need this fallback threshold value: |
I still think this is something we should do. |
|
I agree. |
|
This was a very premature merge. I didn't review the thing properly :/ |
|
Oups sorry, must have been too early ;). For me it looked fine, as it's backward compatible for now. |
SergioRAgostinho
added
changelog: ABI break
|
Hello, this merged PR is not good. First,the following line is redundant and confusing because the following code is enough. or Secondly, it is wrong to use |
|
Create an issue to track this. Let's continue the conversation on the issue |
This PR resolves usdot-fhwa-stol/carma-platform#1559 by updating the NDT optimization parameters to reflect the new PCL 1.9+ API changes. ROS Noetic targets PCL v 1.10 whereas ROS Kinetic target PCL v 1.7.2 It seems that in v1.9 the API for the error epsilons was changed (see PointCloudLibrary/pcl#1724) Now the translation epsilon is actually a squared error and there is an additional epsilon which is the rotation angle error in an axis-angle rotation representation. This PR updates the translation error setting in NDT to therefore be 0.001 which makes the optimization selectivity closer to the old value (TODO I'm still evaluating if 0.001 or 0.0001 (which is 0.01*0.01) is better) (No longer investigating, see comment below). The rotation error is computed as 1.0-translation error which is the default used technique in the ICP matching algorithm (seems they didn't add a default for NDT matching). This may seem strange but actually makes sense as the input is cos(angle error). And arccos(0.999) =0.044 rad or 2.5 deg error. This PR also removes the --sequential build argument from the docker image to hopefully speed up build times. I performed some comparison runs using translation epsilons of 0.001 and 0.0001 respectively. Both gave similar over all NDT score values. The first value required fewer iterations overall and hit the max less often while the second value more often hit the max. Both ran within reasonable time frames though the second value did go above 100ms more frequently. Since I did not see a noticeable improvement in NDT score, I plan to continue with 0.001 for now as it stayed under the 100ms execution time target more reliably. However, I have left a comment describing that 0.0001 can work as documentation of its feasibility.
| if (transformation_rotation_epsilon_ > 0) | ||
| convergence_criteria_->setRotationThreshold (transformation_rotation_epsilon_); | ||
| else | ||
| convergence_criteria_->setRotationThreshold (1.0 - transformation_epsilon_); |
There was a problem hiding this comment.
Is there a particular reason why this line uses transformation_epsilon_ which has a totally different unit than transformation_rotation_epsilon_? is this a bug or intended?
There was a problem hiding this comment.
I guess it was there before, but still wondering.
Although ICP allowed to set the rotation epsilon by overriding the ConvergenceCriteria, setting the convergence metric in the Registration class allows a more uniform approach to provide a rotation and translation convergence metrics to all variants of ICP and NDT.
I also made the usage of transformation_epsilon_ and transformation_rotation_epsilon_ uniform for ICP and NDT.
In NDT i made the convergence criteria selective (if the rotation [and / or] translation parameters are not > 0, they are not used to stop the registration).