Repository navigation
Conversation
ICP, ICP-NL and JointICP pass euclidean_fitness_epsilon_ to DefaultConvergenceCriteria::setRelativeMSE, so it is compared against the relative change of the correspondence MSE between two iterations, not an absolute Euclidean error. Update the setter/getter docs and the termination criteria lists in icp.h and icp_nl.h accordingly. Fixes PointCloudLibrary#5868.
This branch has not been deployed
This file contains hidden or 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
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.
Fixes #5868.
As discussed in the issue, the doc of
Registration::setEuclideanFitnessEpsilonsays it is the "maximum allowed Euclidean error between two consecutive steps", but that is not how it is used.IterativeClosestPoint::computeTransformation(also used byIterativeClosestPointNonLinear) andJointIterativeClosestPointpass it toDefaultConvergenceCriteria::setRelativeMSE, so convergence is detected whenwhere the MSE is the mean of the correspondence distances (squared Euclidean distances with the default correspondence estimation). So e.g.
0.01means "the MSE changed by less than 1%", not "the error is below 0.01".Changes (documentation only):
registration.h: reword thesetEuclideanFitnessEpsilon/getEuclideanFitnessEpsilondocs to describe the relative MSE change, which classes use it this way, and that other registration methods may use it differently or ignore it (e.g. GICP has its own loop, FRICP documents that it is not evaluated).icp.h,icp_nl.h: the third termination criterion in the class docs said "the sum of Euclidean squared errors is smaller than a user defined threshold"; changed it to the relative change of the MSE, and updated the matching comment in the ICP usage example.Not changed: the usage example in
icp.hstill callssetEuclideanFitnessEpsilon (1). With a relative threshold, 1 is very loose (almost any iteration counts as converged on the MSE criterion), so maybe a smaller value would be a better example, but I left the code as is. The modeler app's parameter tooltip (apps/modeler/src/icp_registration_worker.cpp) uses the old wording too; I can update it here if you'd like.Checked: the three headers are unchanged by clang-format 14 (registration is in the format whitelist), and a small program including
icp.h,icp_nl.handjoint_icp.hcompiles with GCC.I used an AI assistant (Grok Bot) to help trace how the value is used and to draft this change; I checked the code paths and the wording myself.