Opened 13 years ago
Closed 12 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)
Change History (27)
comment:1 by , 13 years ago
Cc: | added |
---|---|
Component: | default → OGR_SF |
Keywords: | gml topology added |
Version: | svn-trunk → 1.8.0 |
comment:2 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 13 years ago
Cc: | added; removed |
---|
by , 13 years ago
comment:4 by , 13 years ago
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.
comment:5 by , 13 years ago
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 by , 13 years ago
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).
by , 13 years ago
Attachment: | PeriCase.xml added |
---|
a case allowing a valid and an invalid interpretation
comment:7 by , 13 years ago
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 by , 13 years ago
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.
follow-up: 10 comment:9 by , 13 years ago
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 by , 13 years ago
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.
follow-up: 12 comment:11 by , 13 years ago
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 by , 13 years ago
strk,
Can you tell me which part of the GEOS library can be used for Geometry cleaning?
comment:13 by , 13 years ago
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 by , 12 years ago
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 by , 12 years ago
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
by , 12 years ago
Attachment: | patch-face-hole-negative.zip added |
---|
by , 12 years ago
Attachment: | sample_gml_face_hole_negative_no.zip added |
---|
comment:16 by , 12 years ago
Several observations :
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 by , 12 years ago
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 by , 12 years ago
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
by , 12 years ago
Attachment: | patch-face-hole-negative-2.diff added |
---|
comment:19 by , 12 years ago
I've attached the GML driver HTML doc
please see: patch-face-hole-negative-doc.diff
bye Sandro
by , 12 years ago
Attachment: | patch-face-hole-negative-doc.diff added |
---|
comment:20 by , 12 years ago
Milestone: | → 1.9.0 |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Fixed in r23638
face with hole