Opened 17 years ago

Closed 16 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 17 years ago.
screenshot of exported poly with incorrect (clockwise) winding order
good_winding_order.jpg (102.1 KB ) - added by mashbridge 17 years ago.
screenshot of polygon with corrected (anti-clockwise) winding order
reversewindingorder.patch (3.3 KB ) - added by mashbridge 17 years ago.
patch for top of trunk
test-before.kml (539 bytes ) - added by Mateusz Łoskot 17 years ago.
Clockwise order of KML polygon coords generated with ogr2ogr before applying the patch
test-after.kml (549 bytes ) - added by Mateusz Łoskot 17 years ago.
Correct KML polygon with anti-clockwise coords generated with patched OGR

Download all attachments as: .zip

Change History (15)

by mashbridge, 17 years ago

Attachment: bad_winding_order.jpg added

screenshot of exported poly with incorrect (clockwise) winding order

by mashbridge, 17 years ago

Attachment: good_winding_order.jpg added

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

by mashbridge, 17 years ago

Attachment: reversewindingorder.patch added

patch for top of trunk

comment:1 by mashbridge, 17 years ago

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 by warmerdam, 17 years ago

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 by Mateusz Łoskot, 17 years ago

Priority: normalhigh
Status: newassigned

comment:4 by Mateusz Łoskot, 17 years ago

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.

by Mateusz Łoskot, 17 years ago

Attachment: test-before.kml added

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

by Mateusz Łoskot, 17 years ago

Attachment: test-after.kml added

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

comment:5 by Mateusz Łoskot, 17 years ago

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 by mashbridge, 17 years ago

Cool. Thanks guys.

comment:7 by Mateusz Łoskot, 16 years ago

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

comment:8 by Mateusz Łoskot, 16 years ago

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

comment:9 by Mateusz Łoskot, 16 years ago

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 by Mateusz Łoskot, 16 years ago

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.