[Insight-developers] RE: Changes to ImageSource::GraftNthOutput

[Miller, James V (Research)]

Luis, 

I still don't think the dynamic_cast works since the image pointer was already
static_cast'ed to TOutputImage*.  Once cast to that, I don't think the dynamic_cast
will be able to determine whether the image was really of a different type.

The implementation of Graft() in ImageBase and Image do take some these things
into consideration.  The argument to Graft() in ImageBase and Image is a
ImageBase of the same dimension as the image you are grafting onto. 

So the output and graft image must be instances of an ImageBase with the 
same dimension.  Unfortunately, due to the static_cast to TOutputImage*, this
is probably always true.

In Image::Graft(), the grafting of the pixel data will only occur if the 
graft image has the same type of pixel as the image being grafted onto.  This is
what we want.

What would be ideal, is if ImageSource::GraftNthOutput() could take an ImageBase
that is correct dimension for the output being grafted onto.  I can't think 
of a way to do this.

Thus, I think the GraftNthOutput() method and the GraftOutput() method of ImageSource
should probably take a DataObject*.  

Ideally, we would then want to cast this DataObject to an ImageBase of the appropriate dimension
for the output being grafted.  If that case was successful, then we could call the 
the Graft method in Image. Unfortunately, I don't think there is a way to do this.

This leaves us with also having to change the arguments to ImageBase::Graft() and Image::Graft() to take
a DataObject*.  Each of these implementations would do a dynamic cast to what they are expecting
(ImageBase::Graft would dynamic_cast to an ImageBase of the appropriate dimension, Image::Graft
would dynamic_cast to an Image of the appropriate dimension AND pixel type).

We should probably call ProcessObject::GetOutput() to get the data as an DataObject* (avoids
a static_cast to a TOutputImage*).

ImageSource<TOutputImage>
::GraftNthOutput(unsigned int idx, DataObject *graft)
{
  DataObject * output = this->ProcessObject::GetOutput(idx);
  output->Graft( graft );
}


-----Original Message-----
From: Luis Ibanez [mailto:luis.ibanez at kitware.com]
Sent: Wednesday, November 30, 2005 11:24 AM
To: Miller, James V (Research)
Cc: Lorensen, William E (Research); Insight Developers List
Subject: Re: Changes to ImageSource::GraftNthOutput




Hi Jim,


I'm looking at the changes on the ImageSource::GraftNthOutput() method
and found that the code was already assuming that all the outputs are
of the same type.



Please take a look at the differences between the current version and
the previous one:

http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkImageSource.txx?root=Insight&r1=1.59&r2=1.60



The previous version (1.59) was doing roughtly the following:


ImageSource<TOutputImage>
::GraftNthOutput(unsigned int idx, TOutputImage *graft)
{
  typename ImageSource<TOutputImage>::OutputImageType * output
                                           = this->GetOutput(idx);

    output->Graft( graft );
}



So:



1) The signature of GraftNthOutput() itself is already assuming
    that the argument to be grafted is of type "TOutputImage".
    Which is incorrect for filters having multiple output images
    of different types.


2) It takes the DataObject returned by GetOutput( idx ) and
    statically casted (without using static_cast<>) into a raw
    pointer to TOutputImageType. Again, assuming that the outpu
    idx, is actually of type TOutputImageType *.  This is also
    incorrect for filters with multiple output images of different
    types.


3) It passes the second argument (of type TImageOutput *)
    to the Graft() method the "output" variable.


In this context, the addition of the dynamic_cast was appropriate,
because it is actually verifying that the output is of the
correct type, instead of forcefully casting it without verification.



It seems that what should be fixed is the type itself of the
second argument of GraftNthOutput(), which should probably be
just a (DataObject *) in order to be as general as the
GetOutput() method.


The code should look like:


ImageSource<TOutputImage>
::GraftNthOutput(unsigned int idx, DataObject *graft)
{
  DataObject * output = this->GetOutput(idx);
  output->Graft( graft );
}


Which also means that the Graft() method should be declared
virtual at the level of the DataObject, and all subsequent
derived classes should reimplement it with an internal
dynamic_cast that will verify if the argument DataObject*
is actually pointing to the expected data type.


Please let me know if you find these changes to
be appropriate, and if so, I will commit then to
the trunk and to the release branch.



   Thanks


      Luis




---------------------------------
Miller, James V (Research) wrote:
> Luis, 
> 
> You made some changes to GraftNthOutput in ImageSource that include throwing exceptions and performing a dynamic_cast.
> 
> Why was the dynamic_cast change necessary?  The output being grafted does not have to be same type as output 0.  If we are worried about the types, then it really has to be the same type as the output being grafted (indicated by idx) not output 0.  May have to check the typeid of graft and the selected output.
> 
> I am not sure the dynamic_cast as written will do anything.  The return type of GetOutput() has already been static_cast to be the same output 0 anyway.
> 
> Jim Miller 
> _____________________________________
> Visualization & Computer Vision
> GE Research
> Bldg. KW, Room C223
> 1 Research Circle, Schenectady NY 12309-1027
> 
> millerjv at research.ge.com <mailto:millerjv at research.ge.com>
> (518) 387-4005, Dial Comm: 8*833-4005
> Cell: (518) 505-7065, Fax: (518) 387-6981
> 
> 
> 
>