IndexedFacetDistance

[dbaston]

This PR ports the IndexedFacetDistance class from JTS, which can massively speed up distance calculations. It adds a GEOSDistanceIndexed method to the C API which can take advantage of this. There is currently no way in the C API to reuse an IndexedFacetDistance for multiple calculations, although this could be added in the future.

[strk]

XMLTester-XMLTester.o: In function `XMLTester::parseTest(TiXmlNode const*)':
/usr/src/geos/geos/tests/xmltester/XMLTester.cpp:1192: undefined reference to `geos::geom::GeometryFactory::createGeometryCollection(std::vector<geos::geom::Geometry*, std::allocator<geos::geom::Geometry*> >*) const'
/usr/src/geos/geos/tests/xmltester/XMLTester.cpp:1220: undefined reference to `geos::geom::GeometryFactory::createGeometryCollection(std::vector<geos::geom::Geometry*, std::allocator<geos::geom::Geometry*> >*) const'

[dbaston]

@strk is this failure related to the PR? I'm unable to reproduce it.

[strk]

Uhm, you seem to be right, Travis is also happy (someone please kill AppVeyor!)

[strk]

I've removed the appveyor webhook, could you please rebase and force-push ?

[strk]

Can you please:

• Squash to become a single commit
• Specify which new APIs are added with the change (maybe in the NEWS file)

[strk]

Oh, and make sure to reference a trac ticket in your commit log (and this PR)

[strk]

You were right, it was my fault. Committed as r4295 = ff501d3 (refs/remotes/trunk)

[strk]

Actually, I just noticed that the new unit test was not enabled. I'll do it, but please make sure the tests run in the buildbots.

[strk]

The patch to enable the test:

diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 4ce182e..999dbaa 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -120,6 +120,7 @@ geos_unit_SOURCES = \
        capi/GEOSClipByRectTest.cpp \
        capi/GEOSCoordSeqTest.cpp \
        capi/GEOSDelaunayTriangulationTest.cpp \
+       capi/GEOSDistanceTest.cpp \
        capi/GEOSVoronoiDiagramTest.cpp \
        capi/GEOSGeomFromWKBTest.cpp \
        capi/GEOSGeomToWKTTest.cpp \

Maybe you can rebase your branch against svn-trunk and keep force-pushing
to this same branch

[strk]

Actually, it's best if you take care of this, as it doesn't build: tests/unit/tut/tut.hpp:273: multiple definition of `tut::test_capigeosdistance_group'

[strk]

Sorry, I just realized the test was enabled already, so the error were triggered by my duplicating them (guess sorting those entries in Makefile.am might help reducing this confusion)

[strk]

Please fix memory leaks exposed by running the test:

libtool --mode=execute valgrind --leak-check=full ./geos_unit capi::GEOSDistance

Here is the report I got:

==14108== 4,704 bytes in 84 blocks are definitely lost in loss record 3 of 5    
==14108==    at 0x4C2E105: operator new(unsigned long) (vg_replace_malloc.c:327)
==14108==    by 0x4F5E75B: geos::operation::distance::FacetSequenceTreeBuilder::addFacetSequences(geos::geom::CoordinateSequence const*, std::vector<geos::operation::distance::FacetSequence*, std::allocator<geos::operation::distance::FacetSequence*> >&) (FacetSequenceTreeBuilder.cpp:79)
==14108==    by 0x4EFE583: geos::geom::Polygon::apply_ro(geos::geom::GeometryComponentFilter*) const (Polygon.cpp:407)
==14108==    by 0x4F5E58E: geos::operation::distance::FacetSequenceTreeBuilder::computeFacetSequences(geos::geom::Geometry const*) (FacetSequenceTreeBuilder.cpp:64)
==14108==    by 0x4F5E60E: geos::operation::distance::FacetSequenceTreeBuilder::build(geos::geom::Geometry const*) (FacetSequenceTreeBuilder.cpp:32)
==14108==    by 0x4F5EB5B: geos::operation::distance::IndexedFacetDistance::getDistance(geos::geom::Geometry const*) const (IndexedFacetDistance.cpp:40)
==14108==    by 0x4F5EBFB: geos::operation::distance::IndexedFacetDistance::distance(geos::geom::Geometry const*, geos::geom::Geometry const*) (IndexedFacetDistance.cpp:30)
==14108==    by 0x52051FD: GEOSDistanceIndexed_r (geos_ts_c.cpp:1181)           
==14108==    by 0x5F8A2D: void tut::test_object<tut::test_capigeosdistance_data>::test<2>() (GEOSDistanceTest.cpp:125)
==14108==    by 0x5FB80A: run_test_seh_ (tut.hpp:452)                           
==14108==    by 0x5FB80A: tut::test_group<tut::test_capigeosdistance_data, 50>::run_test_(std::_Rb_tree_iterator<std::pair<int const, void (tut::test_object<tut::test_capigeosdistance_data>::*)()> > const&, tut::test_group<tut::test_capigeosdistance_data, 50>::safe_holder<tut::test_object<tut::test_capigeosdistance_data> >&, tut::test_result&) (tut.hpp:372)
==14108==    by 0x5FC427: tut::test_group<tut::test_capigeosdistance_data, 50>::run_next(tut::test_result&) (tut.hpp:329)
==14108==    by 0x418C37: run_all_tests_in_group_ (tut_runner.hpp:323)          
==14108==    by 0x418C37: tut::test_runner::run_tests(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (tut_runner.hpp:232)
==14108==                                                                       
==14108== 9,352 bytes in 167 blocks are definitely lost in loss record 4 of 5   
==14108==    at 0x4C2E105: operator new(unsigned long) (vg_replace_malloc.c:327)
==14108==    by 0x4F5E75B: geos::operation::distance::FacetSequenceTreeBuilder::addFacetSequences(geos::geom::CoordinateSequence const*, std::vector<geos::operation::distance::FacetSequence*, std::allocator<geos::operation::distance::FacetSequence*> >&) (FacetSequenceTreeBuilder.cpp:79)
==14108==    by 0x4EFE583: geos::geom::Polygon::apply_ro(geos::geom::GeometryComponentFilter*) const (Polygon.cpp:407)
==14108==    by 0x4F5E58E: geos::operation::distance::FacetSequenceTreeBuilder::computeFacetSequences(geos::geom::Geometry const*) (FacetSequenceTreeBuilder.cpp:64)
==14108==    by 0x4F5E60E: geos::operation::distance::FacetSequenceTreeBuilder::build(geos::geom::Geometry const*) (FacetSequenceTreeBuilder.cpp:32)
==14108==    by 0x4F5EBEC: IndexedFacetDistance (IndexedFacetDistance.h:30)     
==14108==    by 0x4F5EBEC: geos::operation::distance::IndexedFacetDistance::distance(geos::geom::Geometry const*, geos::geom::Geometry const*) (IndexedFacetDistance.cpp:29)
==14108==    by 0x52051FD: GEOSDistanceIndexed_r (geos_ts_c.cpp:1181)           
==14108==    by 0x5F8A2D: void tut::test_object<tut::test_capigeosdistance_data>::test<2>() (GEOSDistanceTest.cpp:125)
==14108==    by 0x5FB80A: run_test_seh_ (tut.hpp:452)                           
==14108==    by 0x5FB80A: tut::test_group<tut::test_capigeosdistance_data, 50>::run_test_(std::_Rb_tree_iterator<std::pair<int const, void (tut::test_object<tut::test_capigeosdistance_data>::*)()> > const&, tut::test_group<tut::test_capigeosdistance_data, 50>::safe_holder<tut::test_object<tut::test_capigeosdistance_data> >&, tut::test_result&) (tut.hpp:372)
==14108==    by 0x5FC427: tut::test_group<tut::test_capigeosdistance_data, 50>::run_next(tut::test_result&) (tut.hpp:329)
==14108==    by 0x418C37: run_all_tests_in_group_ (tut_runner.hpp:323)          
==14108==    by 0x418C37: tut::test_runner::run_tests(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (tut_runner.hpp:232)
==14108==    by 0x414306: main (geos_unit.cpp:95)                               
==14108==                                                                       
==14108== LEAK SUMMARY:                                                         
==14108==    definitely lost: 14,056 bytes in 251 blocks                        
==14108==    indirectly lost: 0 bytes in 0 blocks                               
==14108==      possibly lost: 0 bytes in 0 blocks                               
==14108==    still reachable: 72,760 bytes in 3 blocks                          
==14108==         suppressed: 0 bytes in 0 blocks                    

[strk]

Could you please squash-rebase this ?

[dbaston]

@strk did that work?

[strk]

More or less, except the commit is attributed to me ... try git rebase --reset-author svn-trunk

[strk]

(I just realized it's not a big deal, as authorship info is going to be lost anyway)

[strk]

Worked, no more leaks exposed by test:
r4300 = b7943e7 (refs/remotes/trunk)