Geoconcept Open() not careful enough

[warmerdam]

It seems that the geoconcept open function is not careful enough when trying to determine if a dataset is in fact a geoconcept file/fileset.

eg. 

warmerda@gdal64[75]% ogrinfo gdal
OGR: OGROpen(gdal/0x607320) succeeded as Geoconcept Export.
INFO: Open of `gdal'
      using driver `Geoconcept Export' successful.
INFO: Internal data source name `gdal/LICENSE.TXT'
      different from user name `gdal'.
OGR: GetLayerCount() = 0

(in this case gdal is my source directory).

[warmerdam]

I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?

We really need a test script for this driver! I don't really know if I've completely broken it.

[warmerdam]

Still problems with single files:

warmerda@gdal64[110]% ogrinfo Mills.mif 
OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept.
INFO: Open of `Mills.mif'
      using driver `Geoconcept' successful.
OGR: GetLayerCount() = 0

[warmerdam]

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.

    /* Let CreateLayer do the job ... */
    if( bUpdate )
    {
      _pszName = CPLStrdup( pszName );
      return TRUE;
    }

This logic will need to be updated in some non-obvious way by dgrichard.

[dgrichard]

Replying to warmerdam:

I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?

I have modified accordingly and my test script gives me the same results as before.

We really need a test script for this driver! I don't really know if I've completely broken it.

I have a small (for the moment) shell script for testing. It uses a Shapefile datasets (<1kb) to test basic creation and update as describe in drv_geoconcept.html. I will attach it to this ticket together with test files.

[dgrichard]

Replying to warmerdam:

Still problems with single files: {{{ warmerda@gdal64[110]% ogrinfo Mills.mif OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept. INFO: Open of `Mills.mif' using driver `Geoconcept' successful. OGR: GetLayerCount?() = 0 }}}

As for the moment, reading the Geoconcept file is still not implemented, I have just started to implement hight level API before coding the parsing of Geoconcept file at low level.

[dgrichard]

Replying to warmerdam:

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode. {{{ /* Let CreateLayer? do the job ... */ if( bUpdate ) { _pszName = CPLStrdup( pszName ); return TRUE; } }}} This logic will need to be updated in some non-obvious way by dgrichard.

Ok, I am going to check how to improve this (but, still need to code the parsing, I guess).

[dgrichard]

Shape for unit test (DBF)

[dgrichard]

Shape for unit test (SHP)

[dgrichard]

Shape for unit test (SHX)

[dgrichard]

Replying to dgrichard:

Replying to warmerdam:

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.

I propose a new patch geoconcept-20081301.patch with lot of changes including the logic. The reading support is about to be completed as asked by Even. There are some more improvements to add like on-the-fly Geoconcept schema creation. I have already posted a new shell script for unit tests.

[dgrichard]

patch against trunk with better handling of read and SRS and lco CONFIG optional

[rouault]

I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled).

I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.

[rouault]

Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference? mainly), and a missing pointer initialization. Could you check that it's OK ?

I've a few observations :

• I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.

• You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.

• Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature? and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature?())

• For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at  https://svn.osgeo.org/gdal/trunk/autotest.

[rouault]

Didier, sorry for having mixed your first and last name my previous post.

[dgrichard]

Replying to rouault:

I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled). I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.

My mistake for mitab_coordsys.cpp, I am working on it and forgot to remove it from the whole patch. Sorry.

[dgrichard]

Replying to rouault:

Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference? mainly), and a missing pointer initialization. Could you check that it's OK ?

Seems to. As you noticed in the code, I had some question marks about Reference/Dereference?.

I've a few observations : * I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.

The definition is created at low level, I'll changed that.

* You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.

Yeap, I noticed that. I'll improve the process.

* Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature? and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature?())

Ok, it is quite cumbersome !

The great thing would have been to have in common tools : - a index (BTree or like); - a geometric index (Quad or like);

* For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at  https://svn.osgeo.org/gdal/trunk/autotest.

Ok, I'll give it a glance and manage to provide the autotest.

Thanks for all

[rouault]

Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.

[warmerdam]

Didier,

I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature?() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.

[dgrichard]

Replying to rouault:

Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.

I have to check for that. Geoconcept ID when set to -1 means the incrementation when importing into the GIS, otherwise use the ID. So, this should be not to complex to fix when writing. I am wondering what to do when reading (the export could have mixed -1 and IDs).

[dgrichard]

Replying to warmerdam:

Didier, I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature?() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.

Frank, the following seems working : --- geoconcept.c 2008-01-20 22:05:48.000000000 +0100 +++ geoconcept.c 2008-01-20 22:05:46.000000000 +0100 @@ -1864,10 +1864,7 @@

theSubType= NULL;

}

