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

[Ari Jolma]

Mark Overmeer kirjoitti:
>>
>> (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?

Yes. That's the design for the main interface. The methods are the same
as before and in the old docs http://map.hut.fi/Geo/GDAL/gdal.html etc.

> 
> 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().

Is the naming practice written down somewhere? I know some modules use
finderClean naming but I've thought that's more Java's naming practise.
I thought finder_clean was used more in Perl. Here I'm trying to stick
to the naming and names that are in GDAL/OGR itself. My thinking is that
it should be easy to port GDAL scripts written in Python or Ruby to Perl
and vice versa.

My hands are also tied up a bit because of the Swig interface, where the
names are mostly defined and they are shared among all languages.

The methods usually call croak, if they fail.

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

Swig does some magic to tie things like $driver->{LongName} to getter
and setter functions.

> 
> 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.  

Things like these are partly due to my poor knowledge of Swig. Current
versions of Swig support adding Perl code to the modules it generates,
so Perlifying things this way are more easy to do now.

>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);

I have to admit this is because I did not know how to achieve the
latter, which is better also IMHO, in Swig :-(

> 
> 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;
>   }

ah, but that's just an example in the docs...

> 
> 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 ... ?

Bands are accessed through Dataset, which is thus essentially the Image
class.

I wrote the constructor functions into the Geo::GDAL module, which are
probably yet another OO sin, because I did not know how to get from
having classes like Geo::GDAL::gdal::Band (or
Geo::GDAL::something::Band) that I get from Swig to Geo::GDAL::Band,
which would be desirable. I mean without having to change the shared
swig files too much -- i.e. write #ifdef SWIGPERL ...

I'm also a bit worried about performance penalty if I introduce yet
another layer by inheriting from Geo::GDAL::gdal::Band to Geo::GDAL::Band

There is also the problem that the GDAL library has to be used through
the swig wrappers with care, things like $image->band(3)->name are
usually dangerous, since the reference counting systems in high level
languages and in GDAL itself are disconnected.

> 
> 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.

The current path in implementing the Perl interface to GDAL/OGR is

1) get a working interface
2) make it CPAN compatible (namespace mostly)
3) make it more Perl / fix the reference counting

We are at step 1) now. If we concentrate now just on my proposal for
Geo::GDAL and forget the other things, you're saying that:

- make it more OO (there shouldn't be any functions as in my design)
- declare the classes into Geo::GDAL namespace as (my interpretation)

Geo::GDAL::Image
Geo::GDAL::Image::Driver
Geo::GDAL::Image::Band
Geo::GDAL::Image::ColorTable
Geo::GDAL::GCP
Geo::GDAL::Transformation # I'm not very sure what classes we need for
                          # SRSs etc.
Geo::GDAL::Database # a short form of GeographicDatabase or 	

                    # VectorDatasource
Geo::GDAL::Database::Driver
Geo::GDAL::Layer
Geo::GDAL::Feature
Geo::GDAL::FeatureDefn
Geo::GDAL::FieldDefn
Geo::GDAL::Geometry

Should the last ones be Geo::GDAL::Database::Layer etc?

So, instead of a function GetGDALDriverCount we'd have a static function
Geo::GDAL::Image::Driver::Count that returns the number of available
raster drivers. And instead of CreateLayer we'd call Geo::GDAL::Layer->new.

Probably the easiest way would be to inherit the new classes from what
Swig produces. The method calls could be made better, for example it is
possible to add functionality later so that
$dataset->SetGeoTransform($minX, $dx, 0, $maxY, 0, $dy); works, while we
can keep the old $dataset->SetGeoTransform([$minX, $dx, 0, $maxY, 0,
$dy]); as deprecated form. In this step it is important to get the
namespace fixed.

It would be good to have as closely matching names as possible in all
wrapper languages.

Thanks for your comments!

Ari


-- 
Prof. Ari Jolma
Kartografia ja Geoinformatiikka / Cartography and Geoinformatics
Teknillinen Korkeakoulu / Helsinki University of Technology
tel: +358 9 451 3886 address: POBox 1200, 02015 TKK, Finland
Email: ari.jolma at tkk.fi URL: http://www.tkk.fi/~jolma