ogr2ogr/shapefile fails to generate valid polygons with hole touching boundary

OGR shapefile patched for touching polygons

We (think we) fixed this bug for touching polygons : see attached file shpopen.c, from line 1925 to line 1980.

All polygon pairs were tested for inclusion of its first polygon starting vertex in the other polygon, with a non strict inclusion. So in your case, the outer ring starting vertex was incorrectly considered "inside" the inner ring.

=> the inclusion is now strict in shpopen.c

note : that issue can also appear with outer touching polygons.

Mateusz,

Can you look into this for 1.4.2? Once you are happy with the change, I can upstream it to the shapelib repository.

added myself in CC since GRASS 6 uses shpopen.c, too (lib/external/shapelib/).

Markus

Lars,

BTW, Your polygon is invalid in terms of Shapefile Specification because the inner ring is clockwise instead of counterclockwise.

Certainly, it doesn't change the problem because translation of valid version:

POLYGON ((0 0,0 3,3 3,3 0,0 0), (0 0,2 1,1 2,0 0))

produces incorrect output:

MULTIPOLYGON (((0 0,0 3,3 3,3 0,0 0)),((0 0,1 2,2 1,0 0)))

Fixing it.

The original attachement but in form of a patch

Unfortunately, the patch attached to this Ticket doesn't solve the problem. I tested it taking following steps:

• Imported following sample polygon to PostGIS database

  POLYGON ((0 0, 0 3, 3 3, 3 0, 0 0), (0 0, 1 2, 2 1, 0 0))
  

• Dumped to Shapefile with pgsql2shp and here is the result:

D:\dev\gdal\bugs\976\data>ogrinfo 1415.shp 1415
Layer name: 1415
Geometry: Polygon
Feature Count: 1
Extent: (0.000000, 0.000000) - (3.000000, 3.000000)
Layer SRS WKT:
(unknown)
ID: Real (11.0)
INFO: String (120.0)
OGRFeature(1415):0
  ID (Real) =           4
  INFO (String) = Polygon with 1 clockwise hole touching the outer ring
  POLYGON ((0 0,0 3,3 3,3 0,0 0),(0 0,2 1,1 2,0 0))

As you see, the pgsql2shp rewinded the inner ring to make it counter-clockwise, according to the Shapefile Specification.

• Next, I converted my test file 1415.shp to another Shapefile with ogr2ogr patched using attached version of shpopen.c

D:\dev\gdal\bugs\976\data>ogr2ogr out.shp 1415.shp

• The result is still incorrect:

D:\dev\gdal\bugs\976\data>ogrinfo out.shp out

Layer name: out
Geometry: Polygon
Feature Count: 1
Extent: (0.000000, 0.000000) - (3.000000, 3.000000)
Layer SRS WKT:
(unknown)
ID: Real (11.0)
INFO: String (120.0)
OGRFeature(out):0
  ID (Real) =           4
  INFO (String) = Polygon with 1 clockwise hole touching the outer ring
  MULTIPOLYGON (((0 0,0 3,3 3,3 0,0 0)),((0 0,1 2,2 1,0 0)))

I don't know if it is an issue, but the code that determines if something should be treated as a POLYGON or MULTIPOLYGON as it is read from shapefile to OGR is in shape2ogr.cpp (see RingInRing?(), etc). SHPRewind() is only used while writing.

I believe there could be similar issues in the shape2ogr.cpp code (written by another) and that there might be some sort of consolodation with SHPRewind() that could be accomplished.

The patch applied (r11689) for Ticket #976 does also fix this bug.

I'm closing this bug, but if the problem still leaks, please reopen it. I'd be also thankful for confirmation if the bug has been fixed (or not).

Test polygon from the ticket report and result of translation with fixed OGR.

I confirm that patch #1415 is not working either on my version, for the (correct) test shapefile you posted in the 1415_fix_test.zip.

The issue comes from the starting points of rings : the inner (triangle) ring starting point lies on the edge of the outer (square) ring, so the patch considers that it is outside of the outer ring, since it is now testing for strict inclusion => so none of the rings is reported as beeing inside of the other, which gives two outer rings (incorrect).

The previous behaviour, with normal inclusion, was to report that the square outer ring starting point (0,0) was inside of the triangle, and that conversely the trinagle starting point (0,0) also was inside of the square ring => that lead to double inversion, and generated two outer rings as well.

For info, I modified your test file 1415.shp so that the starting point of the triangle now lies completely inside of the square. => path 1415 now does its job (see attched gml file 1415g.gml I used to modify the starting point, and its conversion to shape 1415g.shp)

Modified 1415_test_fix.zip, so that the triangle starting point is completely inside of the square

Replying to fredc:

I confirm that patch #1415 is not working either on my version, for the (correct) test shapefile you posted in the 1415_fix_test.zip. ...

FYI, I tested your Shapefile and OGR with fix I applied (r11689) translates it correctly:

• Original file

D:\dev\gdal\bugs\976\data>ogrinfo 1415g.shp 1415g

Layer name: 1415g
Geometry: Polygon
Feature Count: 1
Extent: (0.000000, 0.000000) - (3.000000, 3.000000)
Layer SRS WKT:
(unknown)
ID: Real (11.0)
INFO: String (80.0)
OGRFeature(1415g):0
  ID (Real) =           4
  INFO (String) = Polygon with 1 clockwise hole touching the outer ring
  POLYGON ((0 0,0 3,3 3,3 0,0 0),(2 1,1 2,0 0,2 1))

• Translation with ogr2ogr

• Translation output looks as follows:

D:\dev\gdal\bugs\976\data>ogrinfo out.shp out

Layer name: out
Geometry: Polygon
Feature Count: 1
Extent: (0.000000, 0.000000) - (3.000000, 3.000000)
Layer SRS WKT:
(unknown)
ID: Real (11.0)
INFO: String (80.0)
OGRFeature(out):0
  ID (Real) =           4
  INFO (String) = Polygon with 1 clockwise hole touching the outer ring
  POLYGON ((0 0,0 3,3 3,3 0,0 0),(2 1,1 2,0 0,2 1))

Frederic, I assume the bug has been confirmed as fixed.

Patch for Shapelib upstream with changeset r10975

Patch for Shapelib upstream with changeset r11008