Skip to content

FT2Font extension improvements #28842

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

Merged
merged 5 commits into from
Oct 24, 2024
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 19, 2024

PR summary

This does two things:

  1. Add more documentation to all of ft2font, and follow numpydoc style.
  2. Convert FreeType enums/flags from "just a bunch of constants" to Enum-like objects. Unfortunately, pybind11 doesn't support Python's new-ish Enum class, but it should be a reasonable approximation. This is based on the implementation in New FreeType wrapper. #9763 and mplcairo, but extended to all the flags as well.
    The Enum change still accepts plain integers, but raises a deprecation warning (as does accessing those constants from the module.) I have not written an API change note in case we don't want to do this. We may also want to mark the deprecation as pending.

PR checklist

@anntzer
Copy link
Contributor

anntzer commented Sep 19, 2024

If you want real Pythonic enums (that actually inherit from enum.Enum) you can reuse the P11X_DECLARE_ENUM macro I wrote in mplcairo.

@QuLogic QuLogic force-pushed the ft2font-improvements branch from 2217d5f to a36f9c0 Compare September 19, 2024 08:12
@QuLogic
Copy link
Member Author

QuLogic commented Sep 19, 2024

I can't say I understand all of that macro, but I can certainly copy it over if that's the way we want to go.

@anntzer
Copy link
Contributor

anntzer commented Sep 19, 2024

