Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#1415 closed defect (fixed)

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

Reported by: llo@… Owned by: Mateusz Łoskot
Priority: normal Milestone: 1.4.2
Component: OGR_SF Version: 1.3.2
Severity: normal Keywords: shapefile
Cc: llo@…, warmerdam, neteler, frederic.claudel@…

Description (last modified by warmerdam)

Using ogr2ogr to generate a shapefile will produce invalid polygons with hole if the hole touches the outer-boundary in one point. It produces a simple polygon without any hole, and ONLY a self-intersecting boundary.

This ONLY applies to shapefiles, no problems found with postgis or tab-file writer.

Example polygon in WKT that cannot be converted to a valid shapefile: POLYGON ((0 0, 0 3, 3 3, 3 0, 0 0), (0 0, 1 2, 2 1, 0 0))

/Regards Lars

Attachments (6)

shpopen.c (73.2 KB) - added by fredc 10 years ago.
OGR shapefile patched for touching polygons
shpopen.c.patch (1.9 KB) - added by Mateusz Łoskot 10 years ago.
The original attachement but in form of a patch
1415_fix_test.zip (1.2 KB) - added by Mateusz Łoskot 10 years ago.
Test polygon from the ticket report and result of translation with fixed OGR.
1415_startPoint.zip (2.0 KB) - added by fredc 10 years ago.
Modified 1415_test_fix.zip, so that the triangle starting point is completely inside of the square
gdal-r10975-to-shapelib.patch (1.0 KB) - added by Mateusz Łoskot 9 years ago.
Patch for Shapelib upstream with changeset r10975
gdal-r11008-to-shapelib.patch (1.0 KB) - added by Mateusz Łoskot 9 years ago.
Patch for Shapelib upstream with changeset r11008

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by fredc

Attachment: shpopen.c added

OGR shapefile patched for touching polygons

comment:1 Changed 10 years ago by fredc

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.

comment:2 Changed 10 years ago by warmerdam

Cc: warmerdam added
Description: modified (diff)
Keywords: shapefile added
Milestone: 1.4.2
Owner: changed from warmerdam to Mateusz Łoskot
Priority: highestnormal
Severity: criticalnormal

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.

comment:3 Changed 10 years ago by neteler

Cc: neteler added

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

Markus

comment:4 Changed 10 years ago by fredc

Cc: frederic.claudel@… added

comment:5 Changed 10 years ago by Mateusz Łoskot

Status: newassigned

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.

Changed 10 years ago by Mateusz Łoskot

Attachment: shpopen.c.patch added

The original attachement but in form of a patch

comment:6 Changed 10 years ago by Mateusz Łoskot

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)))

comment:7 Changed 10 years ago by warmerdam

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.

comment:8 Changed 10 years ago by Mateusz Łoskot

Resolution: fixed
Status: assignedclosed

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).

Changed 10 years ago by Mateusz Łoskot

Attachment: 1415_fix_test.zip added

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

comment:9 Changed 10 years ago by 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.

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)

Changed 10 years ago by fredc

Attachment: 1415_startPoint.zip added

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

comment:10 in reply to:  9 Changed 10 years ago by Mateusz Łoskot

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.

Changed 9 years ago by Mateusz Łoskot

Patch for Shapelib upstream with changeset r10975

Changed 9 years ago by Mateusz Łoskot

Patch for Shapelib upstream with changeset r11008

comment:11 Changed 8 years ago by zanollim

I think that there is a correlated problem when importing shapefile in postgis with ogr2ogr. I try to explain my problem.

I have a shapefile that comes from an arcinfo coverage (more or less 5000 polygons). In this shapefile I have some polygons with this 'problem':

these polygons have an outer ring and an inner ring (hole) that touches (tangent) the outer ring in one point. This point is the starting point of the two rings.

with ogr2ogr these POLYGON are interpreted wrongly as MULTIPOLYGON

The solution is to change 'manually' the starting point of the inner ring with another point of the same ring completely inside of the outer ring, but I think this is not the best (final) solution of the problem.

michele

Note: See TracTickets for help on using tickets.