Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#4451 closed defect (fixed)

GML Curve concatenation results in invalid geometries due to point duplication

Reported by: warmerdam Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SF Version: 1.8.0
Severity: normal Keywords: gml nas
Cc: astrid_emde


The OGR geometry for:

        <gml:PolygonPatch interpolation="planar">
              <gml:LineStringSegment interpolation="linear">
               <gml:pos>0 -1</gml:pos>
               <gml:pos>0 1</gml:pos>
              <gml:Arc interpolation="circularArc3Points" numArc="1">
               <gml:pos>0 1</gml:pos>
               <gml:pos>1 0</gml:pos>
               <gml:pos>0 -1</gml:pos>

is like:

  POLYGON ((0 -1,0 1,0 1,0.069756473744125 0.997564050259824...

Note that point "0 1" is duplicated from the end of the line string to the beginning of the curve. This causes the geometry to be invalid from a simple features point of view (observable presumably with an isvalid test in postgis?). Presumably the concatenation code should be deduplicating things between chunks of the geometry.

Change History (6)

comment:1 Changed 6 years ago by warmerdam

Resolution: fixed
Status: newclosed

Problem corrected with a test in trunk (r23767). Backported just the fix to 1.9 (r23768) and 1.8 (r23769).

comment:2 Changed 6 years ago by warmerdam

Resolution: fixed
Status: closedreopened

Astrid reports that the geometries this relates to were still coming up invalid. I added a test to check the geometry validity in the autotest and confirmed the problem. It seems this is due to the end points being approximated instead of carried exactly from the source gml text.

Looking at the code it seems no attempt is made to clamp the end points of the arc when it is being approximated in gml2ogrgeometry.cpp.

I have added this clamping specifically for non-circles. I'm less sure how circles are supposed to work. I also wonder why this isn't using the generic arc approximation code which already did stuff like that. I see it was written by Even, so perhaps he can comment and consider simplifying the code and/or adding end clamping for the circle case.

In the meantime my minimal fixes (and a test) appear in trunk (r23856).

I'm going to wait briefly for Even's review but I'd like to see this or a related fix back proted to 1.9 and 1.8.

comment:3 Changed 6 years ago by Even Rouault

Frank, initially I considered using approximateArcAngles() but its parameters (center, angles, radius) don't match what we get from GML (control points). So the hard part of the GML approximate code is to compute them. Now I see that after that computation, we should have all parameters needed to actually use approximateArcAngles() and replace each of the for loops in the GML approximation code. However approximateArcAngles() cannot help for using the exact coordinates provided, so I don't see a better alternative to your fix.

We don't try to also clamp the intermediate control point (x1,y1), but that's probably less critical.

And for circles, I guess it should be the following to ensure that the last point is identical to the first point, but I am not sure, and gml:Circle are a very rare beast.

if( poLine->getNumPoints() >= 2 ) 

comment:4 Changed 6 years ago by Even Rouault

The following commit avoid the post-clamping :

r23863 /trunk/gdal/ogr/gml2ogrgeometry.cpp: gml2ogrgeometry : make sure to use provided control points in linear approximation of gml:Arc (#4451)

comment:5 Changed 6 years ago by Even Rouault

Milestone: 1.8.2
Resolution: fixed
Status: reopenedclosed
Version: unspecified1.8.0

r23863 backported to 1.9 (r23891) and 1.8 (r23892) branches

comment:6 Changed 5 years ago by Even Rouault

Milestone: 1.8.2

Milestone 1.8.2 deleted

Note: See TracTickets for help on using tickets.