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

Allow passing capsules for draw callbacks #130

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Allow passing capsules for draw callbacks #130

merged 2 commits into from
Jul 29, 2022

Conversation

khaledhosny
Copy link
Collaborator

This allow defining the callbacks in C modules and passing the pointers as Python objects.

@behdad
Copy link
Member

behdad commented Jul 26, 2022

This is neat.

This allow defining the callbacks in C modules and passing the pointers
as Python objects.
@justvanrossum
Copy link
Collaborator

Should user_data also be allowed to be a capsule?

@khaledhosny
Copy link
Collaborator Author

Should user_data also be allowed to be a capsule?

I wondered about that too, but it seems to work without a change. I guess because user_data is accessed from the callback functions, so that is the part the needs to deal with whether it is a capsule or a another Python object.

@justvanrossum
Copy link
Collaborator

I'm just worried that the callback code will have to deal with unpacking the capsule. If it requires a Python C API call that would be unfortunate.

@khaledhosny
Copy link
Collaborator Author

Good point.

@justvanrossum
Copy link
Collaborator

Can capsules interoperate with ctypes pointers? It's a bit unclear to me how compatible the two are.

@khaledhosny
Copy link
Collaborator Author

Can capsules interoperate with ctypes pointers? It's a bit unclear to me how compatible the two are.

I think they should work, PyCapsule is essentially wrapping an integer in a Python object. The problem, I think, will be that there is no Python API for dealing with PyCapsule’s, only a C-API.

@anthrotype
Copy link
Member

I think ctypes.pythonapi exposes the Python API, including PyCapsule, not sure if that's what you're after.
I do think cython is neater than ctypes in general.

@anthrotype
Copy link
Member

found this on https://stackoverflow.com/a/65057267

@khaledhosny khaledhosny marked this pull request as ready for review July 29, 2022 03:59
Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and works well in FontGoggles. Some tests would be nice, but we can do that later.

@khaledhosny khaledhosny merged commit 90e138e into main Jul 29, 2022
@khaledhosny khaledhosny deleted the capsule branch July 29, 2022 13:35
@khaledhosny
Copy link
Collaborator Author

Testing would be good indeed, but I was hesitant to look into it because it will need some compiled native code and I have no idea how to pull that for tsets.

@justvanrossum
Copy link
Collaborator

justvanrossum commented Jul 29, 2022

Could it be faked done with some cdefs in the main .pyx source?

@khaledhosny
Copy link
Collaborator Author

I don’t know, I’m apparently not a Cython expert. @anthrotype probably has some ideas.

@anthrotype
Copy link
Member

maybe you could define some dummy move_to, line_to etc. methods inside the main .pyx module (or a new extension module that's only for testing but that complicates the setup), then from inside the pure-python test module you use ctypes to ctypes.cdll.LoadLibrary the uharfbuzz extension module itself in the same way FontGoggles loads its own objc dylib (an extension module is like a shared library, I think) and then use ctypes to make the capsules and pass them on to uharfbuzz DrawFuncs..

@anthrotype
Copy link
Member

I tried just for fun to load the uharfbuzz extension via ctypes, looks like it works

In [1]: import ctypes
In [2]: import uharfbuzz._harfbuzz
In [3]: uharfbuzz._harfbuzz.__file__
Out[3]: '/Users/clupo/Github/nanoemoji/.venv/lib/python3.10/site-packages/uharfbuzz/_harfbuzz.cpython-310-darwin.so'
In [4]: lib = ctypes.cdll.LoadLibrary(uharfbuzz._harfbuzz.__file__)
In [5]: hb_version_string = lib.hb_version_string
In [6]: hb_version_string.restype = ctypes.c_char_p
In [7]: hb_version_string.argtypes = ()
In [8]: hb_version_string()
Out[8]: b'5.0.0'

I think the dummy move_to, line_to, etc. for testing should be defined as cdef methods (so they are only visible from C)

@khaledhosny
Copy link
Collaborator Author

Thanks, I’ll give this a try.

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.

4 participants