[Insight-developers] question about preprocessor condition in itk::QuadEdgeMesh<>::AddFace method

[Alexandre GOUAILLARD]

Dear arnaud,

The QuadEdgeMesh classes are checking for half the world out there
(and their moms) which is great in term of robustness, but bad in term
of speed. Especially, it was great when writing those classes, and
when creating a QEMesh, but it is disastrous in term of performance in
a pipeline, when the input is already a QEMesh.

Using NDEBUG was an attempt during one of the SLC project week at
speed up QE filters by:
1 - checking things only in debug mode.
2 - Know when the mesh is already "secure" and bypass the tests during
copy for example
Note that 1 is a very crude attempt, as NDEBUG is not standard, and I
m open to suggestions on how to make it better / more standard / cross
platform.

For this specific code, the idea would be to go ahead with the test
only in debug mode, i.e. only when NDEBUG is *NOT* defined (MSVC
defines NDEBUG for you in release mode, as well as _DEBUG. NDEBUG is
specifically for asserts, _DEBUG is supposed to be used for CRT.)

HTH.

regards,

alex.
PS: enjoy european food for us ;-)

On Tue, Apr 17, 2012 at 3:59 PM, Arnaud Gelas <arnaudgelas at gmail.com> wrote:
> Hi Alex,
>
> In the AddFace method of itk::QuadEdgeMesh, there is a preprocessor
> condition (#ifndef NDEBUG, see code below).
> Is it correct to make these tests only when NDEBUG is undefined? Is it the
> other way around? or should it be tested all the time?
>
> Thanks,
> Arnaud
>
> ---
>
> template< typename TPixel, unsigned int VDimension, typename TTraits >
> typename QuadEdgeMesh< TPixel, VDimension, TTraits >::QEPrimal *
> QuadEdgeMesh< TPixel, VDimension, TTraits >
> ::AddFace(const PointIdList & points)
> {
> #ifndef NDEBUG
>  // Check that there are no duplicate points
>  for ( size_t i = 0; i < points.size(); i++ )
>    {
>    typename PointIdList::const_iterator itr = points.begin();
>    typename PointIdList::const_iterator end = points.end();
>    PointIdentifier count = NumericTraits< PointIdentifier >::Zero;
>    const PointIdentifier pointId = points[i];
>    while ( itr != end )
>      {
>      if ( *itr == pointId )
>        {
>        ++count;
>        }
>      ++itr;
>      }
>    if ( count != 1 )
>      {
>      itkDebugMacro("Point " << i << " is duplicated");
>      return ( (QEPrimal *)NULL );
>      }
>    }
>
>  // Check that all points exist
>  for ( size_t i = 0; i < points.size(); i++ )
>    {
>    if ( !this->GetPoints()->IndexExists(points[i]) )
>      {
>      itkDebugMacro("Point " << i << " is missing in the mesh");
>      return (QEPrimal *)NULL;
>      }
>    }
> #endif
>
>  // Check if existing edges have no face on the left.
>  for ( size_t i = 0; i < points.size(); i++ )
>    {
>    PointIdentifier pid0 = points[i];
>    PointIdentifier pid1 = points[( i + 1 ) % points.size()];
>
>    QEPrimal *edge = this->FindEdge(pid0, pid1);
>
>    if ( edge )
>      {
>      if ( edge->IsLeftSet() )
>        {
>        itkDebugMacro("Edge [" << i << " " << ( ( i + 1 ) % points.size() )
> << " has a left face.");
>        return (QEPrimal *)NULL;
>        }
>      }
>    }
>
>  return AddFaceWithSecurePointList(points);
> }