<div class="gmail_quote">On Wed, Nov 4, 2009 at 2:21 PM, David Doria <span dir="ltr"><<a href="mailto:daviddoria%2Bvtk@gmail.com">daviddoria+vtk@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Wed, Nov 4, 2009 at 12:20 PM, Karthik Krishnan<br>
<div class="im"><<a href="mailto:karthik.krishnan@kitware.com">karthik.krishnan@kitware.com</a>> wrote:<br>
</div>> Thanks<br>
<br>
<br>
These tests failed before my changes and after:<br>
The following tests FAILED:<br>
         93 - SurfacePlusEdges (Failed)<br>
        105 - TestLabelPlacementMapper (SEGFAULT)<br>
<br>
but everything else passes with the changes. Here are the files:<br>
<a href="http://www.rpi.edu/%7Edoriad/VTK_List/vtkPlane/" target="_blank">http://www.rpi.edu/~doriad/VTK_List/vtkPlane/</a><br>
<br>
I wasn't sure how to get a vtkPoints from a vtkDataSet, so I did this:<br>
<br>
void vtkTextureMapToPlane::ComputeNormal(vtkDataSet *input)<br>
{<br>
  vtkSmartPointer<vtkPoints> Points = vtkSmartPointer<vtkPoints>::New();<br>
  for(unsigned int i = 0; i < input->GetNumberOfPoints(); i++)<br>
    {<br>
    double p[3];<br>
    input->GetPoint(i, p);<br>
    Points->InsertNextPoint(p);<br>
    }<br></blockquote><div><br>Not a fan of copying the entire list of points and retrieving the same back from vtkPlane. The copy seems quite unncessary. <br><br>It merely seems to satisfy your API in vtkPlane, which takes in a vtkPoints as input to compute the best fit plane, while vtkTextureMapToPlane takes in any vtkDataSet as input.<br>
<br>What I would instead do is add an overloaded API to vtkPlane that can take in both a vtkDataSet and a vtkPoints. <br><br>something like :<br><br>  vtkPlane::ComputeBestFittingPlane( vtkPoints *points )<br>     {<br>     ...<br>
     }<br><br>  vtkPlane::ComputeBestFittingPlane( vtkDataSet *dataset )<br>
     {<br>
     ...<br>
     }<br>
<br>
<br>Apart from the unnecessary copy, things look good.<br><br>-----<br><br>A point regarding coding style : In VTK, (or ITK), we don't tend to use capitalization for local variables. Specifically, the line<br>   vtkSmartPointer<vtkPoints> Points = vtkSmartPointer<vtkPoints>::New();<br>
would conform with the coding guidelines if it were :<br>   vtkSmartPointer<vtkPoints> points = vtkSmartPointer<vtkPoints>::New();<br>
<br><br>Thanks for taking the time to contribute the patch.<br>--<br>karthik</div></div><br>