Port MinimumDiameter::getMinimumRectangle algorithm from JTS

[nyalldawson]

This commit ports the JTS MinimumDiameter::getMinimumRectangle algorithm and adds it to the c API.

I've tried to model the code as much as possible off the existing algorithms/unit tests, but this is my first contribution to GEOS (hopefully many more to come!) and I may be unaware of what's considered best-practice here.

[nyalldawson]

I'm guessing the appveyor failure isn't related to this PR?

[strk]

Could you please remove CR from
tests/unit/capi/GEOSMinimumRectangleTest.cpp
?

Out of curiosity, does "git show --check" warn you about those too ?

Also please file a ticket on trac referencing this PR.

Good work, btw ! :)

[mwtoews]

The trac ticket is at https://trac.osgeo.org/geos/ticket/729

[strk]

Thanks, please add a "Fixes #729" comment in the commit log on next force-push.

[strk]

@mloskot what happened to your appveyor branch ? Should you disable checking PRs until it works in a test branch ?

[nyalldawson]

@strk Thanks for the feedback - I've updated the PR to fix the whitespace issues & update the commit log message. And yes, git show --check was reporting those errors for me too, so thanks for alerting me to this!

[strk]

Another thing, I've been adding "Last Port" headers to files to track when was a class last being updated. See an example here: https://github.com/libgeos/libgeos/blob/svn-trunk/include/geos/algorithm/CentroidArea.h#L16

It looks like the latest change in that file for JTS was adding static methods for getMinimumRectangle and getMinimumDiameter in the MinimumDiameter class with r966 (March 2015) and some Javadoc fixes were added with r705 (December 2012)

The class was initially ported to GEOS in 2004 (JTS 1.4) - the JTS history for that file starts in 2010 (I guess it was lost in the CVS to SVN transition).

What about taking a quick pass to the JTS class (is short) and adding the "Last port" item (in both .cpp and .h files) so we're in sync and don't have to check this again in the future ?

Oh, and if you do this, could you also write the commit log to be complete within 70 cols in the first line by moving the detail (included ticket reference) to after a new line ?

Ok, enough nitpicking :)

[strk]

MinimumDiameter.cpp:304:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
for (int i = 0; i < n; i++) {
^

[strk]

Another thing: the new testcases are not enabled, they need to be added to tests/unit/Makefile.am (and probably in some CMakeFile too but I usually don't do that part)

[strk]

FYI: I've pushed a "pull54" branch in my fork, if you want to pull from it :)

The following changes since commit 32d1008:

Port MinimumDiameter::getMinimumRectangle algorithm from JTS and add to C API. Fixes #729. (2015-11-12 22:03:35 +1100)

are available in the git repository at:

git://github.com/strk/libgeos.git pr/54

for you to fetch changes up to 66774e7:

Enable unit tests (2015-11-12 12:43:58 +0100)

---

Sandro Santilli (1):
Enable unit tests

tests/unit/Makefile.am | 2 ++
1 file changed, 2 insertions(+)

[nyalldawson]

@strk all done. I've resynced it to JTS r966 (just the new static methods + doc changes), fixed the warning and commit message, pulled your extra commit (no changes required for cmake, that's automatically handled), also fixed a few memory leaks (which probably could never actually happen, but better to be safe in case things change in future).

[nyalldawson]

@strk I've added tests & protection against null/non polygon geometries

[strk]

Great, thanks !
One last thing is the naming which may be a bit confusing.
Should the method name include a "Rotated" portion to avoid confusing it with the "minimum bounding rectangle" ?

Please also squash-rebase against current trunk and force-push here.

[nyalldawson]

I also think the name is badly chosen, but it's what JTS uses. Is there an issue if we change it for GEOS?

[nyalldawson]

@strk I've squashed & force pushed, with the original JTS name intact for now. So if you're happy with that we should be good to go.

Otherwise, what do you think about "minimumRotatedRectangle" as an alternative name?

[strk]

+1 for "minimumRotatedRectangle". JTS naming is of a method within a class which gives more context (mentioning "diameter").

[strk]

btw, exposing minimumDiameter would be also useful as it would allow PostGIS to use the GEOS implementation for its https://trac.osgeo.org/postgis/ticket/2996

\cc @dbaston

[nyalldawson]

@strk how's this? I've:

• renamed c api methods to GEOSMinimumRotatedRectangle
• ported missing tests for getDiameter from JTS
• added GEOSMinimumDiameter to expose minimumDiameter to c api (with tests)

[strk]

This is great ! Committed as r4123 = 7061e93 (refs/remotes/trunk)

I'm tempted to ask more as you're so good at it -- want to look at exposing these 2 new CAPI methods to the PHP binding ? :)

[dbaston]

Realize I'm a bit late on this one, but I would consider using a name other than GEOSMinimumDiameter, since "minimum diameter" could give the impression that this is somehow related to a minimum bounding circle (which it its not). GEOSMinimumWidth ?

[strk]

It is related to a minimum bounding circle, as far as I understood.
It's the diameter of the smallest hole in can put your polygon into.
How's that different from the minimum bounding circle ?

[dbaston]

The minimum diameter is described in JTS as

The minimum diameter is defined to be the width of the smallest band that
contains the geometry, where a band is a strip of the plane defined by two
parallel lines. This can be thought of as the smallest hole that the geometry
can be moved through, with a single rotation.

The key here is "with a single rotation," with LINESTRING (0 0, 0 1e6) being a good example of the difference. The diameter of its minimum bounding circle is 1e6, but the JTS "minimum diameter" is zero.

[strk]

Now I see it. I agree GEOSMinimumWidth would be better. Up for a patch ? It's not too late for it :)

[nyalldawson]

@strk see #58