[Insight-developers] ImageAdapter::GetPixel

[Luis Ibanez]

Hi Jim,

You're right on both points.

The only GetPixel() method that should be in the ImageAdaptor class 
is the one returning a "const"  reference to a pixel.   The other 
is there just because the API should be the same of the itkImage class. 
But of course, if somebody try to use it, that will lead to trouble.

The SetPixel() method will supply the access to modification of pixel
values, so the non-const GetPixel() is no longer needed.

To make the API uniform, itkImage should also have SetPixel() and
GetPixel() methods declared as:

void  SetPixel( const Index & index, const PixelType & value);
const PixelType & GetPixel( const Index & index ) const;

But, even the version returning a "const" reference is now returning a
reference to a temporary variable, because the DataAccessor::Get() 
method is returning a value by copy. 

This is all bad style, and is only there trying to keep the aspect 
of the API.

The hope with the ImageAdaptor and DataAccessor was that the access
to image data were done only through iterators. The iterators 
(ImageIteratorWithIndex) are returning the pixel value by copy,
as the DataAccessor does. This is ok for small pixel types, but 
something to reconsider for larger pixel types.

If the GetPixel()/SetPixel() are to be keep, the DataAccessor will have
to have an internal variable of type   ExternalType, so the reference 
returned by the DataAccessor::Get() method is still valid after the
method ends. Or... the GetPixel() will return by value.

-------

What we have been experimenting with Stephen here, is to try to extend
the capabilities of DataAccessor, because they are now just stateless
classes, only their type is used to intercept the pixel access and 
perform a simple transformation on the way.

A possibility is to actually instantiate an object of type DataAccessor,
in the constructor of every iterator, image and adaptor, and use this 
object to perform the transformation. That will allow to have some state 
in the DataAccessor.

One of the interesting things to add to Accessors is the possibility of 
performing simple geometric transformations, which again, doesn't
justify to duplicate data in memory.  As far as we have experimented in 
this direction, it seems that the only way to avoid virtual functions 
in pixel access is to use only GetPixel()/SetPixel() as the official 
way to access pixel data. Even the iterators should use this itkImage 
(or itkImageAdaptor) methods for getting access to pixel values. There 
the challenge is to find a way to pass an Index as parameter for each 
pixel access without having a hit on performance...

--------------------------

About the "*" operator, the iterator should NOT provide it.  Because it
is ambiguous (it is doing Set() and Get() at the same time), and that
will prevent to introduce a DataAccessor in the middle. The Set/Get
methods allows to control what happens to data going to and comming 
from a pixel.

----------

Thanks for pointing this out,

----


How much to support Adaptor is an open question,...

It is clear that for converting an char image to a float you don't
want to use a filter and duplicate the data in memory..., in fact,
that's the case for any linear transformation of gray levels.

On the other hand, the difficulty is how to be sure that we don't 
pay for this functionality when we are not using it...


----


BTW,
this is a question about English and generic programming  jargon:

Should we say :

Adaptor or Adapter ?
Accessor or Acce...  ?

I have been using the "..or" termination following C++ use,
or maybe because it sounds familiar to me in spanish :-) 

but seems that the "er" termination is the right one...
any suggestions ?



Luis

----------------------




====================================
Jim Miller Wrote:

> The ImageAdapter class provides two versions of GetPixel which return
a reference to a pixel.  The
> implementation calls the DataAccessor::Get() method which returns a
pixel on the stack. Seems like
> this will cause a problem.

> It looks like we need to be very careful about returning references to
a pixel.  Does GetPixel() need
> to return a pixel on the stack in order to properly support adapters?

> Also, does this mean an iterator should NOT provide operator* ? Or not
provide an lvalue version?

> Jim Miller

--
______________________________________________________________________

Luis Ibanez
Research Assistant Professor - Division of Neurosurgery
University of North Carolina at Chapel Hill
CB# 7060, Chapel Hill, NC 27599
email : ibanez@cs.unc.edu       home  : http://www.cs.unc.edu/~ibanez
phone : (919)-843-9961          fax   : (919)-966-6627
______________________________________________________________________