Fix ICP misbehavior in the "failure after maximum iterations" mode #2892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Exit loop with convergence state instead of `converged_` in icp - Add new convergence state `CONVERGENCE_CRITERIA_FAILURE_AFTER_MAX_ ITERATIONS` - When `failure_after_max_iter_` is set true, `hasConverged()` checks all similarity before returning a failure. - Even though one iteration has several similar conditions, counts `iterations_similar_transforms_` up only 1. - If transform becomes large, `iterations_similar_transforms_` reset to 0. Only consecutive similar iterations are allowed to converge. - Edit a description about the change above in API docs. - When new alignment restarts after a convergence or a failure, `iterations_similar_transforms_` also reset to 0.
taketwo
added
needs: code review
SunBlack
reviewed
Mar 7, 2019
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Outdated
Show resolved
Hide resolved
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Outdated
Show resolved
Hide resolved
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Outdated
Show resolved
Hide resolved
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Show resolved
Hide resolved
WraithKim
commented
Mar 9, 2019
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Show resolved
Hide resolved
|
No. Just remove else case in case there is a return in the other case. This is not the case here |
taketwo
reviewed
Apr 3, 2019
registration/include/pcl/registration/impl/default_convergence_criteria.hpp
Outdated
Show resolved
Hide resolved
Because there is a reset right after hasConverged() is called.
SergioRAgostinho
approved these changes
Apr 9, 2019
There was a problem hiding this comment.
LGTM. @taketwo I would propose a squash with a more meaningful commit message and it's probably better for you to actually write it since I didn't follow this issue properly. 😅
Edit: Forgot to add the question. Should we tag this as a behavioural change? We are adding more statuses...
taketwo
added
the
changelog: behavior change
|
Yes, this certainly qualifies for behavior change. |
taketwo
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Executive summary:
This updates
ICPandDefaultConvergenceCriteriaclasses to fix misbehavior in the "failure after maximum iterations" mode. (Prior to this change the registration loop would hang in an infinite loop #1598). It also refines "maximum iterations similar transforms" option to mean consequtive transforms.If you use
DefaultConvergenceCriteriain your registration code, you need to make an adjustment similar to what this PR does in "icp.hpp".It follows #1598. I try to implement as possible as i can. But I didn't understand what taketwo said about reset similar transforms, so I implement my solution.
converged_in icp.CONVERGENCE_CRITERIA_FAILURE_AFTER_MAX_ ITERATIONS.failure_after_max_iter_is set true,hasConverged()checks all similarity before returning a failure.iterations_similar_transforms_up only 1.iterations_similar_transforms_reset to 0. Only consecutive similar iterations are allowed to converge.iterations_similar_transforms_also reset to 0.