[Gdal-dev] Drivers for Golden Software Grid Formats (long)

[Kevin Locke]

Hi Andrey,

Thanks very much for your input.  New version is at
http://kevinlocke.name/gdal/gdal-driver_for_gsg-r2.patch

On Mon, 2007-01-29 at 14:52 +0300, Andrey Kiselev wrote:
> On Sun, Jan 28, 2007 at 10:11:25AM -0500, Kevin Locke wrote:
>> I have recently written drivers for the Golden Software ASCII and
>> Binary Grid formats that includes read, write, create, delete, and
>> copy capabilities.  I was hoping to get some constructive criticism
>> and, if no major problems are present, that it could be considered for
>> inclusion in future releases of GDAL.
> 
> The patch looks fine to me, I just have few notes.
> 
> 1. The binary driver could be derived from the RAWDataset class, just
>    like the PNM driver. It prevents code duplication and provides us
>    with free of charge Create() method. Also RAWDataset has several
>    optimizations for speed. But the min/max collecting ability will be
>    lost here, of course.

That's a good idea, I hadn't even noticed that class.  The only
drawbacks of not updating the min/max that I have observed is that
Surfer (v8) uses these values to set contour interval and a few other
measurement presets (I'll check more thoroughly this afternoon).  So
it may not be a big problem to leave them untouched, as long as the
data does not change significantly.  Do you think it is worth the
speed/elegance improvements to have the driver subclass RawDataset at
the risk of some incompatibility with other programs?  If you do,
should I have the GetMinimum()/GetMaximum() functions return the value
in the header?

> 2. Ther are people which can be allergic to things like
>    'std::numeric_limits<double>::max()' or 'dynamic_cast' :-)

I've removed the dynamic_casts.  The numeric_limits stuff will take
some thought in a few places, but I should be able to get it done
shortly.

>    (BTW, why do you need dynamic_cast here?).

Nah, not really.  It was just a sanity check (and really unnecessary
in most places).

>    Also we have CPLString
>    class which is derived from std::string and properly wrapped to be
>    portable. Please, use that one instead of std::string.

Done.

> 3. You don't need EXTRAFLAGS parameter in makefile.vc.

Done.

> 4. It is better to place both ASCII and binary modules in the same
>    directory, just to be more compact.

Done.

> 5. RCS Id tag should look like $Id:$, otherwise it won't be recognized
>    as tag.

Done.

>> First, is there a portable file truncate function? [...]
> 
> It seems we do not have the one, though it can be added.

That would be appreciated.  For the ASCII format, grid lines are
likely to change length on every write and setting min/max at the end
of copy() can also require some shifting.  My approach of padding the
end with spaces seems to work, but it is less than elegant.

-- 
Thanks again,   |      kwl7 at cornell.edu     |    kevinoid at jabber.org
Kevin           |   http://kevinlocke.name  |   kevinoid on freenode