Add GEOSDensify to CAPI #391

brendan-ward · 2021-01-28T19:43:43Z

This adds GEOSDensify to the C API.

It was necessary to guard for empty and NULL geometries, or they result in segfaults from the Densifier. Is that expected, or should those be checked there instead?

I wasn't sure if NULL inputs should be guarded, but avoiding segfaults for those seemed like a good idea.

The densifier behavior was not exactly as I expected when testing: I expected any segments greater than tolerance to be subdivided; the behavior is that any segments greater than or equal to tolerance are subdivided. I tried to clarify that in the function docstring, and I added specific tests for this to the test suite.

pramsey · 2021-01-28T19:54:50Z

I tend towards a send-an-empty-get-an-empty, send-a-null-get-a-null policy, I don't think things that segfault, but frankly I don't know what our policy is, whether it's "avoid the footguns" or "no footguns". @dbaston ?

dbaston · 2021-01-28T20:05:33Z

I'd lean towards returning EMPTY on EMPTY (at the Densifier level, not specifically to the C API) and segfaulting on NULL. Dereferencing a null pointer is a programming error and I don't think runtime guards against programming errors are generally a good idea.

brendan-ward · 2021-01-28T20:19:26Z

Possible compatibility issue: PostGIS interprets tolerance as greater than rather than greater than or equal to as is done here (I prefer the PostGIS interpretation). Not sure about JTS.

A tolerance of 10 in PostGIS matches the inputs for our first test case, whereas GEOS requires >10 (all segments in input geometry are <= 10).

It might be good to resolve this before making this publicly available in the CAPI. Should I modify Densifier to handle this (perhaps should be a separate PR)?

pramsey · 2021-01-28T23:44:13Z

This behaviour in GEOS would be coming from JTS. @dr-jts might have an opinion/explanation.

capi/geos_ts_c.cpp

tests/unit/capi/GEOSDensifyTest.cpp

brendan-ward · 2021-01-29T02:43:56Z

It looks like the exact tolerance value is not specifically tested in the JTS densifier tests

The description there is a bit ambiguous on this front as well:

All segments in the created densified geometry will be no longer than the given distance tolerance.

dbaston · 2021-01-29T02:49:01Z

Looks great to me. Thanks for this, @brendan-ward !

pramsey · 2021-01-29T17:00:41Z

@brendan-ward As long as we're consistent with JTS behaviour I don't have any particular horse in the race.

dr-jts · 2021-01-29T17:13:49Z

This behaviour in GEOS would be coming from JTS. @dr-jts might have an opinion/explanation.

This looks like a bug in the JTS code. I agree that only segments longer than the distance tolerance should be densified. This is the semantics implied by a strict interpretation of:

All segments in the created densified geometry will be no longer than the given distance tolerance.

I will change the JTS behaviour to match this. It's good to have it match PostGIS.

pramsey · 2021-01-29T17:14:54Z

There you go @brendan-ward, match the postgis behaviour and let me know, I'll merge.

dr-jts · 2021-01-29T17:49:29Z

There you go @brendan-ward, match the postgis behaviour and let me know, I'll merge.

I am about to pull the PR trigger in JTS, so might want to look at that code when it lands. (Not that it is definitely the World's Best Code, but nice to match it unless there's a good reason).

dr-jts · 2021-01-29T18:00:03Z

JTS PR 676 has this fix.

brendan-ward · 2021-01-30T04:01:28Z

Thanks @dr-jts !

I'm not sure the fix was quite complete though; the original approach would create more segments than necessary to satisfy the requirement of <= tolerance. It also looks like part of the fix would add a duplicate last coordinate.

I adapted the intent here and used ceil to calculate the number of new segments required for each to be less than or equal to tolerance. Some segments were originally cut more than they needed to be; e.g., a tolerance of 5 on a segment that was 10 long would produce 3 segments rather than the required 2.

This required some minor updates to the existing XMLtests here to match the new segment lengths.

brendan-ward · 2021-01-30T04:02:44Z

tests/xmltester/tests/general/TestDensify.xml

