Port updates to BoundaryChainNoder #1279
Conversation
| // Overlapping inputs (unchanged output) | ||
| std::vector<std::string> wkt{ | ||
| "POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0))", | ||
| "POLYGON ((1 0, 0.9 1, 2 1, 2 0, 1 0))" | ||
| }; | ||
|
|
||
| auto g1 = GEOSWKTReader_read(m_reader, wkt[0].c_str()); | ||
| auto g2 = GEOSWKTReader_read(m_reader, wkt[1].c_str()); | ||
|
|
||
| GEOSGeometry* geoms[2] = { g1, g2 }; | ||
|
|
||
| auto input = GEOSGeom_createCollection(GEOS_GEOMETRYCOLLECTION, geoms, 2); | ||
| auto result = GEOSCoverageUnion(input); | ||
| ensure( result != nullptr ); | ||
| ensure( GEOSEquals(input, result) ); | ||
| auto input = GEOSWKTReader_read(m_reader, | ||
| "GEOMETRYCOLLECTION(POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0)), POLYGON ((1 0, 0.9 1, 2 1, 2 0, 1 0)))"); | ||
|
|
||
| // auto input = GEOSGeom_createCollection(GEOS_GEOMETRYCOLLECTION, geoms, 2); | ||
| // Temporary, wrap in a try/catch block until JTS upstream issue is fixed. | ||
| try { | ||
| auto result = GEOSCoverageUnion(input); | ||
| ensure( result != nullptr ); | ||
| ensure( GEOSEquals(input, result) ); | ||
| GEOSGeom_destroy(result); | ||
| } | ||
| catch(std::exception& e) { | ||
| (void)0; | ||
| } |
There was a problem hiding this comment.
@pramsey in shapely, we had the same test for overlapping inputs for coverage union, and so this also started to fail now.
From your comment here, it seems that you are still planning to fix this to get back the previous behaviour of returning the input, but reading the discussion in locationtech/jts#1134, that might not actually be the case?
The doc of GEOSCoverageUnion does mention "It may generate an error (return NULL) for inputs that do not satisfy this constraint, however this is not guaranteed.", so we can also add that to the shapely documentation that this can happen, and update our test to check for an error.
There was a problem hiding this comment.
It seems better to indicate that an error was found, rather than simply returning the input (which would not fulfill the contract of the operation of returning a valid coverage).
There was a problem hiding this comment.
Yes, an error is certainly fine (and agreed it seems better than silently doing nothing).
It is a little bit unfortunate that the error message is "TopologyException: side location conflict at 1 0. This can occur if the input geometry is invalid." which might people lead to think that the individual input geometries are invalid, not necessarily that the combination is an invalid coverage.
There was a problem hiding this comment.
Before GEOS 3.12 (I assume before OverlayNG?), this case actually also already raised an error, but at the time the error message something about "CoverageUnion cannot process incorrectly noded inputs"
There was a problem hiding this comment.
Hm, I wonder if this is something we can distinguish... the fact that we're getting a topology exception indicates to me it might be deeper in the stack, though I guess we could catch it in the CoverageUnion op and throw a different exception from there.
There was a problem hiding this comment.
I gave it a try in df6780c.
That looks good to me. I'll make this change in JTS as well.
3 participants
From locationtech/jts#1134