Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ksunden merged 5 commits intomatplotlib:mainfromQuLogic:ft2font-improvements
Oct 24, 2024

Conversation

QuLogic
Copy link
Member

@QuLogicQuLogic commentedSep 19, 2024
edited
Loading

PR summary

This does two things:

  1. Add more documentation to all offt2font, and follow numpydoc style.
  2. Convert FreeType enums/flags from "just a bunch of constants" toEnum-like objects.Unfortunately,pybind11 doesn't support Python's new-ishEnum class, but it should be a reasonable approximation. This is based on the implementation inNew FreeType wrapper. #9763 and mplcairo, but extended to all the flags as well.
    TheEnum 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

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
Copy link
MemberAuthor

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

(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
Copy link
MemberAuthor

QuLogic commentedSep 20, 2024
edited
Loading

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

forstylein ('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
MemberAuthor

(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 thevariant to produce the deprecation anyway.)

@anntzer
Copy link
Contributor

I guess to add a docstring you need to do something likep11x::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
Copy link
MemberAuthor

I guess to add a docstring you need to do something likep11x::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 theis_enum_v and hardcoding instead ofunderlying_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.


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
MemberAuthor

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
MemberAuthor

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 fromenum (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
MemberAuthor

@QuLogicQuLogicSep 21, 2024
edited
Loading

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.

@QuLogicQuLogicforce-pushed theft2font-improvements branch 2 times, most recently fromed46569 to016a7b4CompareSeptember 21, 2024 00:17
@QuLogic
Copy link
MemberAuthor

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
Copy link
MemberAuthor

If I've understood the remaining failure, MSVC doesn't expand__VA_ARGS__ in theP11X_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?

@QuLogicQuLogic added the CI: Run cibuildwheelRun wheel building tests on a PR labelSep 21, 2024
@anntzer
Copy link
Contributor

If I've understood the remaining failure, MSVC doesn't expand__VA_ARGS__ in theP11X_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
MemberAuthor

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

@QuLogic
Copy link
MemberAuthor

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
MemberAuthor

We're passing now that AppVeyor has been updated.

QuLogicand others added5 commitsOctober 18, 2024 14:31
Port things to numpydoc, and add additional explanation where possible.
This follows some of the idea frommatplotlib#9763, but is based on code frommplcairo.Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Unfortunately, FreeType doesn't define this as an enum of any sort, justas a bunch of macro `#define`s. So we have to make our own for pybind11to bind.
@tacaswell
Copy link
Member

sorry, took two tries to get the rebase right.

@ksundenksunden merged commit1f0ddbe intomatplotlib:mainOct 24, 2024
43 of 44 checks passed
@QuLogicQuLogic deleted the ft2font-improvements branchOctober 24, 2024 19:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@tacaswelltacaswelltacaswell approved these changes

@greglucasgreglucasgreglucas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

5 participants
@QuLogic@anntzer@tacaswell@greglucas@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp