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

Upgrade to Clipper2 #50

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

ferrolho
Copy link
Collaborator

I am opening this draft PR to make Clipper2 available in Julia (and as a means of getting feedback). Once ready, this PR should resolve #49.

In a previous discussion with @SimonDanisch on Slack (here), we agreed to tag a new version in the existing Clipper.jl repository, rather than starting a new repository/package — so that's what I did.

The current status is that a Clipper2_jll for Clipper2 v1.2.2 has been built using BinaryBuilder.jl, and it is now available (see JuliaPackaging/Yggdrasil#7093). But that is pretty much it so far, though.

@ferrolho
Copy link
Collaborator Author

ferrolho commented Aug 27, 2023

So, now that the PR is open, I can start asking questions. Just a quick disclaimer: this is my first time working on wrapping a library. As I mentioned above, Clipper2_jll is now available, so I have started using it in the PR. I am now trying to figure out what the next steps should be.

My attempt at the immediate next step was to check if I am able to call a function, e.g., Clipper2Lib::Area. However, this is what I get:

julia> using Clipper2_jll

julia> @ccall libClipper2.Area()::Float32
ERROR: could not load symbol "Area":
dlsym(0x9be6cd80, area): symbol not found
Stacktrace:
 [1] top-level scope
   @ ./REPL[2]:1

Running nm -gU /Users/henrique/.julia/artifacts/b2f80988615b762a0db5a74dbc8a5c94d752f255/lib/libClipper2.1.2.2.dylib does show me a symbol that contains Area as a substring:

(...)
00000000000059c0 T __ZN11Clipper2Lib4AreaEPNS_5OutPtE
(...)

but I realised that ccall is meant only for C code, not C++ (Julia / Calling C and Fortran Code). That docs page also says at the end: "For tools to create C++ bindings, see the CxxWrap package." — so maybe we could use that?

However, from looking at the Yggdrasil build for Clipper v1 (here), it seems that another approach was taken: the C++ methods were wrapped in C, and then Julia calls the C wrapper methods.

@ferrolho
Copy link
Collaborator Author

ferrolho commented Aug 27, 2023

Okay, I've made a tiny bit of progress. From a Julia script, I was able to load the dynamic library and find the symbol for the Area static function. I had to use the mangled name _ZN11Clipper2Lib4AreaEPNS_5OutPtE; I don't know why the demangled name doesn't work.

Here is my test script:

using Libdl

# Define the Point64 type
# http://www.angusj.com/clipper2/Docs/Units/Clipper/Types/Point64.htm
struct Point64
    X::Cint
    Y::Cint
end

# Load the dynamic library
dll_handle = dlopen("/Users/henrique/.julia/artifacts/b2f80988615b762a0db5a74dbc8a5c94d752f255/lib/libClipper2.1.2.2.dylib")

# Retrieve the `Area` static function symbol
const AreaFunction = dlsym(dll_handle, :_ZN11Clipper2Lib4AreaEPNS_5OutPtE)

# `Area` function wrapper
function area(path::Vector{Point64})
    return ccall(AreaFunction, Cdouble, (Ptr{Point64}, Csize_t), path, length(path))
end

# Create a path
path = [Point64(100, 50), Point64(10, 79), Point64(65, 2)]

# Calculate the area of that path
result = area(path)

# Close the dynamic library
dlclose(dll_handle)

Currently, this script crashes at result = area(path) with the following error:

[67564] signal (11.2): Segmentation fault: 11
in expression starting at /Users/henrique/Documents/test-lib/use_area.jl:37
_ZN11Clipper2Lib4AreaEPNS_5OutPtE at /Users/henrique/.julia/artifacts/b2f80988615b762a0db5a74dbc8a5c94d752f255/lib/libClipper2.1.2.2.dylib (unknown line)
area at /Users/henrique/Documents/test-lib/use_area.jl:28
Allocations: 3001 (Pool: 2989; Big: 12); GC: 0
[1]    67563 segmentation fault  julia use_area.jl

I also had a look at Cxx.jl, but I am not planning to use it because of:

Please, note that Cxx.jl only works (out of the box) currently with Julia 1.1.x to 1.3.x, i.e. with no currently supported Julia, while those versions can still be downloaded at Julialang.org.

CxxWrap.jl seems like a better alternative anyway, but it also does seem like it would require a fair amount of work.

At this point, I don't know if it would be better to use CxxWrap.jl or to just write a custom C wrapper similar to the cclipper.cpp for Clipper1. I spent some time trying to give the latter option a go, but I couldn't get BinaryBuilder.jl to build stuff locally with julia build_tarballs.jl --deploy=local --verbose.

Oh, and I almost forgot to mention that another option could be to just use this already-existing C wrapper: https://github.com/geoffder/clipper2c.

@jebej
Copy link

jebej commented Sep 10, 2023

The simplest way is likely to use the already-existing C wrapper. This is what the Golang wrapper does.

@ferrolho
Copy link
Collaborator Author

ferrolho commented Sep 13, 2023

The simplest way is likely to use the already-existing C wrapper. This is what the Golang wrapper does.

Are you referring to https://github.com/geoffder/clipper2c? After I had posted my messages above, I also found that the author provided a clipper.export.h which seems to export an interface in C that maybe will work.

@ViralBShah
Copy link
Contributor

@ferrolho I just noticed this discussion. Would it be useful for you to have commit access to this repo (if you don't already)?

@ferrolho
Copy link
Collaborator Author

@ferrolho I just noticed this discussion. Would it be useful for you to have commit access to this repo (if you don't already)?

That would be helpful if I resume working on this, but I haven't done it for over a year now. 😬

@ViralBShah
Copy link
Contributor

@ferrolho I just noticed this discussion. Would it be useful for you to have commit access to this repo (if you don't already)?

That would be helpful if I resume working on this, but I haven't done it for over a year now. 😬

Yeah, I realized this is an old thread. Anyways, I invited you for commit access.

@ferrolho
Copy link
Collaborator Author

Thanks! I've accepted the invite.

@ViralBShah ViralBShah marked this pull request as ready for review November 15, 2024 14:33
@ViralBShah ViralBShah marked this pull request as draft November 15, 2024 14:33
@ViralBShah
Copy link
Contributor

ViralBShah commented Nov 15, 2024

Just leaving this here from the updated CI etc. This PR needs to update the API:

Error running: Test Add path to clipper
ERROR: LoadError: could not load symbol "get_clipper":
/home/runner/.julia/artifacts/6412bf9f1ed445d3726be2ea874b3e9e3719826a/lib/libClipper2.so: undefined symbol: get_clipper

If there is a C Wrapper, we should try go the clang.jl route.

Package version should only be bumped to the next minor version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clipper2
3 participants