Opened 6 years ago

Closed 6 years ago

#3937 closed defect (fixed)

GML3: Face interpretation fixes

Reported by: strk Owned by: chaitanya
Priority: normal Milestone: 1.9.0
Component: OGR_SF Version: 1.8.0
Severity: normal Keywords: gml topology
Cc: Even Rouault

Description

Parsing of gml:TopoSurface and gml:Face are likely bogus. See full discussion here: http://lists.osgeo.org/pipermail/gdal-dev/2011-January/027588.html

This ticket to work on fixing it. I'll work ASAP (monday?) on producing example TopoSurface? tags and expected resulting WKT.

Attachments (7)

P5.xml (1.4 KB) - added by strk 6 years ago.
face with hole
P1.xml (4.0 KB) - added by strk 6 years ago.
polygon defined by touching faces
PeriCase.xml (1.2 KB) - added by strk 6 years ago.
a case allowing a valid and an invalid interpretation
patch-face-hole-negative.zip (4.6 KB) - added by esseffe 6 years ago.
sample_gml_face_hole_negative_no.zip (2.5 KB) - added by esseffe 6 years ago.
patch-face-hole-negative-2.diff (22.5 KB) - added by esseffe 6 years ago.
patch-face-hole-negative-doc.diff (2.3 KB) - added by esseffe 6 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by Even Rouault

Cc: chaitanya added
Component: defaultOGR_SF
Keywords: gml topology added
Version: svn-trunk1.8.0

comment:2 Changed 6 years ago by chaitanya

Owner: changed from warmerdam to chaitanya
Status: newassigned

comment:3 Changed 6 years ago by chaitanya

Cc: Even Rouault added; chaitanya removed

Changed 6 years ago by strk

Attachment: P5.xml added

face with hole

comment:4 Changed 6 years ago by strk

P5.xml is TopoGeometry? 'P5' of postgis' test data for topologies. Is a polygon composed by a single face which in turn has an hole. Each ring is defined by a single edge. The XML file gave orientation "-" to edges having the composing face to the right rather than to the left (as per GML specs). The XML file contain expected WKT outcome.

Changed 6 years ago by strk

Attachment: P1.xml added

polygon defined by touching faces

comment:5 Changed 6 years ago by strk

P1 is a TopoGeometry? (TopoSurface?) composed by two adiacent faces. The WKT is shorter than the P5 one (in that it is a single ring). The XML contains an edge that will be dissolved (E9) as it concur to the definition of both faces.

Again, the "left-face" rule has been used for orienting edges.

This version of the XML is also using xrefs.

comment:6 Changed 6 years ago by strk

Note that the given XML files happen to have edges of each ring listed sequencially, but that isn't a requirement. GDAL should be able to reconstruct the area by guessing rings composition.

PostGIS uses the GEOS Polygonize function, feeding it all the rings, then computes the incremental symmetric difference of exterior rings of the so-obtained polygons. Such an algorithm is available as "ST_BuildArea" from PostGIS, but isn't in GEOS itself (might be a good idea to port it over).

Changed 6 years ago by strk

Attachment: PeriCase.xml added

a case allowing a valid and an invalid interpretation

comment:7 Changed 6 years ago by strk

This new case was reported by Andrea Peri as an example of why a sequence on the rings might be really needed. The case allows two different interpretations (single ring self-touching on a point, two rings touching on a point). In fact the single ring interpretation would be invalid as per SFS, so only the two-rings version is correct.

The GEOS Polygonize interprets such edge set correctly.

comment:8 Changed 6 years ago by chaitanya

I am going to deduce the sequence of directedEdge elements in a face from the IDs of directedNode elements in them. This will remove the potential errors caused by more than two edges with same endpoints.

comment:9 Changed 6 years ago by strk

Uhm.. sounds very wild. Node id numbering has no correlation with edge sequences, if not for some implementation detail of the generator.

Is using GEOS C-API an option ?

comment:10 in reply to:  9 Changed 6 years ago by chaitanya

Replying to strk:

Uhm.. sounds very wild. Node id numbering has no correlation with edge sequences, if not for some implementation detail of the generator.

Is using GEOS C-API an option ?

I meant to search for the adjoining edge using the node IDs.

comment:11 Changed 6 years ago by strk

ok, but if you look at PeriCase?.xml you'll see that it might not be enough. In that case you have a single node, and two edges. Both edges start and end at that single node, so wherever you start you'll reach that node and dunno how you'd decide which edge to pick next.

comment:12 in reply to:  11 Changed 6 years ago by chaitanya

strk,

Can you tell me which part of the GEOS library can be used for Geometry cleaning?

comment:13 Changed 6 years ago by strk

See GEOSPolygonize in geos_c.h Given a set of edges (like the ones you find in the gml:Face definition) it'll return a set of Polygon buildable with those. In case of holes these would be the exterior polygon with holes _and_ polygons "plugging" each hole. In that case you'll need to iteratively invoke symmetric difference (GEOSSymDifference) on outer rings, see http://postgis.refractions.net/pipermail/postgis-devel/2005-December/001805.html

