[PATCH] Miscellaneous S-57 output bug fixes and improvements

[freekvw]

Attached are five patches to the GDAL iso8211 writer and OGR S-57 writer to fix compliance issues with the standards. The patches are based on GDAL trunk from October 14th and are also available through my GitHub clone of the GDAL repository: ​https://github.com/freekvw/gdal/commits/trunk

Here are the summaries:

1. Fix ISO8211 array description strings (subfield label separator '!' spuriously output before first subfield if field is repeating).

2. Fix S-57 data structure types for SG2D and SG3D according to S-57 maintenance document 8.

3. Remove duplicate unit and field terminators from S-57 output; they are added by the ISO8211 implementation.

4. Don't output SG2D field for S-57 edges consisting of a straight line between two connected nodes (as per section 5.1.4.4 in the S-57 specification).

5. Allow customization of S-57 metadata in the DSID, DSSI, and DSPM fields through layer creation options.

[freekvw]

Patch 1

[freekvw]

Patch 2

[freekvw]

Patch 3

[freekvw]

Patch 4

[freekvw]

Patch 5

[Jukka Rahkonen]

Hi,

Because you obviously understand S-57 which I can't say about myself, could you tell your opinion about these old and still open S-57 tickets: #1648, #2041, #2101, #2776, and #5162? No need to fix them (it would be great, though) but just to write a short comment to each ticket if you think you have something to say for making the fix easier in the future.

[Jukka Rahkonen]

Sorry, I was meaning #2773 instead of #2776 in the previous comment.

[freekvw]

Hi jratike80, I have indeed had to learn a bunch of things about S-57 and (unfortunately ;-) ISO 8211, and could tell you about the general structure, but I only really went into the details of the writer, not so much the reader. Also, I know hardly anything about the update files (FYI, .000 files are base files on top of which changes (basically errata) described in .001, .002, etc. files are to be applied), which most of the tickets you mention are about. I've also not had to be bothered with character encodings yet, which seems to be a whole new can of worms in S-57. The tickets do all appear perfectly valid, and I'm not surprised that applying the updates is hard to get right. By the way, S-57 file names are supposed to be unique and encode the issuing agency in the first two characters. This is why people in the other tickets refer to a file name without uploading it.

[Jukka Rahkonen]

Thank you for the information. Unfortunately it does not help me to know that JP352BRG.000/001 is an update to some unique dataset if I can't get the same data into my hands for trying if current GDAL versions can handle the situation right so those old tickets may remain open for a while.

[freekvw]

Makes sense... Some of the issues seem to have enough information to potentially reconstruct the issue, but I agree it's probably easiest if you could get hold of the original examples. I don't have them either unfortunately. Hope my patches can get in, though, so I don't need to keep running a forked version of GDAL :-) Thanks, Freek.

[Even Rouault]

Freek, did you test that the generated files by the modified S57 driver can be read properly with the reading side of the driver ? For example by running the autotest/ogr/ogr_s57.py tests ? Side question: did you test your changes with non GDAL based readers ? Regarding testing your modified code, is running ogr2ogr from an existing S57 file sufficient to trigger the modified code paths ? For example ogr_s57_9 from ogr_s57.py

[freekvw]

Hi roualt, The main goal behind the patches was actually to improve compatibility with existing readers. I believe the changed code paths are hit by any non-trivial S-57 output like the ones in the test cases (which ran successfully). I have tested our own output files consistently against the GDAL reader used in QGis as well as SeeMyENC and Caris Easy View. We have not yet been able to make S-57 output from GDAL be understood by ESRI's software, but I will provide further patches if we find that out at some point (note though that, at least for us, this has never worked and is not a regression caused by these patches).

[Even Rouault]

I've briefly reviewed the patches and they look good. However regarding 0005-Allow-customization-of-S-57-metadata-in-the-DSID-DSS.patch, as you're introducing creation options, it would be appropriate to document them as a GDAL_DMD_CREATIONOPTIONLIST in RegisterOGRS57() (see for example frmts/gtiff/geotiff.cpp for an example of the XML syntax. should be straightforward) and in the driver HTML documentation as well. And the CPL_UNUSED attribute of char papszOptions (added recently in trunk) should now be removed.

[freekvw]

Patch 6

[freekvw]

Thanks for the comments. I've added some documentation (though a detailed explanation of the values should be looked up in the standard) and removed the CPL_UNUSED. Note that I wrote "Since GDAL/OGR X.Y.Z, ..." in the documentation, since I don't know when it will be available. I also changed the broken link to "Frank's S-57 Page" to point to archive.org and added a link to the main document of the standard.

[Even Rouault]

Thanks. Applied in trunk r28348 "S57: various compliance fixes in ISO8211 and S57 writer (patch by freekvw, #5798)"

[Even Rouault]

Milestone renamed