Geoconcept export format support - New projections - Bug fixes

[dgrichard]

Provide a first support (writing and updating) to Geoconcept export format (French GIS). Add support to the Equidistant Cylindrical Sphere et Gauss Laborder Sphere Geometric Mean projections. Bug fixes in frmts/gtiff/gt_wkt_srs.cpp (angle conversion) and port/cpl_list.cpp (when nPosition is 0) Add --sql option to read local files in ogr2ogr application. Patch provided in ticket's attachment.

[dgrichard]

patch of gdal 1.5.0

[Even Rouault]

It would be great if you could do separate patches for each bug fix proposal and for your new driver.

[dgrichard]

cpl_list bug fixe proposal

[dgrichard]

bug fix gt_wkt_srs : wrong conversion angle operation

[dgrichard]

bug fix for ogr_srs_proj4 : prime meridian and Greenwich shift counted twice

[dgrichard]

ogr2ogr sql option to support local file

[dgrichard]

Support for eqr and glabsgm new proposed PROJ.4 projections

[dgrichard]

Add support to geoconcept export format

[dgrichard]

Full patch splited into dedicated patches as required. Apply bug fixes patches before other patches as some files got modified several times for different reasons.

[Even Rouault]

Your patch gdal-1.5.0-bug-cpl_list.patch drawn to my attention the fact that CPLList API implementation isn't tested and used at all in GDAL code. I've fixed bugs (crashes && behaviour not working as espected) in CPLListInsert and CPLListRemove. Applied in trunk in r13476, in branches/1.5 in r13478 and I've added tests in C++ unit test framework in r13477.

[dgrichard]

Replying to rouault:

Your patch gdal-1.5.0-bug-cpl_list.patch drawn to my attention the fact that CPLList API implementation isn't tested and used at all in GDAL code. I've fixed bugs (crashes && behaviour not working as espected) in CPLListInsert and CPLListRemove. Applied in trunk in r13476, in branches/1.5 in r13478 and I've added tests in C++ unit test framework in r13477.

I noticed there is no CPLList use in all gdal, which is not helpful when developping some code that needs this list concept. The main problem I have encountered is in using 0 as position in CPLListmodules, that's why I had to change the original code to fix that problem.

in the submitted patch, change Revone by Remove (my mistyping).

[Even Rouault]

Yes, the main issues were with index 0. It should be fixed now.

As far as gdal-1.5.0-bug-gt_wkt_srs.patch and gdal-1.5.0-bug-ogr_srs_proj4.patch are concerned, it is not obvious for me that these patches are necessary/correct. But I'm not a specialist in these areas. Anyway, do you have test cases/files that show that current GDAL behaviour is incorrect ?

[dgrichard]

Replying to rouault:

As far as gdal-1.5.0-bug-gt_wkt_srs.patch and gdal-1.5.0-bug-ogr_srs_proj4.patch are concerned, it is not obvious for me that these patches are necessary/correct. But I'm not a specialist in these areas. Anyway, do you have test cases/files that show that current GDAL behaviour is incorrect ?

for gdal-1.5.0-bug-gt_wkt_srs.patch, I have to be in my office to get the use case. I just remember that the GTiff metadata were incorrect when after using gdalwarp or gdal_translate on "+proj=lcc +nadgrids=ntf_r93.gsb,null +a=6378249.2000 +rf=293.4660210000000 +pm=2.33722916655 +lat_0=46.800000000 +lon_0=0.00000000000 +k_0=0.99987742 +lat_1=46.800000000 +x_0=600000.000 +y_0=2200000.000 +units=m +no_defs" to "+proj=lcc +towgs84=0.0000,0.0000,0.0000 +a=6378137.0000 +rf=298.2572221010000 +lat_0=46.500000000 +lon_0=3.00000000001 +lat_1=44.000000000 +lat_2=49.000000000 +x_0=700000.000 +y_0=6600000.000 +units=m +no_defs". But, you'd better way for feed-back next week. This bug might be related with gdal-1.5.0-bug-ogr_srs_proj4.patch.

