Performance improvement #141
Conversation
…thms::isPointInRing
|
Thanks for this, @doskabouter . I haven't merged it yet because I want to verify / quantify the performance impact myself. (I did test your earlier PR through PostGIS and saw quite a large performance gain, but Postgres adds a lot of noise to performance testing. I'd like to try it by calling GEOS directly from a simple C program.) |
# Conflicts: # src/operation/overlay/PolygonBuilder.cpp
|
And another 25% faster. Total improvement: twice as fast |
JTS b1c0a1e2992c3f3d523dcfb3c0158fbac72b6928
JTS ef92b816438c416303df2eecbc0f655db6e21466
can get MinGW builds at all
…abouter/geos into doskabouter-st_intersection_improvement
|
I tested this without PostGIS in the loop and am seeing similar improvements. Intersects predicate goes from 24 to 11 seconds; intersection overlay goes from 48 to 20 seconds. |
| @@ -152,6 +152,8 @@ class GEOS_DLL PolygonBuilder { | |||
| std::vector<geomgraph::EdgeRing*> &newShellList, | |||
| std::vector<geomgraph::EdgeRing*> &freeHoleList); | |||
|
|
|||
| using FastPIPRing = tuple<EdgeRing*, algorithm::locate::IndexedPointInAreaLocator*>; | |||
There was a problem hiding this comment.
What do you think about defining an internal struct that holds this stuff? (I don't find get<0> terribly descriptive when I see it used.)
Also, any reason not to use std::unique_ptr<IndexedPointInAreaLocator> ?
There was a problem hiding this comment.
internal struct sounds fine, I'll do that soon.
not using std::unique_ptr is due to my inexperience with c++. I assume that when used, the loop where those are delete'd can be removed?
There was a problem hiding this comment.
I assume that when used, the loop where those are delete'd can be removed?
Yes, exactly
There was a problem hiding this comment.
Well, it's quite a hassle (in fact I didn't succeed at it yet), to have a std::vector of structs containing a std::unique_ptr, I keep getting compiler errors about "attempting to reference a deleted function".
I could probably fix this by adding appropriate copy-constructor/move-constructor/destrutors but that seems (at least to me at the moment) like quite the overkill.
There was a problem hiding this comment.
Are you working with Visual Studio by chance? If so, you may need to explicitly delete the copy constructor and assignment operator; see https://github.com/libgeos/geos/blob/master/include/geos/index/strtree/SIRtree.h#L83
If you can't make it work, just leave the raw pointer.
There was a problem hiding this comment.
Indeed I'm using VS (2017). Must say those copy constructor issues are the least of the problems...
Main one being that VS doesn't resolve the namespaces correctly (I had to prepend a huge number of types with their namespace to get it to compile, and as such it's quite a hassle to create a proper pullrequesst,...)
| @@ -336,7 +342,8 @@ PolygonBuilder::findEdgeRingContaining(EdgeRing *testEr, | |||
|
|
|||
| for(auto const& tryShell: newShellList) | |||
| { | |||
| LinearRing *tryShellRing=tryShell->getLinearRing(); | |||
| ; | |||
| @@ -348,15 +355,15 @@ PolygonBuilder::findEdgeRingContaining(EdgeRing *testEr, | |||
| Coordinate testPt = operation::polygonize::EdgeRing::ptNotInList(testRing->getCoordinatesRO(), tsrcs); | |||
| bool isContained=false; | |||
|
|
|||
| if(PointLocation::isInRing(testPt, tsrcs)) | |||
| if (get<1>(tryShell)->locate(&testPt) != Location::EXTERIOR) | |||
There was a problem hiding this comment.
Fix indentation, use braces for if (style consistency with nearby code)
There was a problem hiding this comment.
Braces were not present before, so I left it like that.
Will add them though
|
Shall I continue adding improvements to this branch, (perhaps after this PR is merged?) or shall I create a new one for some other (f.e. missing envelope-checks) improvements? |
|
I'd make a separate branch (and PR) for any improvements unrelated to this one. |
|
Ok, will do |
First check envelope before calling CGAlgorithms::isPointInRing