[Insight-developers] Re: PeriodicBoundaryCondition

[Torsten Rohlfing]

Hi Luis --

I think I am starting to understand the issue, and it may well be a more 
fundamental design problem. Here's what happens:

In the test code, an iterator is declared as

  typedef itk::ConstNeighborhoodIterator<ImageType> IteratorType;

with the default boundary condition template argument

  itk::ZeroFluxNeumannBoundaryCondition<ImageType>

Then further down, the boundary condition is overridden with

    itk::PeriodicBoundaryCondition<ImageType> bc;
    it.OverrideBoundaryCondition(&bc);

Now, inside itk::PeriodicBoundaryCondition::operator(), the dynamic cast

    const ConstNeighborhoodIterator<TImage,Self> * iterator
      = dynamic_cast<const ConstNeighborhoodIterator<TImage,Self> *>(data);

of the iterator (pointed to by "data) returns NULL, because "Self" is 
the periodic BC and not the default BC. This causes the segfault.

The problem is that without the "Self" template parameter in the cast, 
ie., without my patch, the same segfault happens when you originally 
declare the iterator using the "correct" boundary condition. This is 
what happened to me originally.

So it seems that the problem is really with the fundamental 
incompatibility of the iterator's "OverrideBoundaryCondition" method and 
the dynamic cast inside the BC.

Casting to the parent class if the iterator, ie., "itk::Neighborhood" 
doesn't solve the problem, because it doesn't provide the GetImagePtr() 
member that is needed in itk::PeriodicBoundaryCondition::operator().

While I won't claim to grasp the entire class design here, this seems to 
suggest two possible fixes:

a) Try casts to different template instantiations in 
itk::PeriodicBoundaryCondition::operator() and use the one that gives a 
non-NULL result. This isn't really practical, because we shouldn't hard 
code all possible boundary conditions here.

b) Find a different way to provide the iterator data to the boundary 
condition, which doesn't involve RTTI casts.

And then there's the workaround: always declare iterators with the 
default BC and override it with the one we really want. This latter 
options seems to require the least effort right now, but it should 
probably be (better) documented if this is in fact required.

I'll be very interested in hearing your thoughts!
  Torsten

Luis Ibanez wrote:
> Hi Torsten,
>
>
> Thanks for reporting the problem with the
> PeriodicBoundaryCondition class:
>
> http://public.kitware.com/Bug/bug.php?op=show&bugid=4336&pos=3
>
> We just applied the patch that you kindly provided,
> but then the test for this class segfaults.
>
> Could you please verify if the Test is to be modified too ?
>
> Or maybe we should roll back this fix until there is more
> clarity on what the dynamic casting should use as type argument.
>
>
>   Please let us know,
>
>
>     Thanks
>
>
>       Luis
>
>


-- 
Torsten Rohlfing, PhD          SRI International, Neuroscience Program
 Research Scientist             333 Ravenswood Ave, Menlo Park, CA 94025
  Phone: ++1 (650) 859-3379      Fax: ++1 (650) 859-2743
   torsten at synapse.sri.com        http://www.stanford.edu/~rohlfing/

     "Though this be madness, yet there is a method in't"