[Insight-developers] Problem with ITK 3.20 itkQuadEdgeMeshDiscretePrincipalCurvaturesEstimator.h

[Alexandre GOUAILLARD]

hi kent.

thanks for your feedback.

QEMesh is still alive.

The API is supposed to be stable, even though I plan to modify
slightly the internals to improve speed (copy) and to help addressing
non-manifoldness.

I am in the process of moving it out of review, but it impacts a lot
of things. I plan to have a branch ready and approved by a few senior
developpers by november when we have our next meeting.

I will proceed and correct the bug your student found right away in
v4. Luis, can you advice how to fix it (if we should) for 3.20. Do we
plan a maintenance release for 3.20. What is the backporting policy?

thanks in advance.

alex.


On Tue, Sep 14, 2010 at 3:26 PM, kent williams
<norman-k-williams at uiowa.edu> wrote:
> This has implications for ITK4, if the review QuadEdgeMesh stuff is being
> moved out of review. One of our students got the following compiler error:
>
> InsightToolkit/Review/itkQuadEd
> geMeshDiscretePrincipalCurvaturesEstimator.h:141:
> error: ISO C++ says that these are ambiguous, even though the worst
> conversion for the first is better than the worst conversion for the second:
>
> InsightToolkit/Utilities/vxl/core/vnl/vnl_math.h:536: note: candidate 1:
> double vnl_math_max(double, double)
> InsightToolkit/Utilities/vxl/core/vnl/vnl_math.h:535: note: candidate 2:
> float vnl_math_max(float, float)
>
> The problem is this method of
> itk::QuadEdgeMeshDiscretePrincipalCurvatureEstimator:
>
>  virtual OutputCurvatureType ComputeDelta( )
>    {
>    return  vnl_math_max( 0., m_Mean * m_Mean - m_Gaussian );
>    }
>
> The type of m_Mean and m_Gaussian is the pixel type of the output mesh
> PixelType.  I presume that whomever wrote this code only tried it with one
> pixel type (float or double) and if the other type is used, it's too
> ambiguous for the compiler.
>
> 3.20 is closed, and I don't know the status of the review QuadEdgeMesh code.
> But it's pretty clear to me that this function could be re-written to be
> un-ambiguous, eg:
>
> virtual OutputCurvatureType ComputeDelta()
> {
> return vnl_math_max(static_cast<OutputCurvatureType>(0.0),
>                                 m_Mean * m_Mean - m_Gaussian);
> }
>
> And I'd feel happier with
>
> (m_Mean * m_Mean) - m_Gaussian
>
> As well, because while the compiler always remembers operator precedence, I
> have to think 'My Dear Aunt Sally' to understand what that expression means.
>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.html
>
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers
>