comment:14 Changed 6 years ago by strk

So, it's been suggested by Alessandro Furieri to use this switch:

--config GML_FACE_HOLE_NEGATIVE YES

to request use of old interpretation.

The old interpretation is still used by some legacy GML used by local authorities in Italy so it is worth keeping as an option.

Ideally the testsuite of GDAL would contain regression testing for both old and new interpretation.

comment:15 Changed 6 years ago by esseffe

The attached patch implements GML_FACE_HOLE_NEGATIVE:

a) few changes have been applied here and there in

-/ogr/ogrsf_frmts/gml sources so to enable the recognition of this new option

b) the maincore implementation mainly affects

-/ogr/ogrgeometry.cpp and -/ogr/gml2ogrgeometry.cpp

c) -/ogr/ogrgeometry.cpp

the new function OGRGeometry::Polygonize() [widely based on GEOSPolygonize()] will reconstruct a Polygon Geometry starting form a sparse collection of Linestrings; this including any possible Interior Ring (hole)

d) -/ogr/gml2ogrgeometry.cpp

the function GML2OGRGeometry_XMLNode() has been modified so to implement both interpretations:

  • GML_FACE_HOLE_NEGATIVE=YES (the old one)
  • GML_FACE_HOLE_NEGATIVE=NO (default, the new one)

a small (but really hot & spicy) test sample is attached:

  • sample_gml_face_hole_negative_no.zip

please note: using GML_SKIP_RESOLVE_ELEMS NONE this patch immediately works, and can successfully parse the test sample.

but using GML_SKIP_RESOLVE_ELEMS HUGE the test sample will fail: the patch proposed in ticket #4407 is absolutely required to succesfully resolve the attached test sample.

best regards, Sandro Furieri

Changed 6 years ago by esseffe

Changed 6 years ago by esseffe

comment:16 Changed 6 years ago by Even Rouault

Several observations :

OGRGeometry::Polygonize() shoud test before casting to (OGRGeometryCollection *) that the wkbFlatten(getGeometryType()) == wkbGeometryCollection
wkbFlatten(getGeometryType()) == wkbMultiLineString
  • the declaration of OGRPolygon *GML2FaceExtRing( OGRGeometry *poGeom ); in ogr_p.h isn't appropriate at that place, because it is only internal in gml2ogrgeometry.cpp.
  • in gml2ogrgeometry.cpp, there's obviously something wrong in the comment of "the FaceHoleNegative? = TRUE rules". A duplicated line in the middle of the comment, and likely a missing line too
  • I'm afraid I don't understand why we now have 2 interpretations of Face orientation. Which one is the good one ? Should we really have both ? If the old one is bogus, we should completely remove it. No need to add options to keep buggy behaviour. If both interpretations are possible, is the one that is now the default the one that matches the recommandations of the standard ? Sorry for the naive questions...
  • If it is confirmed to be needed, the GML_FACE_HOLE_NEGATIVE configuration option should be documented in the GML driver page.
  • I'd suggest :
+                while( psFaceChild != NULL &&
+                       !EQUAL(BareGMLElement(psFaceChild->pszValue),"Face") )
+                        psFaceChild = psFaceChild->psNext;

to be replaced by

+                while( psFaceChild != NULL &&
+                       !(psFaceChild->eType == CXT_Element &&
+                         EQUAL(BareGMLElement(psFaceChild->pszValue),"Face")) )
+                        psFaceChild = psFaceChild->psNext;

Ah, and please attach directly patches to ticket, so they can be viewed directly. zip files are generally not needed unless many files must be attached.

comment:17 Changed 6 years ago by strk

Evan: about the "double implementation": the old and bogus one is legacy for Tuscany Region so there is content out there with such format. We want to be able to still read it, when known to be of that kind. The new one matches standard recommendations as far as we can tell.

comment:18 Changed 6 years ago by esseffe

Hi Even,

I've implemented all your suggestions (so I hope ...)

  • GML2FaceExtRing() is now a static function
  • I've fixed the comment for FaceHoleNegative?=TRUE
  • I've corrected the "while( psFaceChild != NULL" code accordingly to your suggestion.

Please note well: I've simply copied this code snippet from the "old" implementation. You'll find the same identical issue at the very beginning of the FaceHoleNegative?=TRUE code ;-)

please see: patch-face-hole-negative-2.diff

I'll send you ASAP a further patch intended to update the GML driver HTML doc

bye Sandro

Changed 6 years ago by esseffe

comment:19 Changed 6 years ago by esseffe

I've attached the GML driver HTML doc

please see: patch-face-hole-negative-doc.diff

bye Sandro

Changed 6 years ago by esseffe

comment:20 Changed 6 years ago by Even Rouault

Milestone: 1.9.0
Resolution: fixed
Status: assignedclosed

Fixed in r23638

Note: See TracTickets for help on using tickets.