[vtk-developers] BestFitPlane function for vtkPlane

[Karthik Krishnan]

On Wed, Nov 4, 2009 at 2:21 PM, David Doria
<daviddoria+vtk at gmail.com<daviddoria%2Bvtk at gmail.com>
> wrote:

> On Wed, Nov 4, 2009 at 12:20 PM, Karthik Krishnan
> <karthik.krishnan at kitware.com> wrote:
> > Thanks
>
>
> These tests failed before my changes and after:
> The following tests FAILED:
>         93 - SurfacePlusEdges (Failed)
>        105 - TestLabelPlacementMapper (SEGFAULT)
>
> but everything else passes with the changes. Here are the files:
> http://www.rpi.edu/~doriad/VTK_List/vtkPlane/<http://www.rpi.edu/%7Edoriad/VTK_List/vtkPlane/>
>
> I wasn't sure how to get a vtkPoints from a vtkDataSet, so I did this:
>
> void vtkTextureMapToPlane::ComputeNormal(vtkDataSet *input)
> {
>  vtkSmartPointer<vtkPoints> Points = vtkSmartPointer<vtkPoints>::New();
>  for(unsigned int i = 0; i < input->GetNumberOfPoints(); i++)
>    {
>    double p[3];
>    input->GetPoint(i, p);
>    Points->InsertNextPoint(p);
>    }
>

Not a fan of copying the entire list of points and retrieving the same back
from vtkPlane. The copy seems quite unncessary.

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.

What I would instead do is add an overloaded API to vtkPlane that can take
in both a vtkDataSet and a vtkPoints.

something like :

  vtkPlane::ComputeBestFittingPlane( vtkPoints *points )
     {
     ...
     }

  vtkPlane::ComputeBestFittingPlane( vtkDataSet *dataset )
     {
     ...
     }


Apart from the unnecessary copy, things look good.

-----

A point regarding coding style : In VTK, (or ITK), we don't tend to use
capitalization for local variables. Specifically, the line
   vtkSmartPointer<vtkPoints> Points = vtkSmartPointer<vtkPoints>::New();
would conform with the coding guidelines if it were :
   vtkSmartPointer<vtkPoints> points = vtkSmartPointer<vtkPoints>::New();


Thanks for taking the time to contribute the patch.
--
karthik
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20091110/98a587cd/attachment.html>