Convert ft2font extension to pybind11 #28785
Conversation
e0fe354 to
1b39e89
Compare
|
I do not understand the remaining stubtest failures; I'm also not sure what to do about |
1b39e89 to
b3e6500
Compare
|
Okay, so on current main, The methods which return a Glyph do work, and return something that the repr says it should be at This PR changes that such that the Python class does exist. As for the metaclass, my guess is that that is effectively the same problem. My leaning would be go ahead and add a constructor, it doesn't hurt anything and may solve these issues. That or see if you can omit Glyph from the PYMODULE and it just works. I'm not sure why stubtest was not erroring about the class not existing at runtime previously, but it wasn't. The type hints see no |
408b128 to
2079fc8
Compare
|
For For the metaclass, I added |
2079fc8 to
b48c76e
Compare
b48c76e to
2d6d2a8
Compare
Inline `convert_xys_to_array` and modify the arguments to take a C++ container, so we don't need a less-safe pointer, and we don't need to copy another time over.
This allows pybind11 to generate the type hints for us, and it also takes care of checking the list and its contents are the right type.
The former is not available on the macOS deployment target we use for wheels. We could revert back to `PyOS_snprintf`, but C++11 contains `snprintf`, and it seems to guarantee the same things.
And also ignore the `numpy.float64` reference. The latter seems to be broken since Sphinx tries to auto-link type hints as `py:class`, but it's an alias in NumPy making it a `py:attr` in their inventory.
2d6d2a8 to
a0649e7
Compare
Reverted that, since it causes weird results in the docs. Now |
| @@ -232,23 +215,73 @@ class FT2Font: | |||
| def set_text( | |||
| self, string: str, angle: float = ..., flags: int = ... | |||
| ) -> NDArray[np.float64]: ... | |||
There was a problem hiding this comment.
I'm wondering if np.double is the correct return type?
https://numpy.org/doc/stable/reference/arrays.scalars.html#numpy.double
From reading the documentation it seems like float64 is an alias for double on some (probably most though) platforms.
(Not added in the PR, but the missing reference is...)
There was a problem hiding this comment.
Good thought, but unfortunately it doesn't fix the missing reference. It appears that Sphinx doesn't read any .pyi files (which is why most other pages don't have type hint issues), but in this case is seeing something that pybind11 is generating in the runtime itself.
|
I was going to say that we shouldn't need |
| } | ||
|
|
||
| PyFT2Font *self = new PyFT2Font(); | ||
| self->x = NULL; |
There was a problem hiding this comment.
Ideally we'd use the C++ nullptr instead of NULL, but as there are a number of NULLs already in this file let's leave it consistent with that.
|
Merging as there are 2 approvals. I intend to make time to review #27011 sometime in the next week. |
|
My understanding is that @QuLogic has some follow on work to finish removing our build-time numpy dependency all together. |
PR summary
Unlike #9763, this is not a full rewrite, but more of a refactor. There may be further cleanup possible in the future, but this is somewhat large enough already.
PR checklist