[Insight-developers] Re: PeriodicBoundaryCondition

[Luis Ibanez]

Hi Torsten,

Yes, it seems that there is a circularity problem in this design.

In general, dynamic_casting should be avoided in generic programing,
since, it introduces assumptions about the actual type of an object.

Reviewing the code, it seems that the boundary condition uses the
down casted iterator in order to gain access to the image pointer
via the iterator->GetImagePointer() method.

Instead of having the boundary condition request the image pointer
this way, we could have a SetImage() method in the ImageBoundary
Condition, and have the NeighborhoodIterator pass the image pointer
to the boundary condition.

In other words, the Neighborhood iterator could feed data to the
boundary condition instead of the boundary condition having to query
data from the iterator.

The other methods that the PeriodicBoundary condition is invoking
are already available in the Neighborhood and therefore no dynamic
casting should be necessary.

We are giving it a try to this modification. If it seems to work
then we will send you a patch so you can give it a try.


   Thanks


      Luis



=========================
Torsten Rohlfing wrote:
> 
> 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
>>
>>
> 
>