(I don't really mind either way, but you mentioned that real enums may be better, so I suggested it. I can also try to explain better how it works...)

@QuLogic QuLogic force-pushed the ft2font-improvements branch from a36f9c0 to ee7082b Compare September 20, 2024 00:57
@QuLogic
Copy link
Member Author

QuLogic commented Sep 20, 2024

The failure in the docs actually shows the confusion with the "just a bunch of constants" approach:

for style in ('Italic',
'Bold',
'Scalable',
'Fixed sizes',
'Fixed width',
'SFNT',
'Horizontal',
'Vertical',
'Kerning',
'Fast glyphs',
'Multiple masters',
'Glyph names',
'External stream'):
bitpos = getattr(ft, style.replace(' ', '_').upper()) - 1
print(f"{style+':':17}", bool(font.style_flags & (1 << bitpos)))

Only Bold and Italic are style flags; the rest are face flags, and checking them as they are here would always return False.

Also, given that the enum values are bitfields already, not bit positions, the math there doesn't make sense.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 20, 2024

(I don't really mind either way, but you mentioned that real enums may be better, so I suggested it. I can also try to explain better how it works...)

On second reading, I have some understanding of it all. It would be nice though if I could add a docstring to each class (as done here), and avoid the C++ enum (but I think I need this one for the variant to produce the deprecation anyway.)

@QuLogic QuLogic force-pushed the ft2font-improvements branch from ee7082b to 082b797 Compare September 20, 2024 09:01
@anntzer
Copy link
Contributor

anntzer commented Sep 20, 2024

I guess to add a docstring you need to do something like p11x::enums[py_name].attr("__doc__") = ...; as there's no way to set it in the function-style API.
The macro is explicitly designed to interface with a C(++) enum but you can probably get away without it if you also remove the entire typecaster machinery?

@QuLogic QuLogic force-pushed the ft2font-improvements branch from 082b797 to 6a3b9fe Compare September 20, 2024 09:15
@QuLogic
Copy link
Member Author

QuLogic commented Sep 20, 2024

I guess to add a docstring you need to do something like p11x::enums[py_name].attr("__doc__") = ...; as there's no way to set it in the function-style API.

Yep, looks like that worked nicely.

The macro is explicitly designed to interface with a C(++) enum but you can probably get away without it if you also remove the entire typecaster machinery?

I started making a copy without the is_enum_v and hardcoding instead of underlying_type_t, but realized I still needed the enum for the deprecation, I think, so gave up on that. Would be nice to de-dupe, but at least the two copies are next to each other now.

@QuLogic QuLogic force-pushed the ft2font-improvements branch from ee46b09 to d3b6c9d Compare September 20, 2024 09:50

The following constants are now part of `.ft2font.Kerning`:

- ``KERNING_DEFAULT``
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps note that the new name doesn't include KERNING_; ditto for LOAD_.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

src/_enums.h Outdated
auto const& [py_base_cls, pairs] =
spec.cast<std::pair<std::string, py::object>>();
mod.attr(py::cast(py_name)) = spec =
py::module::import("pydoc").attr("locate")(py_base_cls)(
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably use pkgutil.resolve_name here instead of the older and undocumented pydoc.locate (though both would work).

Copy link
Member Author

Choose a reason for hiding this comment

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

Or should we just hard-code this coming from enum (and only provide the class name to the macro)?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably fine too, anything works here...

Copy link
Member Author

@QuLogic QuLogic Sep 21, 2024

Choose a reason for hiding this comment

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

OK, I just did the hard-coding then, and added more explicit docs about it.

@QuLogic QuLogic force-pushed the ft2font-improvements branch 2 times, most recently from ed46569 to 016a7b4 Compare September 21, 2024 00:17
@QuLogic
Copy link
Member Author

QuLogic commented Sep 21, 2024

Ah, apparently using #ifdef in macro calls is undefined, and MSVC doesn't accept it. I'll just comment out the new ones until we update FreeType.

@QuLogic QuLogic force-pushed the ft2font-improvements branch from 016a7b4 to 7fd09b7 Compare September 21, 2024 05:46
@QuLogic
Copy link
Member Author

QuLogic commented Sep 21, 2024

If I've understood the remaining failure, MSVC doesn't expand __VA_ARGS__ in the P11X_ENUM_TYPE macro call to be multiple arguments, so everything falls apart after. I worked around that by just manually passing the type, unless you have any better ideas?

@QuLogic QuLogic added the CI: Run cibuildwheel Run wheel building tests on a PR label Sep 21, 2024
@anntzer
Copy link
Contributor

anntzer commented Sep 23, 2024

If I've understood the remaining failure, MSVC doesn't expand __VA_ARGS__ in the P11X_ENUM_TYPE macro call to be multiple arguments, so everything falls apart after. I worked around that by just manually passing the type, unless you have any better ideas?

You likely need to pass /Zc:preprocessor (or the older equivalent /experimental:preprocessor, which is what I use in mplcairo) (https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview?view=msvc-170) in extra_compile_args (or whatever that's called for meson).

@QuLogic
Copy link
Member Author

QuLogic commented Sep 23, 2024

OK, I checked and /experimental:preprocessor should be old enough for us to depend on.

@QuLogic QuLogic force-pushed the ft2font-improvements branch from c7d1d03 to 437dfc0 Compare September 23, 2024 23:04
@QuLogic
Copy link
Member Author

QuLogic commented Sep 24, 2024

So this is failing on AppVeyor because we're still on the 2017 image; I opened #28869 separately to see if we can bump to the 2019 one.

@QuLogic
Copy link
Member Author

QuLogic commented Oct 11, 2024

We're passing now that AppVeyor has been updated.

@tacaswell tacaswell added this to the v3.10.0 milestone Oct 11, 2024
@QuLogic QuLogic force-pushed the ft2font-improvements branch 2 times, most recently from 2096a3a to 69c873d Compare October 16, 2024 08:01
@QuLogic QuLogic force-pushed the ft2font-improvements branch from 69c873d to 200b849 Compare October 16, 2024 20:26
@tacaswell tacaswell force-pushed the ft2font-improvements branch from 200b849 to bc2b853 Compare October 18, 2024 18:20
QuLogic and others added 5 commits October 18, 2024 14:31
Port things to numpydoc, and add additional explanation where possible.
This follows some of the idea from matplotlib#9763, but is based on code from
mplcairo.

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Unfortunately, FreeType doesn't define this as an enum of any sort, just
as a bunch of macro `#define`s. So we have to make our own for pybind11
to bind.
@tacaswell tacaswell force-pushed the ft2font-improvements branch from bc2b853 to 5864017 Compare October 18, 2024 18:35
@tacaswell
Copy link
Member

sorry, took two tries to get the rebase right.

@ksunden ksunden merged commit 1f0ddbe into matplotlib:main Oct 24, 2024
43 of 44 checks passed
@QuLogic QuLogic deleted the ft2font-improvements branch October 24, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants