[Insight-developers] Re: PeriodicBoundaryCondition

[Bill Lorensen]

Luis,

Another idea. Perhaps the Override method should enforce the type of the
BoundaryCondition that was used as the template parameter. I think the
intent of the Override was not to provide a different BC, but rather to
provide a different instance so that parameters could be set for the BC. If
that is the case, the Override method could do the dynamic cast and throw an
exception if the types do not match. In this case, Torsten's patch would be
required.

Bill



On 2/2/07, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> 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
> >>
> >>
> >
> >
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.itk.org/mailman/private/insight-developers/attachments/20070203/c7d46ea1/attachment.html