Opened 13 years ago

Closed 12 years ago

#1704 closed defect (fixed)

ogr2ogr kml export of polygons can have incorrect vertex winding order

Reported by: mashbridge Owned by: Mateusz Łoskot
Priority: high Milestone: 1.5.0
Component: OGR_SF Version: 1.4.2
Severity: normal Keywords: kml
Cc: condit@…, Didge, warmerdam

Description

Exporting to KML via ogr2ogr, particularly where the source data is an ESRI Shapefile, often results in an incorrect polygon winding order.

3D implementations of KML use the vertex winding order of a polygon to calculate lighting. The "front" of a face is defined as a face with an anti-clockwise winding order (positive surface normal). I have attached two images showing the effect of correct and incorrect winding orders.

Attachments (5)

bad_winding_order.jpg (98.8 KB) - added by mashbridge 13 years ago.
screenshot of exported poly with incorrect (clockwise) winding order
good_winding_order.jpg (102.1 KB) - added by mashbridge 13 years ago.
screenshot of polygon with corrected (anti-clockwise) winding order
reversewindingorder.patch (3.3 KB) - added by mashbridge 13 years ago.
patch for top of trunk
test-before.kml (539 bytes) - added by Mateusz Łoskot 13 years ago.
Clockwise order of KML polygon coords generated with ogr2ogr before applying the patch
test-after.kml (549 bytes) - added by Mateusz Łoskot 13 years ago.
Correct KML polygon with anti-clockwise coords generated with patched OGR

Download all attachments as: .zip

Change History (15)

Changed 13 years ago by mashbridge

Attachment: bad_winding_order.jpg added

screenshot of exported poly with incorrect (clockwise) winding order

Changed 13 years ago by mashbridge

Attachment: good_winding_order.jpg added

screenshot of polygon with corrected (anti-clockwise) winding order

Changed 13 years ago by mashbridge

Attachment: reversewindingorder.patch added

patch for top of trunk

comment:1 Changed 13 years ago by mashbridge

I created a patch for the current TOT. I regret to say that this evening's experimentation is the totality of my familiarity with the GDAL codebase. Apologies for the inevitable naivety of my fix.

Briefly, I:

  • added a generic reverseWindingOrder() method to the OGRLinearRing class in ogr/ogr_geometry.h
  • which is implemented in ogr/ogrlinearring.cpp as an efficient array swap
  • and called in ogr/ogrsf_frmts/kml/ogr2kmlgeometry.cpp if the winding order is determined to be clockwise

No doubt there are many issues of which I'm unaware, but I hope this helps a little.

comment:2 Changed 13 years ago by warmerdam

Cc: Didge warmerdam added
Keywords: kml added
Owner: changed from warmerdam to Mateusz Łoskot

Jens / Matuesz,

Could you jointly review this patch for appropriateness?

comment:3 Changed 13 years ago by Mateusz Łoskot

Priority: normalhigh
Status: newassigned

comment:4 Changed 13 years ago by Mateusz Łoskot

I'm pasting e-mail from Jens:

Jens Oberender wrote:
> The patch looks fine, clear and efficient.
> I haven't testet it with data,
> but tried the algorithm with pen and paper.
> 
> Ciao,
> 	Jens

Jens, thanks for the review. I will test it.

Changed 13 years ago by Mateusz Łoskot

Attachment: test-before.kml added

Clockwise order of KML polygon coords generated with ogr2ogr before applying the patch

Changed 13 years ago by Mateusz Łoskot

Attachment: test-after.kml added

Correct KML polygon with anti-clockwise coords generated with patched OGR

comment:5 Changed 13 years ago by Mateusz Łoskot

Resolution: fixed
Status: assignedclosed

I've also tested the patch. It works correct, so it's been applied (r1185).

I'm closing this Ticket.

comment:6 Changed 13 years ago by mashbridge

Cool. Thanks guys.

comment:7 Changed 12 years ago by Mateusz Łoskot

One note of clarification. The fix was applied to the trunk as r11885, but not r1185.

comment:8 Changed 12 years ago by Mateusz Łoskot

The fix has been ported to branches/1.4 (r12517)

comment:9 Changed 12 years ago by Mateusz Łoskot

Resolution: fixed
Status: closedreopened

This patch has broken the C++ ABI so it should be removed from 1.4.x line. More details here: [http://lists.osgeo.org/pipermail/gdal-dev/2007-November/014975.html GDAL/OGR 1.4.3 ABI incompatability with 1.4.2}

comment:10 Changed 12 years ago by Mateusz Łoskot

Milestone: 1.4.31.5.0
Resolution: fixed
Status: reopenedclosed

The patch has been removed from branches/1.4 (r12921). It's still available in trunk and targets 1.5.0.

Note: See TracTickets for help on using tickets.