#2134 closed enhancement (fixed)
Geoconcept export format support - New projections - Bug fixes
Reported by: | dgrichard | Owned by: | warmerdam |
---|---|---|---|
Priority: | normal | Milestone: | 1.6.0 |
Component: | OGR_SF | Version: | unspecified |
Severity: | normal | Keywords: | geoconcept |
Cc: | Even Rouault |
Description
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.
Attachments (13)
Change History (37)
by , 15 years ago
Attachment: | gdal-1.5.0.patch added |
---|
comment:1 by , 15 years ago
It would be great if you could do separate patches for each bug fix proposal and for your new driver.
by , 15 years ago
Attachment: | gdal-1.5.0-bug-gt_wkt_srs.patch added |
---|
bug fix gt_wkt_srs : wrong conversion angle operation
by , 15 years ago
Attachment: | gdal-1.5.0-bug-ogr_srs_proj4.patch added |
---|
bug fix for ogr_srs_proj4 : prime meridian and Greenwich shift counted twice
by , 15 years ago
Attachment: | gdal-1.5.0-ogr2ogr-sql.patch added |
---|
ogr2ogr sql option to support local file
by , 15 years ago
Attachment: | gdal-1.5.0-eqr-glabsgm.patch added |
---|
Support for eqr and glabsgm new proposed PROJ.4 projections
by , 15 years ago
Attachment: | gdal-1.5.0-geoconcept.patch added |
---|
Add support to geoconcept export format
comment:2 by , 15 years ago
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.
follow-up: 4 comment:3 by , 15 years ago
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.
comment:4 by , 15 years ago
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).
follow-ups: 6 10 comment:5 by , 15 years ago
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 ?
comment:6 by , 15 years ago
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.
follow-up: 8 comment:7 by , 15 years ago
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).
comment:8 by , 15 years ago
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.
follow-up: 11 comment:9 by , 15 years ago
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
comment:10 by , 15 years ago
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.
comment:11 by , 15 years ago
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.
comment:12 by , 15 years ago
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
follow-up: 15 comment:13 by , 15 years ago
Component: | default → OGR_SF |
---|---|
Keywords: | geoconcept added |
Milestone: | → 1.6.0 |
Status: | new → assigned |
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.
comment:14 by , 15 years ago
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?
comment:15 by , 15 years ago
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.
follow-up: 18 comment:16 by , 15 years ago
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
comment:17 by , 15 years ago
comment:18 by , 15 years ago
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.
by , 15 years ago
Attachment: | gdal-1.6.0-ogr_spatialref-20080223.patch added |
---|
synchronisation of trunk with new eqc/gausslab projection
by , 15 years ago
Attachment: | gdal-1.6.0-ogrspatialreference-20080223.patch added |
---|
synchronisation of trunk with new eqc/gausslab projection
by , 15 years ago
Attachment: | gdal-1.6.0-ogr_srs_api-20080223.patch added |
---|
synchronisation of trunk with new eqc/gausslab projection
by , 15 years ago
Attachment: | gdal-1.6.0-ogr_srs_proj4-20080223.patch added |
---|
synchronisation of trunk with new eqc/gausslab projection
by , 15 years ago
Attachment: | gdal-1.6.0-osr-20080223.patch added |
---|
synchronisation of trunk with new eqc/gausslab projection
comment:19 by , 15 years ago
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)
comment:20 by , 15 years ago
Cc: | added |
---|
follow-up: 22 comment:21 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
comment:22 by , 15 years ago
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 ?
follow-up: 24 comment:23 by , 15 years ago
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.
comment:24 by , 15 years ago
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
patch of gdal 1.5.0