Opened 7 years ago

Closed 6 years ago

#6533 closed enhancement (fixed)

[PATCH] Northwood GRD write support

Reported by: jamesramm Owned by: warmerdam
Priority: normal Milestone: 2.2.0
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: NWT_GRD, driver, write, raster


This patch adds write support to the Northwood GRD driver (NWT_GRD). Northwood Grid is also referred to as Vertical Mapper Grid or MapInfo Grid

The read driver creates 3 'virtual' RGB bands (i.e. they are mapped from the single data band on disk), with the 4th band being the data band. In write mode, there is therefore just 1 band. To make switching between read/write mode less clunky, I think it would be helpful if the band order in read mode was data, R, G, B rather than the current R, G, B, data. Then the actual data band would always be band 1 regardless of the mode. This would prevent things like statistics being calculated in write mode then showing up in the wrong band for read mode.

However, this would be a breaking change (albeit relatively minor), so I have not included it in the patch, but I'd like seem feedback on whether such a change could be included?

The write support is not fully complete. There is potential to:

  • Allow different data types in write mode and just convert these to the on disk data type...
  • Support more creation options to alter the header/tab file information. This is less desirable as this stuff (transparency, colour options etc) only has any affect when opening the file in mapinfo;
  • Map the on-disk no data value to whatever was specified when writing, using the PAM dataset to store the desired no data value

Attachments (3)

NWT_GRD_WRITE_SUPPORT.patch (49.0 KB ) - added by jamesramm 7 years ago.
NWT_GRD_WRITE.patch (49.2 KB ) - added by jamesramm 6 years ago.
updates 13/06/2016
NWT_GRD_TEST.patch (1.9 KB ) - added by jamesramm 6 years ago.
python tests for write support

Download all attachments as: .zip

Change History (12)

by jamesramm, 7 years ago

Attachment: NWT_GRD_WRITE_SUPPORT.patch added

comment:1 by Even Rouault, 7 years ago

  • NWT_GRDDataset::FlushCache(): this method could potentially be called on a read-only dataset, so the call to UpdateHeader() should be condtionnalized. And potentially even in update mode, repeated calls to FlushCache() (or call of the dataset destructor) shouldn't cause the header to be rewritten if not needed. Some flag bUpdateHeaderNeeded could be used for that.
  • NWT_GRDDataset::GetProjectionRef() : the if ( pszProjection == NULL ) test will always be false since GDALPamDataset::GetProjectionRef() alway return a non NULL string (but possibly an empty string)
  • NWT_GRDDataset::SetProjection(): naming convention : OGRSpatialReference poSpatialRef --> OGRSpatialReference oSpatialRef ( po means pointer to object )
  • NWT_GRDDataset::SetProjection(): "poSpatialRef.importFromWkt( (char )&pszProjection );" will alter pszProjection. You should first do "char* pszTmp = pszProjection; oSpatialRef.importFromWkt(&pszTmp);"
  • NWT_GRDDataset::SetProjection(): memory leak: MITABSpatialRef2CoordSys() return should be freed with CPLFree()
  • NWT_GRDDataset::Create(): poDS->fp = VSIFOpenL(pszFilename, "wb");. The return should be checked against NULL.
  • NWT_GRDDataset::UpdateHeader(): The logic after " Info on shading" is a bit more complicated than needed. Could be changed to if (pGrd->bShowHillShade ) byDisplayStatus |= 1 << 6; if (pGrd->bShowGradient ) byDisplayStatus |= 1 << 7;
  • CPLsprintf( &sMax[0], "%f", pdfMax ); --> CPLsnprintf( sMax, sizeof(szMax), "%f", pdfMax ). Same for sMin
  • Use CPLAtof() instead of atof() to avoid issues with non C locale
  • Python tests in autotest/gdrivers/ would be welcome

comment:2 by jamesramm, 7 years ago

Thank you. I hope to implement the changes in the coming week.

by jamesramm, 6 years ago

Attachment: NWT_GRD_WRITE.patch added

updates 13/06/2016

by jamesramm, 6 years ago

Attachment: NWT_GRD_TEST.patch added

python tests for write support

comment:3 by jamesramm, 6 years ago

I've added updates addressing your comments and some tests.

comment:4 by Even Rouault, 6 years ago

trunk r34386 "NWT_GRD: add write support (patch by James Ramm, #6533 + fixes by myself)"

I mostly fixed your tests that didn't pass at all (perhaps you sent a wrong version?). A few memory leaks / uninitialized memory / C++11 syntax fixes in the code itself. And I've added an open option NUM_BANDS=1 or 4 as well (used by the tests)

comment:5 by Even Rouault, 6 years ago

trunk r34387 "NWT_GRD: fix MSVC compilation warnings"

comment:6 by Even Rouault, 6 years ago

trunk r34388 "NWT_GRD: fix Clang warnings about 64->32 cast and unused private members"

comment:7 by Even Rouault, 6 years ago

trunk r34389 "NWT_GRD: fix crash when CreateCopy() called with a 0 band src dataset (fix crash on misc_6)"

comment:8 by Even Rouault, 6 years ago

trunk r34391 "NWT_GRD: fix -Werror=unused-but-set-variable warning

comment:9 by Even Rouault, 6 years ago

Component: defaultGDAL_Raster
Milestone: 2.2.0
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.