Ticket #1704 (closed defect: fixed)

Opened 1 year ago

Last modified 8 months ago

ogr2ogr kml export of polygons can have incorrect vertex winding order

Reported by: mashbridge Assigned to: mloskot
Priority: high Milestone: 1.5.0
Component: OGR_SF Version: 1.4.2
Severity: normal Keywords: kml
Cc: condit@sdsc.edu, 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

bad_winding_order.jpg (98.8 kB) - added by mashbridge on 07/21/07 02:57:05.
screenshot of exported poly with incorrect (clockwise) winding order
good_winding_order.jpg (102.1 kB) - added by mashbridge on 07/21/07 02:59:06.
screenshot of polygon with corrected (anti-clockwise) winding order
reversewindingorder.patch (3.3 kB) - added by mashbridge on 07/21/07 03:01:49.
patch for top of trunk
test-before.kml (0.5 kB) - added by mloskot on 08/16/07 09:27:13.
Clockwise order of KML polygon coords generated with ogr2ogr before applying the patch
test-after.kml (0.5 kB) - added by mloskot on 08/16/07 09:28:27.
Correct KML polygon with anti-clockwise coords generated with patched OGR

Change History

07/21/07 02:57:05 changed by mashbridge

  • attachment bad_winding_order.jpg added.

screenshot of exported poly with incorrect (clockwise) winding order

07/21/07 02:59:06 changed by mashbridge

  • attachment good_winding_order.jpg added.

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

07/21/07 03:01:49 changed by mashbridge

  • attachment reversewindingorder.patch added.

patch for top of trunk

07/21/07 03:08:59 changed 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.

07/23/07 10:40:00 changed by warmerdam

  • keywords set to kml.
  • owner changed from warmerdam to mloskot.
  • cc changed from condit@sdsc.edu to condit@sdsc.edu, Didge, warmerdam.

Jens / Matuesz,

Could you jointly review this patch for appropriateness?

08/10/07 10:15:09 changed by mloskot

  • priority changed from normal to high.
  • status changed from new to assigned.

08/11/07 12:29:04 changed by mloskot

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.

08/16/07 09:27:13 changed by mloskot

  • attachment test-before.kml added.

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

08/16/07 09:28:27 changed by mloskot

  • attachment test-after.kml added.

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

08/16/07 09:30:20 changed by mloskot

  • status changed from assigned to closed.
  • resolution set to fixed.

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

I'm closing this Ticket.

08/16/07 11:26:43 changed by mashbridge

Cool. Thanks guys.

10/23/07 10:57:49 changed by mloskot

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

10/23/07 11:11:01 changed by mloskot

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

11/21/07 14:22:18 changed by mloskot

  • status changed from closed to reopened.
  • resolution deleted.

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}

11/21/07 14:39:27 changed by mloskot

  • status changed from reopened to closed.
  • resolution set to fixed.
  • milestone changed from 1.4.3 to 1.5.0.

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