- if( GetGCMode_GCIO(H)==vReadAccess_GCIO ) - { - Rewind_GCIO(H,NULL); - } + Rewind_GCIO(H,NULL);

return Meta;

}/* _completeHeader_GCIO */

I'll send the patch when I am through with other points I need to fix/upgrade.

[warmerdam]

I have added a very minimal read test (r13557) of one of the attached samples. I have so far been unable to successfully create a geoconcept file with ogr2ogr so I haven't adding any write testing.

[warmerdam]

rewind patch applied as r13558. It appears to correct the problem.

[dgrichard]

Mapinfo for unit test (mid)

[dgrichard]

Mapinfo for unit test (mif)

[dgrichard]

geoconcept unit tests shell script

[dgrichard]

patch against trunk with various enhancements (bug fix for feature definition, round-tripping of SRS complete, spatial and attribute filters added, better support for update, FID support, etc ...)

[rouault]

Didier, I applied geoconcept-20080129.patch in r13624.

In r13625, I also moved some code in OGRSpatialReference2SysCoord_GCSRS, otherwise I was unable to assign EPSG:4326 (or other geographics SRS) as an output SRS for geoconcept. Does it look ok to you ?

I'm also wondering if there wouldn't be a prettier way to test the SRS in OGRSpatialReference2SysCoord_GCSRS. All those hard-coded parameters in the middle of code look scary to me. Couldn't they be put in a few arrays like it seems to have been done in ogr/ogrsf_frmts/mitab/mitab_spatialref.cpp ?

With SRS round-tripping, we should now be able to add write tests in the GDAL autotest.

[rouault]

Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "//$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.

[rouault]

A few memory leaks fixed in r13670.

[rouault]

Replace spaces by underscores in field names, when creating fields in r13672.

Add a test for writing Geoconcept file in r13673.

[dgrichard]

Correct reading of unknown SRS

[dgrichard]

Replying to rouault:

Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "//$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.

Patch for not handled SRS sent. I am currently working on re-organising the SRS comparizon in geoconcept_syscoord.c

[rouault]

geoconcept-20080202.patch applied in r13676

[dgrichard]

Upgrade of coordinate systems handler in Geoconcept driver

[rouault]

Didier,

I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test.

In http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr/ogr_geoconcept.py, ogr_gxt_2 reads the following file and translates it to Geoconcept, and read it back. The round-trip of the SRS is KO.

Input file :

//$DELIMITER "  "
//$QUOTED-TEXT "no"
//$CHARSET ANSI
//$UNIT Distance:m
//$FORMAT 2
//$SYSCOORD {Type: 101}
//$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class   Private#Subclass        Private#NbFields        Primary_ID      Secondary_ID   Third_ID Private#X       Private#Y
-1      points  points  3       PID1    SID1    TID1    0       1
-1      points  points  3       PID2    SID2            2       3

Output file :

//$DELIMITER "  "
//$QUOTED-TEXT "no"
//$CHARSET ANSI
//$UNIT Distance:m
//$FORMAT 2
//$SYSCOORD {Type: 113020768};{TimeZone: 113020824}
//$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class   Private#Subclass        Private#NbFields        Primary_ID      Secondary_ID   Third_ID Private#X       Private#Y
-1      points  points  3       PID1    SID1    TID1    0       1
-1      points  points  3       PID2    SID2            2       3

There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian?_GCSRS(theSysCoord, 0);

Could you look at this ?

[dgrichard]

Replying to rouault:

Didier, I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test. There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian?_GCSRS(theSysCoord, 0);

Added

Could you look at this ?

Allright, I'll have a look. Nevertheless, my unit tests were OK with ogrgc.sh; I had the same problem as you mentioned, but after a full compilation it was running ... I have also tested the SRS round-trip and EPSG:4326 (Type:101) was OK.

[rouault]

r13869 : "Apply geoconcept-20080223.patch (except for the makefile)"

[rouault]

r13870 : "A few changes for WGS 84 handling so that ogr_gxt_2 works"

[dgrichard]

Creation test expected result

[dgrichard]

Creation test expected result (2)

[dgrichard]

Update unit test expected result

[dgrichard]

Update unit test expected result

[dgrichard]

Update unit test expected result (2)

[dgrichard]

Pragma DELIMITER enhancement, Pragma VERSION addition. Bug fixes in error messages.

[dgrichard]

Frank, Even,

I made few changes and fixes to the Geoconcept driver and change test data accordingly. I wonder whether I may open an enhancement request against Geoconcept for transferring changes/fixes next time or I still use this bug folder ?

[rouault]

r13907 : apply geoconcept-20080103.patch

r13908 : Enhance geoconcept test with data file with TAB delimitor

[warmerdam]

Are we at the point where this ticket can be closed?

[rouault]

Yes, closing