Add PolygonizerExcludeHoles and MakeValid operations (fixes #952) #151
Conversation
| @@ -0,0 +1,76 @@ | |||
| /********************************************************************** | |||
There was a problem hiding this comment.
I think @dr-jts made some improvements in the JTS polygonizer (excluding holes from output) that may make this redundant. I see no problem including it now and replacing the implementation later, though.
There was a problem hiding this comment.
No, Polygonizer has not changed recently. I might have done some experimental work on a BuildArea equivalent though. Will have to look for the code. So probably not relevant to this PR.
There was a problem hiding this comment.
@dr-jts I was thinking of this: dbaston/jts-svn-history@31e500b#diff-9c4ebe19717a7d64b755b588909103a7
There was a problem hiding this comment.
@dbaston ah right, had forgotten about that. And I see a bit of cleanup needed there too...
As you say, can always port this JTS code later and replace internals of BuildArea.
|
|
||
| static std::unique_ptr<geom::MultiPolygon> collectFacesWithEvenAncestors( | ||
| std::vector<std::unique_ptr<Face>>& faces) { | ||
| std::vector<geom::Geometry*>* geoms = new std::vector<geom::Geometry*>(); |
There was a problem hiding this comment.
Why not use a std::unique_ptr<std::vector<geom::Geometry*>> here (and other similar locations). I'm guessing it's because the geometries would still be leaked on error, and GeometryFactory doesn't have a GeometryFactory::createMultiPolygon(std::vector<std::unique_ptr<Geometry>>) ?
There was a problem hiding this comment.
Yes exactly. That would cause memory leaks. I considered adding createMultiPolygon(std::vector<std::unique_ptr>) but didn't want to make this pull request touch too many things
src/operation/valid/MakeValid.cpp
Outdated
| * Fully node given linework | ||
| */ | ||
| static std::unique_ptr<geom::Geometry> | ||
| GEOS_nodeLines(const geom::Geometry* geom) |
There was a problem hiding this comment.
How about something like nodeLineWithFirstCoordinate ?
There was a problem hiding this comment.
This should be just GEOSNode call. When I tried replacing it with GEOSNode in PostGIS it crashed painfully. Inside GEOS I expect it to be replaced with GEOSNode and GEOSNode fixed to not crash.
There was a problem hiding this comment.
GEOSNode is in the C API; it's not callable here.
There was a problem hiding this comment.
its C++ internals should be called then? not a new implementation. It's just a side effect that Union does the noding.
There was a problem hiding this comment.
Sure, there may be an other way to implement it. But I see no reason that Even should fix unticketed bugs as part of this PR.
There was a problem hiding this comment.
Renamed as nodeLineWithFirstCoordinate() as suggested.
GEOSNode() just calls geos::noding::GeometryNoder::node(). I tried this, but unfortunatelly this doesn't give the same results as the current implementation. For example it doesn't return POINT(0 0) for LINESTRING(0 0,0 0), but MULTILINESTRING((0 0,0 0))
rouault
force-pushed
the
add_GEOSMakeValid_cpp
branch
2 times, most recently
from
53eef58
to
46afb99
Compare
rouault
changed the title
rouault
force-pushed
the
add_GEOSMakeValid_cpp
branch
from
46afb99
to
3ec75a3
Compare
rouault
force-pushed
the
add_GEOSMakeValid_cpp
branch
from
73d5ecf
to
92c4a2e
Compare
|
OK, just pushed changes to rename to BuildArea/GEOSBuildArea (with git history cleaning) |
And GEOSBuildArea_r() (refs libgeos#952)
and add GEOSMakeValid_r() (fixes libgeos#952)
rouault
force-pushed
the
add_GEOSMakeValid_cpp
branch
from
92c4a2e
to
e04ba73
Compare
See https://trac.osgeo.org/geos/ticket/952
This is a proper C++ port of the librttopo code, originally using GEOS C API.