GML Curve concatenation results in invalid geometries due to point duplication

[warmerdam]

The OGR geometry for:

      <gml:Surface>
       <gml:patches>
        <gml:PolygonPatch interpolation="planar">
         <gml:exterior>
          <gml:Ring>
           <gml:curveMember>
            <gml:Curve>
             <gml:segments>
              <gml:LineStringSegment interpolation="linear">
               <gml:pos>0 -1</gml:pos>
               <gml:pos>0 1</gml:pos>
              </gml:LineStringSegment>
              <gml:Arc interpolation="circularArc3Points" numArc="1">
               <gml:pos>0 1</gml:pos>
               <gml:pos>1 0</gml:pos>
               <gml:pos>0 -1</gml:pos>
              </gml:Arc>
             </gml:segments>
            </gml:Curve>
           </gml:curveMember>
          </gml:Ring></gml:exterior>
         </gml:PolygonPatch>
        </gml:patches>
       </gml:Surface>

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.

[warmerdam]

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

[warmerdam]

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.

[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 ) 
{
     poLine->setPoint(0,x0,y0); 
     poLine->setPoint(poLine->getNumPoints()-1,x0,y0);
}

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

[Even Rouault]

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

[Even Rouault]

Milestone 1.8.2 deleted