<div class="gmail_quote">On Tue, Feb 23, 2010 at 3:43 AM, Luca Antiga <span dir="ltr"><<a href="mailto:luca.antiga@gmail.com">luca.antiga@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">Hi David,<div> thanks a lot for this contribution, it will probably solve a few problems I had with ICP in the past.</div><div>A few suggestions:</div><div><br></div><div>- 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;</div>
<div><br></div><div>- there is an indentation glitch in the for loop;</div><div><br></div><div>- you don't really need to Delete closestp, just use closestp->Initialize();</div><div><br></div><div>- 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).</div>
<div><br></div><div>- TooFarThreshold is compared to dist2, which is the squared distance; my vote is for specifying it as a distance and square it internally;</div><div><br></div><div>- 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.</div>
<div><br></div><div>Here's my try:</div><div><br></div><div>  double tooFarThreshold2 = this->TooFarThreshold * this->TooFarThreshold;</div><div><div>  do </div><div>    {</div><div>    // Fill points with the closest points to each vertex in input</div>
<div>    if (!this->UseTooFarThreshold)</div><div>      {</div><div>      closestp->SetNumberOfPoints(nb_points); // It was set above, but at this point it should be removed from there</div><div>      for(i = 0; i < nb_points; i++)</div>
<div>        {</div><div>        this->Locator->FindClosestPoint(a->GetPoint(i),</div><div>                                        outPoint,</div><div>                                        cell_id,</div><div>                                        sub_id,</div>
<div>                                        dist2);</div><div>        closestp->SetPoint(i, outPoint);</div><div>        }</div><div>      this->LandmarkTransform->SetSourceLandmarks(a);</div><div>      }</div><div>
    else //use a TooFarThreshold</div><div>      {</div><div>      vtkPoints* sourceLandmarks = vtkPoints::New();</div><div>      closestp->Initialize();</div><div>      for(i = 0; i < nb_points; i++)</div><div>        {</div>
<div>        this->Locator->FindClosestPoint(a->GetPoint(i),</div><div>                                        outPoint,</div><div>                                        cell_id,</div><div>                                        sub_id,</div>
<div>                                        dist2);</div><div>        if(dist2 < tooFarThreshold2)</div><div>          {</div><div>          closestp->InsertNextPoint(outPoint);</div><div>          sourceLandmarks->InsertNextPoint(a->GetPoint(i));</div>
<div>          }</div><div>        }</div><div>      this->LandmarkTransform->SetSourceLandmarks(sourceLandmarks);</div><div><div>      sourceLandmarks->Delete();</div></div><div>      }</div><div>    // Build the landmark transform</div>
<div><br></div><div>    this->LandmarkTransform->SetTargetLandmarks(closestp);</div><div>    this->LandmarkTransform->Update();</div><div><br></div><div><br></div><div>Cheers</div><div><br></div></div><div><br>
</div><div>Luca</div><div></div></div></blockquote></div><br><div>Luca -</div><div><br></div><div>Overall, sounds/looks good to me.</div><div><br></div><div>1) UseTooFarThreshold - Sure, I like it.</div><div><br></div><div>
2) I had never used Initialize() - so if it clears the data without having to delete it, that was my goal.</div><div><br></div><div>3) Any reason to compare TooFarThreshold2 to dist2 instead of TooFarThreshold to sqrt(dist2)?</div>
<div><br></div><div>Any other thoughts?</div><div><br clear="all">Thanks,<br><br>David<br></div>