Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems arise when using LinearRing convenience initializer #37

Closed
biomiker opened this issue Mar 27, 2017 · 5 comments
Closed

Problems arise when using LinearRing convenience initializer #37

biomiker opened this issue Mar 27, 2017 · 5 comments

Comments

@biomiker
Copy link

First off, thank you, this library is amazing! Here's the issue I found.

The convenience initializer LinearRing(points: [Coordinate]) gives you back a LinearRing object but not quite a valid one. This is evident in its WKT which describes itself as a LINESTRING. It also causes errors in methods that expect a valid LinearRing, such as the convenience initializer
Polygon(shell: LinearRing, holes: Array<LinearRing>?).

Some sample code and output follows to help illustrate the problem. If I take the LinearRing's WKT, replace LINESTRING with LINEARRING, and pass it to Geometry.create(), I end up with a valid LinearRing.

    func showLinearRingError() {
        
        let origin = MKMapPointForCoordinate(CLLocationCoordinate2D(latitude: 37.9505544833345, longitude: -100.990952407583))

        var mapPoints = [MKMapPoint]()

        mapPoints.append(origin)
        mapPoints.append(MKMapPoint(x:origin.x+10000,y:origin.y+20000))
        mapPoints.append(MKMapPoint(x:origin.x,y:origin.y+20000))
        mapPoints.append(origin)

        let coords = mapPoints.map({ (mp: MKMapPoint) -> Coordinate in
            return CoordinateFromCLLocationCoordinate2D(MKCoordinateForMapPoint(mp))
            })
        
        print(String(describing: coords))
        
        guard let lr1 = LinearRing(points: coords) else {
            print("LR1 failed")
            return
        }
        
        print("-------- LR1 ---------")
        print(String(describing: lr1))

        guard let wkt1 = lr1.WKT else {
            print ("Couldn't get WKT for LR1");
            return
        }
        print(wkt1)
        print("----------------------")
        
        let wkt_new = wkt1.replacingOccurrences(of: "LINESTRING", with: "LINEARRING")
        
        print("NEW WKT = " + wkt_new)
        
        guard let lr2 = Geometry.create(wkt_new) as? LinearRing else {
            print("LR2 failed")
            return
        }
        print("-------- LR2 ---------")
        print(String(describing: lr2))
        
        guard let wkt2 = lr2.WKT else {
            print ("Couldn't get WKT for LR2");
            return
        }
        print(wkt2)
        print("----------------------")
        
        if let polygon1 = Polygon(shell: lr1, holes: nil) {
            print("Created polygon using shell lr1")
            print(String(describing: polygon1))
        }
        else {
            print("Cannot create polygon using shell lr1")
        }

        if let polygon2 = Polygon(shell: lr2, holes: nil) {
            print("Created polygon using shell lr2")
            print(String(describing: polygon2))
        }
        else {
            print("Cannot create polygon using shell lr2")
        }
    }

Output:

[GEOSwift.Coordinate(x: -100.99095240758299, y: 37.950554483334486), GEOSwift.Coordinate(x: -100.97754136250853, y: 37.92940110061317), GEOSwift.Coordinate(x: -100.99095240758299, y: 37.92940110061317), GEOSwift.Coordinate(x: -100.99095240758299, y: 37.950554483334486)]
-------- LR1 ---------
<GEOSwift.LinearRing: 0x608000484420>
LINESTRING (-100.990952407583 37.95055448333449, -100.9775413625085 37.92940110061317, -100.990952407583 37.92940110061317, -100.990952407583 37.95055448333449)
----------------------
NEW WKT = LINEARRING (-100.990952407583 37.95055448333449, -100.9775413625085 37.92940110061317, -100.990952407583 37.92940110061317, -100.990952407583 37.95055448333449)
-------- LR2 ---------
<GEOSwift.LinearRing: 0x600000284f60>
LINEARRING (-100.990952407583 37.95055448333449, -100.9775413625085 37.92940110061317, -100.990952407583 37.92940110061317, -100.990952407583 37.95055448333449)
----------------------
GEOSwift # %s.
Cannot create polygon using shell lr1
Created polygon using shell lr2
<GEOSwift.Polygon: 0x600000285370>
@vfn
Copy link
Member

vfn commented Apr 10, 2017

@biomiker have you had a chance to look at this issue? Do you think you could fix it and create a PR?

@johncrenshaw
Copy link
Contributor

I just ran into this and came here looking to see if it was a known issue. The problem is that LinearRing creates the underlying geometry using GEOSGeom_createLineString_r(), but GEOSGeom_createPolygon_r() requires that the shell and holes be created using GEOSGeom_createLinearRing_r().

I can confirm that adding a convenience initializer to LinearRing and and using GEOSGeom_createLinearRing_r() in that initializer resolves the problem.

This is what LinearRing looks like with the fix:

open class LinearRing : LineString {
    
    public convenience init?(points: [Coordinate]) {
        let seq = GEOSCoordSeq_create_r(GEOS_HANDLE, UInt32(points.count), 2)
        for (i,coord) in points.enumerated() {
            GEOSCoordSeq_setX_r(GEOS_HANDLE, seq, UInt32(i), coord.x)
            GEOSCoordSeq_setY_r(GEOS_HANDLE, seq, UInt32(i), coord.y)
        }
        guard let GEOSGeom = GEOSGeom_createLinearRing_r(GEOS_HANDLE, seq) else {
            return nil
        }
        self.init(GEOSGeom: GEOSGeom, destroyOnDeinit: true)
    }
    
}

@vfn
Copy link
Member

vfn commented Jun 24, 2017

@johncrenshaw could you create a pull request with this change and tests? Thanks!

johncrenshaw added a commit to johncrenshaw/GEOSwift that referenced this issue Jul 1, 2017
johncrenshaw added a commit to johncrenshaw/GEOSwift that referenced this issue Jul 1, 2017
@vfn
Copy link
Member

vfn commented Jul 3, 2017

@johncrenshaw Thank you for creating the pull request.

I have made a minor change and created a PR on your repo. Please have a look at it, and if you're happy with it, update your PR and we can merge it. Let me know your thoughts

johncrenshaw#1

@vfn
Copy link
Member

vfn commented Jul 4, 2017

Fixed by #50

@vfn vfn closed this as completed Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants