[vtk-developers] ICP Enhancement - TooFarThreshold

[David Doria]

On Tue, Feb 23, 2010 at 3:43 AM, Luca Antiga <luca.antiga at gmail.com> wrote:

> Hi David,
>  thanks a lot for this contribution, it will probably solve a few problems
> I had with ICP in the past.
> A few suggestions:
>
> - I'd suggest is to add a UseTooFarThreshold flag with the
> relative vtkSetMacro, vtkGetMacro and vtkBooleanMacro to activate/deactivate
> TooFar (as in UseTooFarThresholdOn, UseTooFarThresholdOff, etc) instead
> of resorting to the -1.0 trick;
>
> - there is an indentation glitch in the for loop;
>
> - you don't really need to Delete closestp, just use
> closestp->Initialize();
>
> - actually, the code will crash if rerun with TooFar on first and TooFar
> off afterwards, because in the latter you call closestp->SetPoint without
> first setting the NumberOfPoints after you Delete and instantiate a new one
> (or call Initialize).
>
> - TooFarThreshold is compared to dist2, which is the squared distance; my
> vote is for specifying it as a distance and square it internally;
>
> - I personally would handle sourceLandmarks in a bit more readable way: the
> smart pointer initialized in the else scope and used outside will behave
> correctly feels weird, but it's just that I'm old fashioned. Read below for
> a proposal.
>
> Here's my try:
>
>   double tooFarThreshold2 = this->TooFarThreshold * this->TooFarThreshold;
>   do
>     {
>     // Fill points with the closest points to each vertex in input
>     if (!this->UseTooFarThreshold)
>       {
>       closestp->SetNumberOfPoints(nb_points); // It was set above, but at
> this point it should be removed from there
>       for(i = 0; i < nb_points; i++)
>         {
>         this->Locator->FindClosestPoint(a->GetPoint(i),
>                                         outPoint,
>                                         cell_id,
>                                         sub_id,
>                                         dist2);
>         closestp->SetPoint(i, outPoint);
>         }
>       this->LandmarkTransform->SetSourceLandmarks(a);
>       }
>     else //use a TooFarThreshold
>       {
>       vtkPoints* sourceLandmarks = vtkPoints::New();
>       closestp->Initialize();
>       for(i = 0; i < nb_points; i++)
>         {
>         this->Locator->FindClosestPoint(a->GetPoint(i),
>                                         outPoint,
>                                         cell_id,
>                                         sub_id,
>                                         dist2);
>         if(dist2 < tooFarThreshold2)
>           {
>           closestp->InsertNextPoint(outPoint);
>           sourceLandmarks->InsertNextPoint(a->GetPoint(i));
>           }
>         }
>       this->LandmarkTransform->SetSourceLandmarks(sourceLandmarks);
>       sourceLandmarks->Delete();
>       }
>     // Build the landmark transform
>
>     this->LandmarkTransform->SetTargetLandmarks(closestp);
>     this->LandmarkTransform->Update();
>
>
> Cheers
>
>
> Luca
>

Luca -

Overall, sounds/looks good to me.

1) UseTooFarThreshold - Sure, I like it.

2) I had never used Initialize() - so if it clears the data without having
to delete it, that was my goal.

3) Any reason to compare TooFarThreshold2 to dist2 instead of
TooFarThreshold to sqrt(dist2)?

Any other thoughts?

Thanks,

David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20100223/a99bbdfb/attachment.html>