[Insight-developers] ITKImageFunction dependecy in Transform

[Alexandre GOUAILLARD]

thanks, testing it right now, and trying to rebase gaethan's work on
it if necessary:
http://review.source.kitware.com/#change,2531

alex.

On Thu, Aug 25, 2011 at 7:51 PM, Matt McCormick
<matt.mccormick at kitware.com> wrote:
> Hi Alex,
>
> Please review the following patch:
>
> http://review.source.kitware.com/#change,2563
>
> A patch that fixes the issue will follow sometime today on that branch.
>
> Thanks,
> Matt
>
> On Wed, Aug 24, 2011 at 11:44 PM, Alexandre GOUAILLARD
> <agouaillard at gmail.com> wrote:
>> Dear all,
>>
>> The wrapping builds have been broken because of this issue for more
>> than two weeks now.
>>
>> Could we move forward to a solution *soon* so that we can make
>> progress on Wrapping? Almost no work could be done for the current
>> Sprint, and there is only one sprint left before the beta.
>>
>> regards,
>>
>> alex.
>>
>>
>> 2011/8/21 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>>>
>>> Le 19 août 11 à 19:08, M Stauffer (V) a écrit :
>>>
>>>>>> This issue has popped up again, see below...
>>>>>>
>>>>>>>>>>> In rebasing the Transform changes patch set onto the latest
>>>>>>>>>>
>>>>>>>>>> itk master, I've
>>>>>>>>>>>
>>>>>>>>>>> got a dependency problem. DeformationFieldTransform
>>>>>>>>>>> includes itkVectorLinearInterpolateImageFunction.h, which is
>>>>>>>>>>> in ITKImageFunction. But adding this to
>>>>>>>>>>
>>>>>>>>>> Transform/itk-module.cmake causes a
>>>>>>>>>>>
>>>>>>>>>>> circular dependency. Any suggestions? I don't know how to
>>>>>>>>>>
>>>>>>>>>> handle this.
>>>>>>>>>>
>>>>>>>>>> That is a difficult problem.  Moving DeformationFieldTransform to
>>>>>>>>>> ITKImageFunction would probably not be appropriate.  Is it
>>>>>>>>
>>>>>>>> possible to
>>>>>>>>>>
>>>>>>>>>> use a C++ forward declaration for
>>>>>>>>>> VectorLinearInterpolateImageFunction?  I assume that any module
>>>>>>>>>> depending on ITKTransform is also likely to depend on
>>>>>>>>
>>>>>>>> ITKImageFunction
>>>>>>>>>>
>>>>>>>>>> anyway.
>>>>>>>>>
>>>>>>>>> OK, a forward declare works. Of course I have to declare
>>>>>>>
>>>>>>> the template
>>>>>>>>>
>>>>>>>>> parrameters along with the forward declaration so it's a
>>>>>>>>
>>>>>>>> little messy in
>>>>>>>>>
>>>>>>>>> that it will need changing if the
>>>>>>>>
>>>>>>>> VectorLinearInterpolateImageFunction
>>>>>>>>>
>>>>>>>>> template parameters change. Although if the class template
>>>>>>>>
>>>>>>>> pararmeters
>>>>>>>>>
>>>>>>>>> change, lots of classes will change so I figure that's not a real
>>>>>>>>> problem. So is forward-declaration like this ok in the ITK style?
>>>>>>>>
>>>>>>>> Great.
>>>>>>>>
>>>>>>>> ITK has very strong backwards compatibility, changes to the API like
>>>>>>>> that will not take place after the ITKv4 release, and changes that
>>>>>>>> break compatibility during ITKv4 need to have migration documents.
>>>>>>>> http://www.vtk.org/Wiki/ITK_Release_4/Users_Migration_Guide
>>>>>>>>
>>>>>>>> Forward declaration is not typical ITK style.  However, if it is
>>>>>>>> documented with a comment in the code explaining the need to break
>>>>>>>> circular dependency, it could be OK.
>>>>>>>>
>>>>>>
>>>>>> It turns out this has worked only because the cxx files where
>>>>>> itkDisplacementTransform.h is included also include
>>>>>> itkVectorInterpolateImageFunction.h and
>>>>>> itkVectorLinearIntepolateImageFunction.h. Of course these have to be
>>>>>> included somewhere for the forward-declare to work, but this
>>>>>
>>>>> seems bad
>>>>>>
>>>>>> for the use to have to include these other two headers separately.
>>>>>>
>>>>>> Looking at Core/ImageFunction, the only class that includes
>>>>>
>>>>> Transform is
>>>>>>
>>>>>> RayCastInterpolateImageFunction. Would it make sense to move this to
>>>>>> another module so that ImageFunction needn't depend on Transform?
>>>>>>
>>>>>
>>>>> Good detective work.  Maybe move RayCastInterpolateImageFunction to
>>>>> Core/Common instead of the forward declare?
>>>>>
>>>>> Thanks,
>>>>> Matt
>>>>
>>>> OK I've tried that and it works. I removed ITKTransform as a dependency
>>>> of ITKImageFunction, and added ITKImageFunction as a dependency of
>>>> ITKTransform. I moved RayCastInterpolateImageFunction to Core/Common.
>>>>
>>>
>>> itkRayCastInterpolateImageFunction.h still includes itkTransform.h, right?
>>>
>>> If that's the case, it can't be moved to Core/Common, because itkTransform.h
>>> is in ITKTransforms which is not a dependency of ITKCommon.
>>>
>>>> What's the protocol for going ahead with a move like this? Doesn't seem
>>>> like much of a change since RayCastInterpolateImageFunction is still in
>>>> Core. Matt, do you feel ok giving the final go-ahead for this, or do we
>>>> get other approval too?
>>>>
>>>
>>> Module groups don't have much meaning when dealing dependencies.
>>>
>>> This problem is preventing wrapping to build - the forward declaration is
>>> not enough, the headers must be included - it would be fine to find a
>>> solution.
>>>
>>> In that case I guess the only options are:
>>>
>>>  a. move RayCastInterpolateImageFunction to ITKTransforms
>>>
>>>  b. move DeformationFieldTransform to ITKImageFunction
>>>
>>>  c. move RayCastInterpolateImageFunction to another module which depends on
>>> both ITKTransforms and ITKImageFunction. If no current module fit for that
>>> task, then a new one should be created.
>>>
>>>  d. move DeformationFieldTransform to another module which depends on both
>>> ITKTransforms and ITKImageFunction. If no current module fit for that task,
>>> then a new one should be created.
>>>
>>> And of course adjust the module dependencies.
>>>
>>> I think my preference would go for c.
>>>
>>> Regards,
>>>
>>> Gaëtan
>>>
>>> --
>>> Gaëtan Lehmann
>>> Biologie du Développement et de la Reproduction
>>> INRA de Jouy-en-Josas (France)
>>> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
>>> http://mima2.jouy.inra.fr  http://www.itk.org
>>> http://www.bepo.fr
>>>
>>>
>>
>