[Insight-developers] Rigid3DTransform

[Luis Ibanez]

Hi Bill,


The class itk::Rigid3DTransform  is used in Slicer in the following files:


1) Base/Logic/vtkSlicerTransformLogic.cxx
2) Applications/CLI/BRAINSTools/BRAINSCommonLib/GenericTransformImage.cxx
3) Applications/CLI/ResampleVolume2.cxx
4) Applications/CLI/DiffusionApplications/ResampleDTI/ResampleDTI.cxx
5) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.txx
6) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.h



The uses in (1) and (2) are now obsolete.

They were registering all the Transforms factories in order to
deal with the absence of the ScaleVersor3DTransform in ITK 3.18.
Now that this transform is in ITK 3.20 (and ITKv4) the factory
registration is no longer needed.

So, lines 170-232 of  (1)
Base/Logic/vtkSlicerTransformLogic.cxx
should be removed.

The same goes for lines 410-476 in (2)
Applications/CLI/BRAINSTools/BRAINSCommonLib/GenericTransformImage.cxx


-----

The uses in (3) lines 890-925 is actually an improper use of
polymorphism. The code is using dynamic_casts to uncover
the type of a derived class, just to call on it a function that was
virtual in the first place. This one actually calls for a refactoring
of the GetInverse() method in ITK. We should discuss that one
with Brian Avants as part of the Image Registration refactoring
effort.


The use in (3) lines 228 can be replaced with MatrixOffsetTransformBase,
since, the code is using only the SetMatrix() and SetTranslation() methods
that are actually defined in this base class.


The use in (3) 287 and 318 can also be replaced with
MatrixOffsetTransformBase. It is actually casting the rigid tranform
to that base class anyways in line 323.

Same goes for the uses in (3) in lines 328 and 330.

The declaration in (3) line 501 is not used at all,
so it should just be deleted.

----

The uses in (4)
Applications/CLI/DiffusionApplications/ResampleDTI/ResampleDTI.cxx

The use in ines 471, does not really requires instantiation with New()
The code is using a dynamic_cast in order to discover the type
of an abstract pointer, in order to assign the same pointer to a
MatrixOffsetTransformBase in line 478, and register a code "rt"
in line 476. The check could have been done with a raw pointer.

The same applies in 483-491.

----

The uses in (5,6)

5) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.txx
6) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.h

The file (6) only has declarations,
The file (5) uses the type in:

Lines 47-56, where it uses the transform as a MatrixOffsetTransformBase.
That is, it only uses the SetMatrix(), SetTranslation() API.

In lines 32-41, the transform is also used just as a MatrixOffsetTransformBase.

So, essentially, the uses of Rigid3DTransform in (5,6) can be replaced
with MatrixOffsetTransformBase.


------

I'll be happy to help convert all the code above.

------

However,
the only unsolved case is:

                                      IO

since the transform writers put the class names in the files *AND*
we do not use a file format version (which is the real problem)...

Then,
previously generated files, that contain the code "Rigid3DTransform"
can't be read with ITKv4.

The lesson to learn there is that we should introduce versioning
in all the file formats over which we have some control.

----

Of course,
it is easier to restore the NewMacro in Rigid3DTransform,
but then...

We have to find an alternative way of preventing new users
from instantiating that transform and trying to use it in their
first registration example.


Any suggestions ?

- Throwing exceptions when they call SetParameters ?
- Displaying warnings ?



     Luis


------------------------------------------------------------------
On Sat, Jan 1, 2011 at 12:23 PM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
> Luis,
>
> As part of our Slicer4/ITK4 experiment, there are a number of compile
> errors because Slicer4(3) instantiate Rigid3dTransform.
>
> Recall that we removes the itkNewMacro for the class in response to
> bug 8320: http://public.kitware.com/Bug/view.php?id=8320
>
> The corresponding gerrit topic is here:
> http://review.source.kitware.com/#change,504
>
> I believe that Slicer4(3)'s use of Rigid3DTransform is valid.
> Slicer4(3) have saved transforms that are part of the test suite. Note
> that in your comments on the bug tracker you state:
> "It doesn't hurt to have a valid implementation of Get/SetParamters at
> the level of the Rigid3DTransform. However, this class simply
> shouldn't be used as a full fledged transform. (e.g. for example as
> part of an image registration framework). "
>
> Note the word "shouldn't". Well, Slicer4(3) did.
>
> I suggest that we restore the itkNewMacro and resolve bug 8320 another
> way. I think it is hard to justify such an API change in light of a
> valid use in Slicer4(3).
>
> Bill
>