Fix CoverageSimplifier to handle holes correctly

[mukoki]

Fixes #953

[dr-jts]

Nice fix - thanks.

It would be best to include the two test cases as unit tests.

[mukoki]

Unfortunately, writing the tests revealed the need to go deeper in the change. Currently, the simplifier removes corners with a distinction between

• normal edge case (end points are preserved) and
• ring case (any corner may be removed as far as three distinct points are left). In the case of a hole touching another hole or touching the shell, we would like to preserve the starting node (which is the node touching the other ring), but edges are passed to the simplifier as a MultiLineString and information that the end node must be preserved is lost. We could preserve initial node everywhen, but for a free ring, it is not required. Do you agree to pass a bit more information than a MultiLineString to the simplifier ?

[dr-jts]

Unfortunately, writing the tests revealed the need to go deeper in the change.

Yay for unit tests!

Do you agree to pass a bit more information than a MultiLineString to the simplifier ?

Yes, that seems to be necessary. What are your ideas for doing this? Perhaps an array of booleans indicating whether a ring can be fully simplified, or should have its endpoint preserved?

I think the TPVWSimplifier needs to be enhanced so that it can remove the endpoint corner of rings, as well.

[dr-jts]

See also #962, which I think requires a different fix.

[mukoki]

I pushed a new commit adding an attribute to CoverageEdge indicating how many nodes need to be preserved (0, 1 or 2). It is a much deeper change. Let me know what you think. The main drawback is that I removed public static method from TPVWSimplifier as MultiLineString is not sufficiant to preserve topology :-( Also added two tests from #953 report

wrt TPVWSimplifier, it could already remove endpoint corner of rings and that was the point ! The LinearRing to CoverageEdge transformation was setting the endpoint on the node touching another ring if any, and but it could be removed as any vertex of a ring. Now it should be preserved (processed as if it was a linestring, not a ring).

See also my comments on #962

[dr-jts]

Another alternative is to provide TPVWSimplifier with a Set of coordinates which are ring nodes that must be preserved.

I prefer this approach, since it reduces the amount of shared classes. This makes TPVWSimplifier more generic. At some point it may be promoted to being a public class (and moved to the 'simplify` package).

[mukoki]

I see. In this case, wouln't it be preferable to pass constrained rings (containing one node) as a third MultiLineString beside normal edges (0/2 nodes) and constraints to avoid computation of edges from nodes in TPVWSimplifier.

[dr-jts]

Wouln't it be preferable to pass constrained rings (containing one node) as a third MultiLineString beside normal edges (0/2 nodes) and constraints to avoid computation of edges from nodes in TPVWSimplifier.

That is another option. I think I'd prefer passing the edges which have their endpoints to be preserved (line or ring), and the ones (rings only) which can have all vertices simplified. Does this make sense?

It's the same amount of work in either case, since the "rings with nodes" have to be determined by checking if their endpoint is in the set of identified nodes. But maybe it is best to keep this processing outside of TPVWSimplifier.

[dr-jts]

Superseded by #963