Add Polygon constructors and createPolygon functionality #134
Conversation
New createPolygon function allows creation of polygons without user specified holes. Polygon constructor added to convert a LinearRing to Polygon. Polygon constructor that takes a pointer limited by type.
Create a test file for GEOSGeom constructors found in geos_types.jl. Tests pass for Polygon constructed from vectors, but an "illegal instruction" error is thrown when one is constructed from a pointer. The error messages mentions destroyGeom. There are more tests below for constructing from a linear ring but these depend on the pointer constructor and thus cause a seg fault.
Adding cloneGeom around pointers sent to Polygon constructor that takes in a pointer has stopped the tests from seg faulting. All desired Polygon constructors and createPolygon functionality added and tested.
There was a problem hiding this comment.
Thank you so much!
If this does seem like a solution to #133 then it will need to be applied to all types within the geos_types.jl.
It does seem like a solution, and should be applied to all types in geos_types.jl (beyond Polygon), but that doesn't have to be a blocker for this PR, which is well-scoped to polygons and leaves the codebase in a better state than it is currently.
I only have one question pertaining to the name of the arguments, but LGTM otherwise!
| if id == 3 | ||
| polygon = new(cloneGeom(ptr)) | ||
| elseif id == 2 | ||
| polygon = new(cloneGeom(createPolygon(ptr))) |
There was a problem hiding this comment.
Is the cloneGeom needed here? Not sure if LibGEOS takes care of that down the line in createPolygon.
There was a problem hiding this comment.
@evetion I also thought that and didn't have it there initially because I assumed that createPolygon would take care of that through the GEOSGeom_createPolygon_r call. However, my tests were throwing a seg fault without the cloneGeom there. I can try it again, but my conclusion was that it is necessary. However, I did find that concerning because I also assumed thatGEOSGeom_createPolygon_r would take care of it given the description within the C API that it creates "a newly allocated geometry." Thoughts on how to test this out?
There was a problem hiding this comment.
Looking into it, it does look like we should clone them for safety because the created polygon/geometry will take ownership of the input arguments (see e.g. https://trac.osgeo.org/geos/ticket/1111).
My comments are (embarrassingly) confusingly worded in
LibGEOS.jl/src/geos_functions.jl
Line 394 in 2c83c28
LibGEOS.jl/src/geos_functions.jl
Lines 432 to 434 in 2c83c28
There was a problem hiding this comment.
Okay, that makes sense @yeesian. Two more questions on this and then I will fix up these comments and submit it again first thing Monday morning.
(1) Do we think we should be using cloneGeom for the case where the polygon is made from coordinate vectors (lines 92-98)? I don't think we need to given that the pointers interiors and exterior aren't used outside of the constructor so it is okay that the new geometry takes ownership of them. Does this logic seem correct or do you think it is safer to make a copy?
(2) Do you think this might have larger impacts on other parts of the code? For example, what about intersection that calls GEOSIntersection_r(). Do you think we should be cloning the output? Could this explain issue #91?
There was a problem hiding this comment.
(1) Do we think we should be using cloneGeom for the case where the polygon is made from coordinate vectors (lines 92-98)? I don't think we need to given that the pointers interiors and exterior aren't used outside of the constructor so it is okay that the new geometry takes ownership of them. Does this logic seem correct or do you think it is safer to make a copy?
Yeah, the logic seems sound: interiors and exterior are created within the scope of that function, so it's safe. I also doublechecked memory ownership of the vectors being used to construct the coordinate sequences and those are fine too (see [1], [2]).
(2) Do you think this might have larger impacts on other parts of the code? For example, what about intersection that calls GEOSIntersection_r(). Do you think we should be cloning the output? Could this explain issue #91?
To my understanding, the output for GEOSIntersection_r() will be a new object and not require cloning. Unfortunately, I think it's likely that we'll have to do this analysis on a case-by-case for the other functions in the code (e.g. I think it might be argued that we should do it for getGeometries and interior/exteriorRings).
From quickly re-reading the code in this package, my hunch is that it's mainly a risk in this package when dealing with containers of geometries (so e.g. multi*, geometrycollection, and polygon).
|
I am also open to working on fixing up the rest of the constructors and trying to find the source of other memory issues like in issue #91 if anyone has suggestions. |
Typo in comments, remove magic numbers, change variable name
|
Ready for merge! |
This starts to address issue #129 (I have not started on the multi-polygon functionality as I have questions on my current implementation). I added a new method to createPolygon that allows the user to create a polygon without specifying any holes as a convenience. I also added two new Polygon constructors and updated the existing one that takes in a pointer. The two new constructors take in one linear ring, and one linear ring and a vector of linear rings respectively. The updates to the existing constructor make sure that only pointers to polygons and linear rings are used to construct a new Polygon. This needs to be added as before if you passed in a different type of pointer it was simply labeled as a polygon, but nothing internally changed (i.e. if you passed in a line it still is classified as a line and has no area. I also added in two calls to cloneGeom within this functions as it was causing seg faults within testing due to reusing the pointer. I think that this might be a solution to issue #133.
However, I did not add a call to cloneGeom within the polygon constructor that takes in a vector of coordinates. This constructor was not causing a seg fault. I think this is because before using the pointer two new objects are created and then these are used to create the polygon and won't be used elsewhere in the code. However, it might still be beneficial to add a call to cloneGeom. I would love advice on this.
If this does seem like a solution to #133 then it will need to be applied to all types within the geos_types.jl.
I also added testing for new Polygon functionality. Should I explicitly destroy the polygons I create within the test function after using them?