Opened 11 years ago

Closed 11 years ago

#4109 closed enhancement (fixed)

Add OGRWriteToShapeBin support for writing Shape binary arrays

Reported by: pramsey Owned by: Even Rouault
Priority: normal Milestone: 1.9.0
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: shape pgeo fgdb
Cc: warmerdam


Both the PGeo and FGDB drivers need to write shape objects, divorced from the rest of the file structure. There is already OGRCreateFromShapeBin, this patch is the reverse process, a OGRWriteToShapeBin function.

Attachments (2)

shapewriter-20110603.patch (54.4 KB) - added by pramsey 11 years ago.
patch generated in ./gdal/ogr
shapewriter-20110607.patch (27.8 KB) - added by pramsey 11 years ago.
rewrite for frank and evan

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by pramsey

Attachment: shapewriter-20110603.patch added

patch generated in ./gdal/ogr

comment:1 Changed 11 years ago by pramsey

Owner: changed from warmerdam to Even Rouault

comment:2 Changed 11 years ago by warmerdam

Cc: warmerdam added
Component: defaultOGR_SF

I think I would prefer shapefile export to be implemented in ogrpgeogeometry.h/cpp instead of distributed through the ogr_geometry.h and the various low level geometry classes. This would be more in keeping with the OGRCreateFromShapeBin function.

comment:3 Changed 11 years ago by pramsey

I started writing it that way, but the code seemed really fugly, so I backed off and followed the exportToWkb pattern instead, which seemed a lot nicer. Preference or requirement?

comment:4 Changed 11 years ago by warmerdam

Let just say, if I'm going to be the one applying the patch I'd end up rewriting it my way. If Even wants to apply it as is, he is within his rights.

I don't see such a public class as OGRGeometry as an appropriate place for a helper function to simplify writing three drivers.

comment:5 Changed 11 years ago by Even Rouault

I had a bit the same reaction as Frank. Plus the fact that the name exportToShp() is a bit confusing because it might imply that the blob is compatible with shapefile blobs. But when you look in details, a PolylineZ for PGEO/FGDB is exported with type 10, which is not recognized by the shapefile driver.

shapefil.h (consistant with ESRI_shapefile_technical_description.pdf page 4) :

#define SHPT_ARC	3
#define SHPT_ARCZ	13
#define SHPT_ARCM	23

ogrpgeogemetry.h (consistant with extended_shapefile_format.pdf page 3) :

#define SHPT_ARC           3
#define SHPT_ARCM         23
#define SHPT_ARCZM        13
#define SHPT_ARCZ         10

Note that from wednesday, I will be away for the next 2 weeks, so Frank might as well apply an updated patch.

comment:6 Changed 11 years ago by pramsey

Working on updated patch now.

comment:7 Changed 11 years ago by Even Rouault

A few remarks :

  • it would be good if you could stick to the 4 spaces indentation convention used elsewhere in the GDAL/OGR base code.
  • I have noticed a few cases where the ShpSize?() method uses the number of sub-geometries, but not the number of *non-empty* sub-geometries (but the serialization code does skip them appropriately). In OGRMultiLineString and OGRMultiPoint as far as I can see.
  • You should also be careful when calling get_IsClosed(). I believe the current implementation will segfault when nPointCount < 2 (see OGRLineString::EndPoint?() that calls getPoint( nPointCount-1, poPoint ) and the asserts in getPoint()). Might be worth fixing it or checking the number of point before calling it as it is done elsewhere in the basecode

Changed 11 years ago by pramsey

Attachment: shapewriter-20110607.patch added

rewrite for frank and evan

comment:8 Changed 11 years ago by Even Rouault

Keywords: pgeo fgdb added
Milestone: 1.9.0
Resolution: fixed
Status: newclosed

r22512 /trunk/gdal/ogr/ (ogrpgeogeometry.cpp ogrpgeogeometry.h): PGeo: add support for decoding multipoint/multipointz geometries; add OGRWriteToShapeBin() to serialize an OGR geometry object into a PGeo/FGDB geometry blob (patch by Paul Ramsey, #4109)

Note: See TracTickets for help on using tickets.