Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ring-endpoint-handling for LineString with isRing() == true #1033

Closed
wants to merge 5 commits into from

Conversation

FObermaier
Copy link
Contributor

@FObermaier FObermaier commented Jan 25, 2024

With the current state of TopologyPreservingSimplifier and related classes, the LINEARRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180) simplifies to LINEARRING (30 220, 380 220, 300 40, 140 30, 30 220) using a tolerance value of 40. (see here)

Performing the same operation on LINESTRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180) gives LINESTRING (220 180, 380 220, 300 40, 140 30, 30 220, 220 180).

* Remove redundant casts
* Remove unnecessary local variable
Note: visibility of constructor is better than that of its arguments
@dr-jts
Copy link
Contributor

dr-jts commented Jan 25, 2024

It's not clear from the description what this PR does?

Are you suggesting changing the TopologyPreservingSimplifier behaviour so that it simplifies the endpoint of closed LineStrings? If so, I disagree - the contract is to preserve the endpoints of LineStrings.

public void testLinearRingRemoveEndpoint() throws Exception {
checkTPS(
"LINEARRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180)",
40,
"LINEARRING (30 220, 380 220, 300 40, 140 30, 30 220)"
);
checkTPS(
"LINESTRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180)",
Copy link
Contributor

@dr-jts dr-jts Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is intended to test behaviour on LINEARRING geometries. A LINESTRING geometry should be in a separate test.

There should be a test for closed LineStrings, even if this behaviour PR is not accepted. I will add that once this has be closed.

@FObermaier
Copy link
Contributor Author

Are you suggesting changing the TopologyPreservingSimplifier behaviour so that it simplifies the endpoint of closed LineStrings?

Yes.

If so, I disagree - the contract is to preserve the endpoints of LineStrings.

Take a look at the following snipplet:

Geometry ls = read("LINESTRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180)");
Geometry lr = read("LINEARRING (220 180, 261 175, 380 220, 300 40, 140 30, 30 220, 176 176, 220 180)");
assertTrue(ls.equals(lr));
assertTrue(ls.equalsExact(lr));

If this is true, then any operation on either of these geometries should give the same result.

@dr-jts
Copy link
Contributor

dr-jts commented Jan 26, 2024

If this is true, then any operation on either of these geometries should give the same result.

Not necessarily. The OGC topological model is somewhat limited in terms of real-world requirements, IMHO. That's why the concept of BoundaryNodeRule was introduced - to give more control over how endpoints are treated.

For an example, consider simplifying a road network that contains "loops" - e.g. turn-arounds modelled as closed LineStrings. In order to preserve network integrity, the endpoints of all lines must be preserved, including those of closed LineStrings.

What could be done is allow the simplify behaviour to be controlled by an option flag. This takes effort and complicates the API, so is there a clear practical use case for this?

@FObermaier
Copy link
Contributor Author

You have a point regarding LineStrings in network models.

@FObermaier FObermaier closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants