Opened 8 years ago

Last modified 8 years ago

#5442 closed enhancement

OGR Support for Selafin format — at Version 4

Reported by: fhissel Owned by: fhissel
Priority: normal Milestone:
Component: OGR_SF Version: unspecified
Severity: normal Keywords: selafin driver ogr
Cc:

Description (last modified by fhissel)

Selafin is a format used by some finite element software (mostly Telemac hydraulic model) to store numerical values at different points or elements and for different time steps.

This driver allows read-write support for Selafin format in OGR.

Change History (7)

Changed 8 years ago by fhissel

Attachment: f2d_bowl_fe.slf added

Test case 1

Changed 8 years ago by fhissel

Attachment: r2d_tide-jmj_real_gen.slf added

Test case 2

comment:1 Changed 8 years ago by Even Rouault

A few remarks from my code review :

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string
  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.
  • port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping
  • the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.
  • the CreateLayer?() and DeleteLayer?() should be completely removed, or disabled with #ifdef notdef if you intend implementing them one day.
  • Same for CreateDataSource?(), DeleteDataSource?().
  • there are very long lines with if else clause on the same line, which make them difficult to read
  • the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Questions :

  • did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?
  • did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant
  • did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Wishes :

  • could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

comment:2 in reply to:  1 ; Changed 8 years ago by fhissel

Owner: changed from warmerdam to fhissel
Status: newassigned

Replying to rouault:

Thanks for your code review! I'm going to fix the driver.

A few remarks from my code review :

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

  • port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping

OK, I will use those macros.

  • the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.

OK, I will change that

OK

  • there are very long lines with if else clause on the same line, which make them difficult to read

OK

  • the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Yes, I will attach new smaller files.

Questions :

  • did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?

Yes, all tests passed without error (at least the first 100 or so, until I stopped the program).

  • did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant

It was OK with gcc under Linux, but I did not test is with MSVC. I will try it.

  • did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Yes, valgrind --leak-check=full did not detect any leak.

Wishes :

  • could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

OK, I am going to add one.

comment:3 in reply to:  2 ; Changed 8 years ago by Even Rouault

Replying to fhissel:

Replying to rouault:

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

Indeed. I missed that line. So that part is OK.

  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

Yes, I know. I'm generally suspicious about long use - I've seen many times wrong uses like fread(&a_long, 1, 4, f) -, but in your code, it looks OK.

Changed 8 years ago by fhissel

Attachment: test2.slf added

Test case

comment:4 in reply to:  3 Changed 8 years ago by fhissel

Description: modified (diff)

Replying to rouault:

Replying to fhissel:

Replying to rouault:

I've just uploaded a new version of the driver. This one supports writing to Selafin format. A smaller test case and an autotest script are attached.

The driver has been tested with valgrind and test_ogrsf (there is one error in test_ogrsf but it is caused by a normal renumbering of FIDs when inserting or deleting a feature in some kind of layers).

I have not tried to compile it with MSVC (no Windows available).

Note: See TracTickets for help on using tickets.