[Breaking] consolidate wrapper and pointer APIs into one wrapper API

[visr]

Upgrade guide

This PR has a few changes that can be breaking. The most important one is that you might get a MethodError that there is no method that accepts a Ptr{Nothing} while this worked before. This can be updated by making sure you work with geometry types rather than pointers. So for instance, instead of buffer(p3.ptr, 0.5), users should remove .ptr; buffer(p3, 0.5). Before some semi-wrapped functions only worked on pointers, this should no longer be the case.

Additionally, the predicate functions for prepared geometries, like prepoverlaps, have been merged into the regular predicates like overlaps. The old prep* still work but are deprecated.

These 3 points made by @yeesian below all apply:

1. There shouldn't be any appearance of ::GEOSGeom in the function arguments
2. There shouldn't be any appearance of .ptr
3. There will cease to be any show of stability for internal functions (prefixed with an underscore) and users should migrate to use the corresponding public version?

Original text

Sorry for the massive PR. I wanted to make a PR similar to yeesian/ArchGDAL.jl#349. By defining methods like:

Base.unsafe_convert(::Type{Ptr{Cvoid}}, x::AbstractGeometry) = x.ptr

We can let ccall know how to handle our geometry wrapper types, in a manner that is GC-safe. I haven't seen the same kind of segfault as for ArchGDAL on nightly, but nevertheless we shouldn't be vulnerable to issues like that. @jw3126 had already started addressing this by adding GC preserve macros to several places. With this approach those are not needed anymore in most places.

Right now LibGEOS.jl has geos_operations.jl with functions for wrapper types, which take out the pointer and call the counterpart function in geos_functions.jl. This essentially folds geos_operations.jl into geos_functions.jl. See for a small sample of this 53e2111. Rather than allowing either pointers or wrapper types, I want to shield the user from pointers as much as possible, so most of geos_functions.jl won't accept them, even if they would work. Realistically that makes this a breaking change, even though I tried to make it not to break too much code.

@jw3126 in line with #143 I tried to use operand context where possible. Some of the tests now break since they call LibGEOS.equals on two geometries with different context. I marked these with the error message "Objects have distinct GEOSContext." Perhaps at least equals works just fine even if they are from a different context? EDIT: this is done now in b42c2be and 27167cb, the tests don't break anymore.

[yeesian]

I think this is a really worthwhile direction. I'm mostly going by the changes in the test files to evaluate the user-facing impact of this change. This repo doesn't have a very high coverage though, so I have a few questions to base the review on:

1. Can we make any definitive statements to distill/summarize this PR? E.g. (i) there shouldn't be any appearance of ::GEOSGeom in the function arguments, (ii) there shouldn't be any appearance of .ptr, and (iii) there will cease to be any show of stability for internal functions (prefixed with an underscore) and users should migrate to use the corresponding public version?
2. Is it possible to describe what are the code changes required for users to migrate from HEAD to the version in this PR?
3. Might it be worthwhile to "merge" geos_functions.jl and geos_operations.jl at some point? (To my understanding, geos_functions.jl was supposed to be the low-level API with pointers and all, and geos_operations.jl was supposed to be the higher-level API defining operations supported by the wrapper types. Given that they're being merged into one wrapper API and types are being used across both files, I think that distinction/organization is no longer a meaningful one.)

[visr]

Thanks for your thoughts.

Can we make any definitive statements to distill/summarize this PR?

I think all 3 points are mostly valid currently, but not 100%. There are still some edge cases like clone being used on pointers in constructors, createPolygon etc. working with coordinate sequences (perhaps coordinate sequeces also need to be wrapped). These edge cases should be isolated and reduced where possible. I'm not sure if that should still be in this PR or a follow up.

Is it possible to describe what are the code changes required for users to migrate from HEAD to the version in this PR?

Yes I can work on such a list.

Might it be worthwhile to "merge" geos_functions.jl and geos_operations.jl at some point?

Yes, geos_operations.jl is now almost empty, I can put the remainder in geos_functions.jl as well. Main reason I didn't do this yet is to keep the diff somewhat readable.

[yeesian]

These edge cases should be isolated and reduced where possible. I'm not sure if that should still be in this PR or a follow up.

Thanks for the explanations! I was just wondering from a tagging of major versions perspective, I think that's fine as a follow-up. Same with the merging of geos_operations.jl and geos_functions.jl.

Yes I can work on such a list.

Thank you, I think that'll be really helpful for users looking to update! We don't have to do a CHANGELOG and having it in the top-level comment of this PR will be good enough.

[visr]

Ok, from my side this is good to go. @yeesian do you mind having another look? I added an upgrade guide to the top post, and still removed geos_operations.jl, and added deprecations for the removed prep exports.

@jw3126 regarding my comment in the top post about contexts, it seems that everything works with mixed contents. So I added b42c2be and 27167cb. Since mixed contents work I just always use the context of the first geometry argument. Please let me know if I'm missing something.

[yeesian]

This is really great, thank you so much!

[jw3126]

@jw3126 regarding my comment in the top post about contexts, it seems that everything works with mixed contents. So I added b42c2be and 27167cb. Since mixed contents work I just always use the context of the first geometry argument. Please let me know if I'm missing something.

I think we had an example of an intersection that crashed with different contexts. I will try to dig it up again.

[jw3126]

I had this in mind. Maybe you modify this test to mix contexts?

[visr]

Thanks for the example. I tried, but couldn't get it to crash. For instance if I use 3 different contexts:

function f91(n)
    # adapted from https://github.com/JuliaGeo/LibGEOS.jl/issues/91#issuecomment-1267732709
    contexts = [LibGEOS.GEOSContext() for i=1:Threads.nthreads()]
    @assert Threads.nthreads() >= 3
    p = [[[-1.,-1],[+1,-1],[+1,+1],[-1,+1],[-1,-1]]]
    Threads.@threads :static for i=1:n
        ctx1 = contexts[1]
        ctx2 = contexts[2]
        ctx3 = contexts[3]
        g1 = LibGEOS.Polygon(p, ctx1)
        g2 = LibGEOS.Polygon(p, ctx2)
        for j=1:n
            @assert LibGEOS.intersects(g1, g2, ctx3)
        end
    end
    GC.gc(true)
    return nothing
end

Though thinking about it I don't see why this would be different from the single threaded case, so perhaps these tests already cover it: b42c2be#diff-ea857d89ed52cb44dabd2f91748dd276a0f5e637479885bda3fcf87c6a26b227

[jw3126]

Awesome that this does not crash anymore! I agree that the single-threaded tests "obviously" cover this. OTOH multi-threading is always good for a WAT, so I think we can add this test anyway?

[visr]

Good point, I added a test in b040fd4.

[jw3126]

Looking again at the test, I am confused, that this works.

    Threads.@threads :static for i=1:n
        ctx1 = contexts[1]
        ctx2 = contexts[2]
        ctx3 = contexts[3]
        g1 = LibGEOS.Polygon(p, ctx1)
        g2 = LibGEOS.Polygon(p, ctx2)
        for j=1:n
            @assert LibGEOS.intersects(g1, g2, ctx3)
        end
    end

For instance the line g1 = LibGEOS.Polygon(p,ctx1) means there could be multiple threads that try to allocate things in ctx1 at the same time.

[jw3126]

BTW a huge thanks for this PR @visr . This is an big and elegant cleanup.

[visr]

Hmm indeed after reading up a bit more in https://libgeos.org/usage/c_api/#reentrantthreadsafe-api and https://libgeos.org/development/rfcs/rfc03/ I understand why that test could be unsafe. I tried to have a look at GEOS tests if they had an example that would definitely fail, but couldn't find it. Therefore I just removed that test for now, people using threading should in general heed this advice from the docs:

Each thread that will be running GEOS operations should create its own context prior to working with the GEOS API.

[jw3126]

LGTM