for gdal-1.5.0-bug-ogr_srs_proj4.patch, the bug is based on the "+proj=lcc +nadgrids=ntf_r93.gsb,null +a=6378249.2000 +rf=293.4660210000000 +pm=2.33722916655 +lat_0=46.800000000 +lon_0=0.00000000000 +k_0=0.99987742 +lat_1=46.800000000 +x_0=600000.000 +y_0=2200000.000 +units=m +no_defs" to "+proj=lcc +towgs84=0.0000,0.0000,0.0000 +a=6378137.0000 +rf=298.2572221010000 +lat_0=46.500000000 +lon_0=3.00000000001 +lat_1=44.000000000 +lat_2=49.000000000 +x_0=700000.000 +y_0=6600000.000 +units=m +no_defs", the prime meridian is counted twice. The use case is just to apply a gdalwarp from the first to the second.

BTW, I have inverted the files in the gdal-1.5.0-bug-ogr_srs_proj4.patch (lines have been added, to the new ogr_srs_proj4.cpp) when I have (re)calculated the diff for splitting the first submitted general patch.

[Even Rouault]

I've tested a bit your OGR GeoConcept driver (translation of GPX trackpoints). Works nicely. (gdal-1.5.0-eqr-glabsgm.patch was also necessary to apply to compile the driver)

I had to make the following 2 changes:

• This one fixes a crash when I tried to read a txt/gxt file with ogrinfo. By the way, do you have any plans adding read support (I've recently found a few old TXT Geoconcept files...) ?

diff -u gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp
--- gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp    2008-01-06 15:43:50.000000000 +0100
+++ gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp  2008-01-06 14:43:55.000000000 +0100
@@ -50,6 +50,7 @@
     _nLayers = 0;
     _bSingleNewFile = FALSE;
     _bUpdate = FALSE;
+    _papszOptions = NULL;
 }

• This one fixes a memory leak :

diff -u gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp
--- gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp 2008-01-06 15:43:50.000000000 +0100
+++ gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp       2008-01-06 15:32:03.000000000 +0100
@@ -95,7 +95,6 @@

 {
     GCField* aField;
-    OGRFieldDefn* ofd;
     int n, i;

     if( (_hGCT= Open_GCIO(pszName,pszExt,pszMode,pszGCTName))==NULL )
@@ -202,8 +201,8 @@
             oft= OFTString;
             break;
           }
-          ofd= new OGRFieldDefn(GetFieldName_GCIO(aField), oft);
-          _poFeatureDefn->AddFieldDefn(ofd);
+          OGRFieldDefn ofd(GetFieldName_GCIO(aField), oft);
+          _poFeatureDefn->AddFieldDefn(&ofd);
         }
       }
     }

As creation of GCT is a bit tedious when the source layer has many attributes, I wonder if the driver couldn't autogenerate the missing attribute definitions (SECTION FIELD) from the feature info of the source features given by OGR (the ID would be the only thing we cannot get from OGR).

[dgrichard]

Replying to rouault:

I've tested a bit your OGR GeoConcept driver (translation of GPX trackpoints). Works nicely. (gdal-1.5.0-eqr-glabsgm.patch was also necessary to apply to compile the driver)

Yes, because of SRSs I have put support to (geoconcept_syscoord.c). This source is quite cumbersome ... I am, again, awaiting, for Geoconcept to add more SRSs. I have the same problem with character sets. Only ANSI is supported ...

I had to make the following 2 changes:

• This one fixes a crash when I tried to read a txt/gxt file with ogrinfo. By the way, do you have any plans adding read support (I've recently found a few old TXT Geoconcept files...) ?

Could you send them to me, I will try to add the read feature, but it is tedious as old geoconcept file use the incremental coordinates (FORMAT equals 1) where the new GXT file use plain coordinates (FORMAT equals 2)!

The other trick is that I am still trying to analyze the export line : each field is separated from the other with the DELIMITER character, but the coordinates are also separated with a character that seems to always be the tab one. I am waiting for Geoconcept to get more information on that topic.

diff -u gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp
--- gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp    2008-01-06 15:43:50.000000000 +0100
+++ gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptdatasource.cpp  2008-01-06 14:43:55.000000000 +0100
@@ -50,6 +50,7 @@
     _nLayers = 0;
     _bSingleNewFile = FALSE;
     _bUpdate = FALSE;
+    _papszOptions = NULL;
 }

Thanks!

• This one fixes a memory leak :

diff -u gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp
--- gdal-1.5.0-geoconcept.ori/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp 2008-01-06 15:43:50.000000000 +0100
+++ gdal-1.5-geoconcept/ogr/ogrsf_frmts/geoconcept/ogrgeoconceptlayer.cpp       2008-01-06 15:32:03.000000000 +0100
@@ -95,7 +95,6 @@

 {
     GCField* aField;
-    OGRFieldDefn* ofd;
     int n, i;

     if( (_hGCT= Open_GCIO(pszName,pszExt,pszMode,pszGCTName))==NULL )
@@ -202,8 +201,8 @@
             oft= OFTString;
             break;
           }
-          ofd= new OGRFieldDefn(GetFieldName_GCIO(aField), oft);
-          _poFeatureDefn->AddFieldDefn(ofd);
+          OGRFieldDefn ofd(GetFieldName_GCIO(aField), oft);
+          _poFeatureDefn->AddFieldDefn(&ofd);
         }
       }
     }

Okay, I missed that one.

As creation of GCT is a bit tedious when the source layer has many attributes, I wonder if the driver couldn't autogenerate the missing attribute definitions (SECTION FIELD) from the feature info of the source features given by OGR (the ID would be the only thing we cannot get from OGR).

In that case I have to make the CONFIG dsco option optional. I have tried to implement schema discovering at line 533 of ogrgeoconceptlayer.cpp (in CreateField() module) and in the CreateFeature() (lines 253 to 365). The key point is that a geoconcept export file can hold lot of features and when updating/appending it, on-the-fly feature definitions implies rewriting the header with the new types/subtypes. Again, I have to ask Geoconcept whether types/subtypes can be written anywhere in the export file (this could help me!-). I will try my best with this feature too.

[Even Rouault]

In fact, the files I'm refering to come from IGN... There are 5 products : GéoFLA, GéoFLCP, GéoFLP, Route120 and Route500 dating back to 1999. Each one with subdirectories for Arcinfo, Arcview, EDIGéo, GEOCONCEPT and MapInfo. I can't probably attach them here, but I could provide some to you by email if you can't find them on the IGN servers ;-) And, yes, they use the incremental coordinate encoding scheme.

One short extract :

//Fichier issu de la traduction du lot R500_4
// traduction des 223 objets de [COMMUNE]
//$2DOBJECT
15      Commune Commune         1       1557    899023  1868508 57      281     68      789     806     -109    565     54      868     106     247     258     127     144
     212     66      283     -251    465     195     540     175     231     29      288     -260    511     -552    177     -368    366     -266    -6      -465    337
     -267    510     -231    1122    -290    503     -243    153     -121    -259    -592    -605    -639    -940    -187    -137    -564    -55     -254    -127    -87
     -267    -183    -219    -93     -278    69      -284    -68     -266    -252    -140    -123    -246    395     -320    -50     -275    -519    -153    -706    -512
    -172    -226    320     -489    226     -183    822     -308    220     -185    140     -256    269     -48     26      -254    -194    -553    292     19      262     120     755     63      134     -250    232     -173    1150    -7      212     -179    -144    -207    57      -195    572     21

[martinoty]

Replying to rouault:

Yes, the main issues were with index 0. It should be fixed now.

As far as gdal-1.5.0-bug-gt_wkt_srs.patch and gdal-1.5.0-bug-ogr_srs_proj4.patch are concerned, it is not obvious for me that these patches are necessary/correct. But I'm not a specialist in these areas. Anyway, do you have test cases/files that show that current GDAL behaviour is incorrect ?

As a matter of fact, gdal-1.5.0-bug-gt_wkt_srs.patch solves bug #601 . It is not a severe bug, since it is only related to the display returned by gdalinfo. But it is disturbing, since one may believe that there is a problem in tiff's metadata, whereas it is only a problem with gdalinfo. For an example, I think you can look at any file in Lambert II etendu: PARAMETER["latitude_of_origin",57.7777777777779] is wrong.

Secondly, gdal-1.5.0-bug-ogr_srs_proj4.patch solves #367 . It is critical, since every projection using +pm= values is affected. In particular, French users cannot get right results if this is not corrected.

[dgrichard]

Replying to rouault:

In fact, the files I'm refering to come from IGN... There are 5 products : GéoFLA, GéoFLCP, GéoFLP, Route120 and Route500 dating back to 1999. Each one with subdirectories for Arcinfo, Arcview, EDIGéo, GEOCONCEPT and MapInfo. I can't probably attach them here, but I could provide some to you by email if you can't find them on the IGN servers ;-) And, yes, they use the incremental coordinate encoding scheme.

No problem with that files, they were (and are still) produced with an IGN's in-house software developed by my team !-) The Geoconcept pilot I submitted is a port of our own software.

[Even Rouault]

