[Insight-developers] RE: Changes to ImageSource::GraftNthOutput
Luis Ibanez
luis.ibanez at kitware.com
Wed Nov 30 19:21:49 EST 2005
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
>
>
More information about the Insight-developers
mailing list