-  ((0 0, 0 8.75, 0 17.5, 0 26.25, 0 35, 0 43.75, 0 52.5, 0 61.25, 0 70, 8.75 70, 17.5 70, 26.25 70, 35 70, 43.75 70, 52.5 70, 61.25 70, 70 70, 70 61.25, 70 52.5, 70 43.75, 70 35, 70 26.25, 70 17.5, 70 8.75, 70 0, 61.25 0, 52.5 0, 43.75 0, 35 0, 26.25 0, 17.5 0, 8.75 0, 0 0), 
-    (10 10, 16.25 16.25, 22.5 22.5, 28.75 28.75, 35 35, 41.25 41.25, 47.5 47.5, 53.75 53.75, 60 60, 51.66666666666667 60, 43.333333333333336 60, 35 60, 26.666666666666668 60, 18.333333333333336 60, 10 60, 10 51.66666666666667, 10 43.333333333333336, 10 35, 10 26.666666666666668, 10 18.333333333333336, 10 10)))      	</op></test>
+<test><op name="densify" arg1='A'  arg2='10.0'>
+  MULTIPOLYGON (((80 70, 80 80, 80 90, 80 100, 80 110, 90 110, 100 110, 110 110, 120 110, 120 100, 120 90, 120 80, 120 70, 110 70, 100 70, 90 70, 80 70)),


Note: first ring normalized (rotated) a bit differently than previous version. The primary change on this ring is that segments are now 10 long, as expected from the tolerance.

If you are open to it, I'd like to simplify the coordinates of these densify tests to make it easier to spot and verify the expected output.

Excellent idea. If you fix them up I will copy them to JTS.

dr-jts · 2021-01-30T04:20:03Z

I'm not sure the fix was quite complete though

Good point. I will follow your lead on the correct code.

dr-jts · 2021-01-30T20:53:02Z

JTS PR 677 has the improved splitting algorithm from this PR.

It also adds tests to the XML test file to capture the various additional tests in the unit test code (since it is more portable to test them in the XML files). This could be copied to this PR.

Thanks for working on this @brendan-ward - this is a great improvement over the old code.

dr-jts · 2021-01-30T20:56:04Z

tests/unit/capi/GEOSDensifyTest.cpp

+    geom1_ = GEOSGeomFromWKT("POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (1 1, 1 7, 7 7, 7 1, 1 1))");
+    ensure(geom1_ != nullptr);
+    GEOSSetSRID(geom1_, 3857);
+
+    geom2_ = GEOSDensify(geom1_, 5.0);
+
+    ensure("result not null", geom2_ != nullptr);
+    ensure_geometry_equals(
+        geom2_,
+        "POLYGON ((0 0, 5 0, 10 0, 10 5, 10 10, 5 10, 0 10, 0 5, 0 0), (1 1, 1 4, 1 7, 4 7, 7 7, 7 4, 7 1, 4 1, 1 1))");
+
+    ensure_equals("result SRID == expected SRID", GEOSGetSRID(geom2_), 3857);


Sure would be nice to reduce the amount of code for tests like this (not just here, for all GEOS unit tests).

Any ideas, @dbaston ? Perhaps some more helper functions?

If I were writing it I would drop lines 77 and 82, but overall it seems pretty reasonable to me.

I don't have a problem with those lines per se (although if they are redundant would be good to remove them). The real problem is how many lines of code are repeated in each test. The only information content is the input WKT, the densify tolerance, and the result WKT.

In JTS I factor out the boilerplate into a separate function which is called by each test. One thing this supports nicely is that if the test logic needs to be modified or enhanced (say with an extra test on a specific aspect of the result) it can be done in one place only. (In other words, the standard reason for refactoring :)

I'm just not sure if the tut templated approach supports that?

It turns out that factoring out the test common code into a support function is very doable. See GEOSMinimumClearanceTest.cpp for an example.

@brendan-ward how do you feel about making this improvement?

Yes, some of the tests do something like doDensifierTest. I actually find those harder to work with, mostly because our test framework has no way to run an individual test. So if you want to debug a test and the test code itself is in a common function, you have to comment out all of the remaining tests.

Yeah, it's a balance. I've found having support functions tucked away in another file makes things more painful, but having them in the actual Test file is a-OK. Also, we can run individual tests (not assertions) with ./bin/test_geos_unit geos::io::WKBReader 6 so having more numbered chunks is often preferable to a long list of assertions, or a loop over a data array (my nemesis).

Also, we can run individual tests (not assertions) with ./bin/test_geos_unit geos::io::WKBReader 6

How am I only learning this now? 🤦

I only found it out a couple months ago! I was like... "what if I add the number..."

(Also, the numbers don't have to be sequential)

And you could make the opposite argument

Of course. My experience makes me less inclined to write something today the way I wrote the minimum clearance tests 5 years ago. Your experience seems to pull you in the other direction. That's fine.

dbaston · 2021-02-01T16:16:51Z

I've merged this in, but please don't hesitate to submit a new PR if you want to make additional changes. Thanks!

brendan-ward added 2 commits January 28, 2021 11:35

Add GEOSDensify to CAPI

7960e9f

Fix geometry variable name

51c6e77

Remove guard for null geometry and associated test

64ccc7c

dbaston reviewed Jan 29, 2021

View reviewed changes

capi/geos_ts_c.cpp Outdated Show resolved Hide resolved

dbaston reviewed Jan 29, 2021

View reviewed changes

tests/unit/capi/GEOSDensifyTest.cpp Outdated Show resolved Hide resolved

Move empty check to Densifier; reduce test boilerplate

9429ff3

Ensure densify results in segments <= tolerance

e5dad0d

brendan-ward commented Jan 30, 2021

View reviewed changes

dr-jts mentioned this pull request Jan 30, 2021

Improve Densifier splitting algorithm locationtech/jts#677

Merged

dr-jts reviewed Jan 30, 2021

View reviewed changes

strk merged commit fff6a78 into libgeos:master Feb 1, 2021

brendan-ward deleted the add_densify_c_api branch February 1, 2021 16:19

brendan-ward mentioned this pull request Feb 2, 2021

TST: Expand XML densify tests, reduce boilerplate in C API densify tests #399

Merged

pramsey mentioned this pull request Nov 14, 2022

3.11.1: test suite is failing in test_docs unit #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GEOSDensify to CAPI #391

Add GEOSDensify to CAPI #391

brendan-ward commented Jan 28, 2021

pramsey commented Jan 28, 2021

dbaston commented Jan 28, 2021

brendan-ward commented Jan 28, 2021

pramsey commented Jan 28, 2021

brendan-ward commented Jan 29, 2021

dbaston commented Jan 29, 2021

pramsey commented Jan 29, 2021

dr-jts commented Jan 29, 2021

pramsey commented Jan 29, 2021

dr-jts commented Jan 29, 2021

dr-jts commented Jan 29, 2021

brendan-ward commented Jan 30, 2021

brendan-ward Jan 30, 2021

brendan-ward Jan 30, 2021

dr-jts Jan 30, 2021 •

edited

Loading

dr-jts commented Jan 30, 2021

dr-jts commented Jan 30, 2021

dr-jts Jan 30, 2021

dbaston Jan 30, 2021

dr-jts Jan 30, 2021 •

edited

Loading

dr-jts Jan 30, 2021 •

edited

Loading

dbaston Jan 30, 2021

pramsey Jan 31, 2021

dbaston Jan 31, 2021

pramsey Jan 31, 2021

pramsey Jan 31, 2021

dbaston Jan 31, 2021

dbaston commented Feb 1, 2021

Add GEOSDensify to CAPI #391

Add GEOSDensify to CAPI #391

Conversation

brendan-ward commented Jan 28, 2021

pramsey commented Jan 28, 2021

dbaston commented Jan 28, 2021

brendan-ward commented Jan 28, 2021

pramsey commented Jan 28, 2021

brendan-ward commented Jan 29, 2021

dbaston commented Jan 29, 2021

pramsey commented Jan 29, 2021

dr-jts commented Jan 29, 2021

pramsey commented Jan 29, 2021

dr-jts commented Jan 29, 2021

dr-jts commented Jan 29, 2021

brendan-ward commented Jan 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-jts Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

dr-jts commented Jan 30, 2021

dr-jts commented Jan 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-jts Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

dr-jts Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaston commented Feb 1, 2021

dr-jts Jan 30, 2021 •

edited

Loading

dr-jts Jan 30, 2021 •

edited

Loading

dr-jts Jan 30, 2021 •

edited

Loading