[Gdal-dev] Changes to and upgrade of the Perl bindings

[Mark Overmeer]

* Ari Jolma (ari.jolma at tkk.fi) [060612 10:12]:
> I believe the latest version of Geo::GDAL Perl interface(1) to GDAL and
> OGR could be the Perl interface for the next release (1.3.3) of GDAL.
> 
> Now, dear PMC of GDAL, I give the ball to you. Instructions, please :-)
> 
> Is the source tree in Subversion already? I still have a working account
> on the CVS, I believe. Do I just commit the changes?
> 
> Ari
> 
> (1)http://map.hut.fi/gdal-perl/Geo-GDAL.html

As author of Geo::Proj4 (and Perl freak in general) I would suggest
a few changes to the interface you plan.  Is this url still the
leading design?

Although a most GDAL (and family) library code uses things which resemble
objects, that does not directly map on an object-oriented implementation
in Perl.  So: if you wish to create an OO interface, you need to start
with seperating your interface thoughts more from what is present in
the libs.  Most importantly: real OO hides internal complexity.

Besides, you can benefit much more from complex Perl features.  I do
not know the specifics of the GDAL libraries (yet), but you could do:

  sub warp(@)
  {   my ($self, %args) = @_;
      my $proj = $args{projection} || 'wgs84';
      ErrorReset;
      my $result = DoWarpNow($proj);
      return $result if defined $result;
      $! = GetLastErrorNo;
      $@ = errtype_to_text(GetLastErrorType).': ',GetLastErrorMsg;
      undef;
  }

  my $image = GDAL::Image->new(....);

  my $result = $image->warp(projection => 'nad83')
     or die $@;
   
Furthermore, it would be nice to follow Perl's naming practice (although
not abligatory) to name packages like Gdal::Driver or GDAL::Driver
(each word starts with a cap), and methods with caps per word
accept the first:   ->finderClean().

Things like this:
    $driver->{LongName}
are a crime against OO.  It should be:
    $driver->longName;
    sub GDAL::Driver::longName() {shift->{LongName}}

GetDescription, SetDescription are in Perl usually merged as
   sub description(;$)
   {   my $self = shift;
       @_ ? ($self->{descr} = shift) : $self->{descr};
   }
So: both setter and getter in one.  On the other hand: I would not
permit people to change the description under fly... only accept
it during (driver)object initiation.  The getter then becomes
   sub description() {shift->{descr}}


RasterXSize $dataset->{RasterXSize}
RasterYSize $dataset->{RasterYSize}
could better become:
   sub rasterDimensions()
   {   ...
       (RasterXSize, RasterYSize);
   }
So people use it like
   my $meta = GDAL::Dataset->new(...);
   my ($w, $h) = $meta->rasterDimensions;

If things like
 SetGeoTransform:
  @transform = ($minX, $dx, 0, $maxY, 0, $dy);
  $dataset->SetGeoTransform(\@transform);
are preparations for actions, you should make it parameters to that
action, not a seperate public method.  For instance, in the above
fake warp() method:

  sub warp(@)
  {   my ($self, %args) = @_;
      ...
      if(my $transform = $args{transform})
      {   SetGeoTransform($self->{c_object}, @$transform);
      }
  }

  my $result = $image->warp
    ( projection => 'utm'
    , transform  => [ $minX, $dx, 0, $maxY, 0, $dy ]
    ) or die $@;

This way, you reduce the amount of knowledge a user needs to have to
use the library correctly.

Except when you have to pass multiple different arguments, avoid
the need for people to use references (most people do not understand
them well enough).  So:
  $dataset->SetGeoTransform(\@transform);
should become
  $dataset->SetGeoTransform(@transform);
or
  $dataset->SetGeoTransform($minX, $dx, 0, $maxY, 0, $dy);

Counters over arrays are a death-sin on Perl, except when you cannot
avoid them.  This:
 GetGCPs:
  my $gcps = $dataset->GetGCPs();
  for (0..$#$gcps) {
    $x[$_] = $gcps->[$_]->{GCPX};
    $y[$_] = $gcps->[$_]->{GCPY};
  }

should become
  my $gcps = $image->GCPs;
  my @x = map {$_->{GCPX}} @$gcps;
  my @y = map {$_->{GCPY}} @$gcps;

but probably nicer is:

  my $gcps   = $image->GCPs;
  my @points = map { [ $_->{GCPX}, {$_->{GCPY} ] } @$gcps;

or better, let GCPs() return a list, then

  my @points = map { [ $_->{GCPX}, {$_->{GCPY} ] }
                  $image->GCPs;
  foreach my $point (@points)
  {   print "X=$point->[0], Y=$point->[1]\n";
  }

or better, hide this knowledge to the user

  foreach my $point ($image->GCPpoints) ...
  
  package GDAL::Image;
  sub GCPpoints()
  {   my $self = shift;
      map { [ $_->{GCPX}, {$_->{GCPY} ] }
                  $self->metadata->GCPs;
  }

Your objects are:
    * gdal::MajorObject
        'major' should be 'base' or 'super', to follow OO terms.  What
        about:  GDAL::Object
    * gdal::Driver        GDAL::Driver
    * gdal::Dataset       GDAL::Image::Metadata
    * gdal::Band          GDAL::Image::Band
    * gdal::ColorTable    GDAL::Image::ColorTable
    * gdal::GCP           GDAL::GCP
And another object: GDAL::Image which combines related metadata and bands.
   print $image->band(3)->name;
"Image" or "Scene" or ... ?

I would also defined a transform object
   my $transform = GDAL::Transform->new(....);
   my $result = $transform->from

So: please first try to find the easiest user interface (write the
test-scripts first), and them map that onto the great features of GDAL.
-- 
Regards,
               MarkOv

------------------------------------------------------------------------
       Mark Overmeer MSc                                MARKOV Solutions
       Mark at Overmeer.net                          solutions at overmeer.net
http://Mark.Overmeer.net                   http://solutions.overmeer.net