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.

Sign up for GitHub

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

Jump to bottom

GICP does not use Default Convergence Criteria #3329

Open
drewtu2 opened this issue Sep 4, 2019 · 2 comments · May be fixed by #5287
Open

GICP does not use Default Convergence Criteria #3329

drewtu2 opened this issue Sep 4, 2019 · 2 comments · May be fixed by #5287
Labels
help wanted kind: bug Type of issue module: registration needs: feedback Specify why not closed/merged yet

Comments

@drewtu2
Copy link
Contributor

drewtu2 commented Sep 4, 2019 •
edited
Loading

The implementation of Generalized ICP does not use the DefaultConvergenceCriteria::Ptr convergence_criteria_ This causes confusion when trying to check the state of convergence after running the alignment. Checking the hasConverged() method will return true but checking the state through getConvergenceState() will return ConvergenceState::CONVERGENCE_CRITERIA_NOT_CONVERGED. Because of the conflicting states, it is difficult understand what the state of the convergence is.

A similar issue was brought up for ICP in issue #1598 and addressed by @WraithKim in PR #2892.

Possible Solution

If the using the DefaultConvergenceCriteria does not apply to GICP, the implementation should override getConvergenceCriteria() to throw an exception in order to avoid confusion. Otherwise, the convergence_criteria_ should be updated with the appropriate convergence conditions.
@stale
Copy link

stale bot commented May 19, 2020

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

@stale stale bot added the status: stale label May 19, 2020
@stale stale bot removed the status: stale label Jun 5, 2020
@kunaltyagi kunaltyagi added needs: feedback Specify why not closed/merged yet and removed kind: todo Type of issue labels Jun 13, 2020
@guilhermelawless
Copy link

Even worse than checking the convergence state is the fact that you could be misled to use setFailureAfterMaximumIterations(true) for GICP. In fact there is no way to make GICP set "not converged" after it fails to converge for max_iterations, which is very counter-intuitive.

@keineahnung2345 keineahnung2345 linked a pull request Jun 7, 2022 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted kind: bug Type of issue module: registration needs: feedback Specify why not closed/merged yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[feat] gicp: add convergence criteria and correspondence estimation keineahnung2345/pcl
3 participants
@kunaltyagi @drewtu2 @guilhermelawless