Fix "Hole lies outside shell" for reversed islands - #53
Conversation
|
I have also tested this against the files referenced on #41, which now gives following output: Also tested as part of the complete OpenGeofiction coastline process. |
|
Thanks for looking into this and for the PR. I had to bring the CI up to date first, can you please rebase the PR on top of the current master? On the whole this looks pretty promising, but I am a bit concerned about last existing test with the changed outcome. In the old version it created three error lines marked "questionable" which is osmcoastline's way of throwing its hands into the air and saying: "Something looks fishy here, but I don't know what is happening, a human needs to look at this." The new code generates a correct land polygon, but it also puts the outermost ring as "invalid" into the errors table, which is wrong. So we have a regression here. Can you look into that? |
151ca86 to
3d577f8
Compare
|
Rebased on current master. You are right about the So instead of falling through to For The island is reported as Two notes on the implementation:
|
|
Ran the data from #41 (https://data.opengeofiction.net/extracts/osmcoastline-issue41/ogf-coastline-data.osm.pbf) through both current master (2c6dd17) and this branch, same command line as in the issue:
That Both runs report 1 warning and 0 errors, and the runtime is unchanged. |
| return false; | ||
| } | ||
|
|
||
| return ring->isPointInRing(&point, FALSE) != FALSE; |
There was a problem hiding this comment.
a) This is C++ not C code and
b) != FALSE??
organizePolygons() decides which rings are holes based on their direction only and uses just a bounding box test when assigning a hole to a polygon. A ring whose coastline was mapped the wrong way round can therefore end up as a hole of a polygon it is not inside of, or as a hole nested inside another hole. GEOS then reports "Hole lies outside shell" or "Holes are nested" and the whole polygon, possibly a whole continent, is invalid and ends up in the error_lines table. The existing direction fix runs after the polygons have been created and only looks at exterior rings, so it never caught these cases. Those rings are not holes at all but land mapped the wrong way round. They are now taken out of the polygon, turned around into land polygons of their own and reported in the error_lines table with the error "direction". While at it, invalid polygons that are not part of a multipolygon are now repaired in the same way as those that are, instead of being dropped. Fixes osmcode#41 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cd8414f to
79a14a7
Compare
|
Thanks for the PR. I am not 100% happy, because it seems kind of backwards to me to fix the output of |
|
Thanks Jochen, appreciate the merge. As you say it's working around the root issue, rather than fixing it at source - never the ideal situation. I would like to return to it in the future, once I clear my plate of a few other things on the go just now. |
2 participants
Fixes #41.
The problem
create_polygons()callsorganizePolygons()withMETHOD=ONLY_CCW, so ring direction alone decides shell vs. hole, and a hole is assigned to a shell using only a bounding box test. An island whose coastline was mapped the wrong way round becomes a hole candidate, and if it happens to lie inside the bounding box of a larger landmass (but outside the landmass itself) it is attached to it as a hole. GEOS then reportsand the whole polygon — possibly a whole continent — is invalid and ends up in the
error_linestable, or is dropped entirely.The existing "Fixing coastlines going the wrong way" step runs after the polygons have been created and only looks at exterior rings, so it never caught this case and reported "Turned 0 polygons around".
The fix
When a polygon is invalid, each interior ring is checked against the exterior ring. The check tests the first point of the interior ring with
OGRLinearRing::isPointInRing()rather than a GEOS operation, because it has to work on invalid geometries too. Rings that are outside their shell are taken out of the polygon, turned around into land polygons of their own, and reported in theerror_linestable with the errordirection(the same error the existing direction fix uses). They are counted innum_rings_turned_around.While at it, invalid polygons that are not part of a multipolygon are now repaired in the same way as those that are (report an
invaliderror line and tryBuffer(0)), instead of just being dropped.Tests
invalid-direction-island-in-bbox— reversed island inside the bounding box of an L-shaped landmass (single polygon code path).invalid-direction-island-in-bbox-of-many— the same with a second, correctly mapped island, soorganizePolygons()returns a multipolygon (the code path hit by the case in Reversed coastline results in "Hole lies outside shell" output #41).valid-inland-sea-with-islandneeded updating: the island in the inland sea is mapped hole-style and so ends up nested inside the hole formed by the sea ("Holes are nested"). That polygon is now repaired instead of discarded, so the land polygon survives and the result is a warning rather than an error. The island itself is still dropped and reported asquestionable; that nested-hole limitation is unchanged.