[Insight-developers] RE: Changes to ImageSource::GraftNthOutput
Miller, James V (Research)
millerjv at crd.ge.com
Thu Dec 1 11:49:20 EST 2005
Luis,
Looked through the changes quickly.
1. There are some typos in the comments to DataObject.h
2. Should ImageBase keep both versions of Graft(), one that takes a
DataObject and one that takes an ImageBase. I suppose keeping them
both maintains backward compatibility.
3. In ImageSource, you made the opposite decision, changing the function
to take an DataObject instead of taking an ImageBase.
4. Should Image::Graft() also be changed to take a DataObject? I understand
why the code works without changing Image::Graft() but it not obvious.
(ImageSource::Graft(DataObject*) calls the virtual function from
DataObject::Graft(DataObject*) which gets mapped ImageBase::Graft(DataObject*) which calls
the virtual function ImageBase::Graft(ImageBase*) which gets mapped to Image::Graft(ImageBase*) ).
This may confuse the innocent. Maybe all versions of Graft() should take a DataObject*. The
only downside to this is that it will be dynamic_cast'ed multiple times.
Jim
-----Original Message-----
From: Luis Ibanez [mailto:luis.ibanez at kitware.com]
Sent: Thursday, December 01, 2005 11:16 AM
To: Miller, James V (Research)
Cc: Luis Ibanez; Insight Developers List; Lorensen, William E (Research)
Subject: Re: [Insight-developers] RE: Changes to
ImageSource::GraftNthOutput
Jim,
The fixes for the Graft methods were committed this morning:
http://www.itk.org/Testing/Sites/dash12.kitware/Win32-vs70/20051201-1219-Continuous/Update.html
They seem to be doing ok in the Continuous builds.
Unless there are any objections, I will commit the same
fixes on to the ITK 2.4 branch and update the tarball files.
Please let me know if you see any reason for not patching
the branch.
Thanks
Luis
---------------------
Luis Ibanez wrote:
> Hi Jim,
>
> The solution that you suggested seems to be the right way to go.
>
> I'm testing that implementation in an Experimental build, and
> if it looks good I'll commit it tonight.
>
>
> Thanks
>
>
> Luis
>
>
> ------------------------------------
> Miller, James V (Research) wrote:
>
>> 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
>>>
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers at itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers
>>
>>
>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers
>
>
More information about the Insight-developers
mailing list