martinoty, dgrichard, can you confirm that gdal-1.5.0-bug-ogr_srs_proj4.patch is still necessary with gdal 1.5.0 ? If this patch is for fixing #367, I - think - this bug has been fixed in GDAL 1.4.4. See revision r12535

FrankW will probably better know what to do for gdal-1.5.0-bug-gt_wkt_srs.patch and #601

[warmerdam]

I have applied the glabsgm and geoconcept patches in trunk (r13482 and r13483).

I understand Even has applied the CPLList patch.

I think the bug-gt-wkt_srs, bug-ogr_srs_proj4 patches need to be broken out into separate bug reports, with some more explanation of what they are fixing, and demonstration data if they aren't already fixed as Even suspects.

I am not too keen on the ogr2ogr-sql patch. Long SQL statements can be passed to ogr2ogr using the --optfile argument to refer to some options coming from a file.

[warmerdam]

Note, I applied the geoconcept patch "as is". Even, could you please update and apply any fixes noting the revision number here for Didier to review?

[dgrichard]

Replying to warmerdam:

I think the bug-gt-wkt_srs, bug-ogr_srs_proj4 patches need to be broken out into separate bug reports, with some more explanation of what they are fixing, and demonstration data if they aren't already fixed as Even suspects.

Ok, I'll try to post demo data with/without the fixes. Do I have to post the bug reports ? If yes, I'll do it the demo data.

I am not too keen on the ogr2ogr-sql patch. Long SQL statements can be passed to ogr2ogr using the --optfile argument to refer to some options coming from a file.

Ok, in that case, this patch is useless.

[warmerdam]

Yes, please create a distinct ticket for each of the gt-wkt_srs and ogr_srs_proj4 issues. Attach the patches and details on how to reproduce the original problem for each ticket.

Thanks

[Even Rouault]

Didier, I've applied the 2 fixes mentionned above in r13491 and in r13492

[dgrichard]

Replying to warmerdam:

Yes, please create a distinct ticket for each of the gt-wkt_srs and ogr_srs_proj4 issues. Attach the patches and details on how to reproduce the original problem for each ticket.

#367 has been fixed in 1.4.4. My bug fix came from my patched 1.4.2. It can be trashed. #601 is still true. I will attached an image and 2 outputs from gdalinfo without/with the fix, plus the gt-wkt_srs patch.

[dgrichard]

Generalized eqc and glabsgm patch

[dgrichard]

synchronisation of trunk with new eqc/gausslab projection

[dgrichard]

synchronisation of trunk with new eqc/gausslab projection

[dgrichard]

synchronisation of trunk with new eqc/gausslab projection

[dgrichard]

synchronisation of trunk with new eqc/gausslab projection

[dgrichard]

synchronisation of trunk with new eqc/gausslab projection

[Even Rouault]

gdal-1.6.0-*-20080223.patch series of 5 patches applied in r13868

(the swig one has been slightly modified. there was a missing closing brace)

[Even Rouault]

Didier,

I'm closing this bug as I think that everything that was pending has been applied... The only remaining patch, gdal-1.5.0-bug-gt_wkt_srs.patch, not yet applied, is attached to #601.

[dgrichard]

Replying to rouault:

Didier,

I'm closing this bug as I think that everything that was pending has been applied...

Ok, it is a great news! Thanks a lot for your help/advices.

The only remaining patch, gdal-1.5.0-bug-gt_wkt_srs.patch, not yet applied, is attached to #601.

Yeap, any idea when it will be applied ?

[Even Rouault]

Hum, a question. In the latest patches applied, OSRSetEquidistantCylindricalSphere that you introduced in previous versions was removed and replaced by OSRSetEquirectangular2 ? Is it intentional ?

As far as the patch for #601, I looked at the example you provided and your patch seems right. I would like Frank to confirm that's OK before applying though.

[dgrichard]

Replying to rouault:

Hum, a question. In the latest patches applied, OSRSetEquidistantCylindricalSphere that you introduced in previous versions was removed and replaced by OSRSetEquirectangular2 ? Is it intentional ?

Yes indeed : the OSRSetEquirectangular() does not hold the latitude of origin. The new EQUIRECTANGULAR projection (eqc) in PROJ.4 svn has been modified with this parameter with Gerald, Frank and I upon a proposal from IGNF. In order not to break courant codes using OSRSetEquirectangular(), I added OSRSetEquirectangular2(). In the end, the new eqc should replace the old one.

As far as the patch for #601, I looked at the example you provided and your patch seems right. I would like Frank to confirm that's OK before applying though.

Ok