Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix pcl version 1.10 compatibility with ndt #190

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

msmcconnell
Copy link

@msmcconnell msmcconnell commented Dec 13, 2021

PR Details

Description

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.

Related Issue

usdot-fhwa-stol/carma-platform#1559

Motivation and Context

Stable localization in Elise

How Has This Been Tested?

Initial testing conducted on Blue lexus shows good results.

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    CARMA Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@msmcconnell
Copy link
Author

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.

@msmcconnell msmcconnell marked this pull request as ready for review December 14, 2021 15:21
@msmcconnell msmcconnell assigned msmcconnell and unassigned kjrush Dec 14, 2021

static double trans_eps = 0.001; // Transformation epsilon. In PCLv1.10 (ros noetic) this value is squared error not base epsilon
// NOTE: A value of 0.0001 can work as well.
// This will increase the required iteration count (and therefore execution time) but might increase performance
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, the mentioned performance here is in terms of accuracy? So the tradeoff is speed vs accuracy?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll clarify


if (_method_type == MethodType::PCL_GENERIC)
if (_method_type == MethodType::PCL_GENERIC) {
ROS_INFO_STREAM("Using tramslation threshold of " << trans_eps);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? tramslation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@msmcconnell msmcconnell merged commit 42bc959 into release/elise Dec 14, 2021
@msmcconnell msmcconnell deleted the fix/ndt_pclv1.10_compatibility branch December 14, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants