Enhance coverage simplification to handle one tolerance per geometry #1037
Conversation
… geometry) Signed-off-by: N_Strahl <[email protected]>
| * @param tolerances comma-separated string of simplification tolerances for each coverage | ||
| * @return the simplified polygons | ||
| */ | ||
| public static Geometry[] simplify(Geometry[] coverage, String tolerances) { |
There was a problem hiding this comment.
Conversion from a string to an array of tolerances should be left up to client code, since it's highly usage-specific. It can be done in CoverageFunctions as well, to allow for easier testing and experimentation.
There was a problem hiding this comment.
Ok, then I will be moving that method into the JTS test builder code
…ts-app Signed-off-by: N_Strahl <[email protected]>
|
The class Javadoc needs some mention of the semantics when using tolerance-per-geometry. Specifically, there should be a comment like: |
Signed-off-by: N_Strahl <[email protected]>
|
@dr-jts Any other suggestions for changes in this PR? |
| * @param tolerances the simplification tolerances for each coverage | ||
| * @return the simplified polygons | ||
| */ | ||
| public static Geometry[] simplify(Geometry[] coverage, List<Double> tolerances) { |
There was a problem hiding this comment.
Why is tolerances a List and not an array? It seems more uniform for it to have the same structure as the geometries.
| for (int i = 0 ; i < lines.getNumGeometries(); i++) { | ||
| LineString line = (LineString) lines.getGeometryN(i); | ||
| boolean isFree = isFreeRing == null ? false : isFreeRing.get(i); | ||
| double areaTolerance = (areaTolerances.isEmpty()) ? -1 : ((areaTolerances.size() == 1) ? areaTolerances.get(0) : areaTolerances.get(i)); |
There was a problem hiding this comment.
Better to put this complex logic into a function
| CoverageEdge edge; | ||
| LineSegment edgeKey = (end == start) ? CoverageEdge.key(ring) : CoverageEdge.key(ring, start, end); | ||
| if (uniqueEdgeMap.containsKey(edgeKey)) { | ||
| edge = uniqueEdgeMap.get(edgeKey); | ||
| if (!coverageTolerances.isEmpty()){ | ||
| edge.setTolerance((edge.getTolerance() < coverageTolerances.get(coverageId)) ? edge.getTolerance() : coverageTolerances.get(coverageId)); |
There was a problem hiding this comment.
can this logic be factored out into a function?
|
I'm about to land a relatively complex extension to |
|
One of my changes involves adding another tolerance value. Passing all this information down the call chain is getting ugly. I might factor the action of getting a tolerance value out into a strategy class. That can also include the array of per-geom tolerances, which will make this transparent to the rest of the codebase. |
|
Actually my changes impact quite a few method signatures. So I'll refactor mine to support a tolerance strategy class, and then that will make refactoring your code on top of that much easier. |
|
One thing I'm adding is removing small rings below the tolerance area (of MultiPolygons, or holes). I suppose this can just use the area tolerance of the parent geometry. Does this make sense? |
Signed-off-by: N_Strahl <[email protected]>
…erance Signed-off-by: N_Strahl <[email protected]>
Signed-off-by: N_Strahl <[email protected]>
|
@dr-jts I hope these few changes help you out. |
I do think removing tiny polygons or holes is a good idea. This feature could even be activated via a user flag. This reminds me a lot of the "qgis:eliminateselectedpolygons" processing algorithm in QGIS, which actually works by forwarding the geometry checks to GEOS anyway. So it might be helpful to draw some inspiration from that algorithm. It's nice to see this exciting new feature being improved. Is there maybe a publication describing the underlying algorithm? If so, could you forward me the DOI? |
I was not planning to make this optional, but could do so. Do you think it's useful to have both options?
I'll be writing a blog post soon. It's a fairly simple extension to the current algorithm. When I have a PR ready I'll link to this one. |
Signed-off-by: N_Strahl <[email protected]>
|
@nstrahl my changes for small ring removal are larger than this PR. So I'll commit mine, and then you or I can rework this. |
|
Superseded by #1060 |
An enhancement has been made to the coverage simplification algorithm (simplify and simplifyInner). Currently, coverage simplification only allows one global tolerance value to be specified for the entire coverage. The proposed enhancement allows multiple tolerances to be passed in as a list, where each entry corresponds to a geometry. The geometries can thus be simplified with different tolerances while still ensuring topological correctness. At coinciding coverage edges the lowest available value is utilized.
This will be very useful for solving problems related to the boundary discretization of tessellated geometries.
The following figure shows an example where the boundary polygon is simplified with a tolerance of 1, and the other hole polygons with 1 and 0.5.
Here is another one with tolerances of 50 and 10.