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

Implement text shaping with libraqm#30000

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
QuLogic merged 6 commits intomatplotlib:text-overhaulfromQuLogic:libraqm
Sep 3, 2025

Conversation

@QuLogic
Copy link
Member

@QuLogicQuLogic commentedMay 2, 2025
edited
Loading

PR summary

This is based on#29816 but doesn't yet include#29794 and#29695. I'm mostly opening it to see how CI will cope.

PR checklist

@QuLogic
Copy link
MemberAuthor

This was useful in pointing out some cross-platform incompatibilities. I may check cibuildwheel as well later to ensure bundling is working correctly.

The failing test is a relatively recent one; I will have to check if that is from here or the FreeType change.

@anntzer
Copy link
Contributor

Do you envision that raqm will become the sole text layout method, or do you plan to keep the old manual layouting around (perhaps togglable)? I just noticed that the old layouting method appears to be missing handling of lsb_delta and rsb_delta (see note athttps://freetype.org/freetype2/docs/reference/ft2-glyph_retrieval.html#ft_glyphslotrec ("If you use strong auto-hinting, you must apply these delta values! Otherwise you will experience far too large inter-glyph spacing at small rendering sizes in most cases.")) but raqm would likely (hopefully?) take care of that for us. If the manual layouting is going to stay then I'll open a separate issue to implement this (perhaps also to be folded into the FreeType update mega-PR); if not, perhaps we can just forget about it.

@QuLogic
Copy link
MemberAuthor

I intend for it to replace everything, I think. I have some in-progress work to apply it to PDF/PS/SVG as well.

I did notice thelsb_delta/rsb_delta note and tried to implement it in#29816, though IIRC not much (or even nothing) changed, but I could be misremembering.

anntzer reacted with thumbs up emoji

@anntzer
Copy link
Contributor

anntzer commentedMay 21, 2025
edited
Loading

Also, does _text_helpers.layout() also need to be rewritten to use raqm as well? (likely it would need to also include y information now)
I had in fact more or less written that function as a "poor man's raqm", IIRC...

A nice endpoint would be if the new _text_helpers.LayoutItem, by supporting both x and y, was general enough that draw_text and draw_mathtext (and even draw_tex) could ultimately just all pass a list of LayoutItems (+ rects, in the mathtext/tex case) down to a single glyph rendering method.

@QuLogic
Copy link
MemberAuthor

QuLogic commentedMay 24, 2025
edited
Loading

OK, I've figured out why I was running into issues before and pushed the vector implementation up now. The issues aren't fixed, but at least in a position to be discussed.

Specifically,_text_helpers.layout used to useglyph.linearHoriAdvance which is a 16.16 fixed-point value, but libraqm returns advances in 26.6 fixed-point values. For example, the string "center Tj" has 16.16 advances:

[3603200, 4032000, 4153600, 2569600, 4032000, 2694400, 2083200, 4003200, 1820800]

which works out to:

>>> orig / 65536array([54.98046875, 61.5234375 , 63.37890625, 39.20898438, 61.5234375 ,       41.11328125, 31.78710938, 61.08398438, 27.78320312])

But the libraqm 26.6 advances are (equivalent to the linear advances divided by 1024 and rounded to integers):

[3519, 3938, 4056, 2509, 3938, 2631, 2034, 3909, 1778]

which works out to:

>>> raqm / 64array([54.984375, 61.53125 , 63.375   , 39.203125, 61.53125 , 41.109375,       31.78125 , 61.078125, 27.78125 ])

or the small difference of:

[0.00390625, 0.0078125, -0.00390625, -0.00585938, 0.0078125, -0.00390625, -0.00585938, -0.00585938, -0.00195312]

Unfortunately, this touches almost every single SVG in a way that they all fail, but invisibly.

I'm not sure if we should attempt to work around this (by scaling DPI by 1024, or adding a 1024 scalingFT_Matrix, or something else), or accept this change (but then maybe we'd want to roll that into#29816's big test image update).

@anntzer
Copy link
Contributor

[I guess what's really stored in the font file are advances in font design units and then FreeType scales them by (font design units->real space) and then by 2^16 whereas raqm (really, harfbuzz) uses 2^6 for that last scaling, so probably neither of them is really right or wrong?]

If the breakages are only in svg files then I guess this isn't really a problem because it's just a few numbers (the glyph x-positions) that will change and so the diff will not actually be so big? (If you check by git diff rather than the github UI or similar it would just be a few small changes?)

@QuLogicQuLogic changed the base branch frommain totext-overhaulJune 5, 2025 01:39
@QuLogicQuLogic moved this toWaiting for other PR inFont and text overhaulJun 5, 2025
@tacaswell
Copy link
Member

The failure on 3.12 on ubuntu is the c-coverage tool. I have a vague memory of fixing this before, but none of the details of how we fixed it?

@QuLogic
Copy link
MemberAuthor

Rebased on latesttext-overhaul branch which merged the latestmain, so hopefully we'll see if the test coverage is working again.

@QuLogicQuLogic added the CI: Run cibuildwheelRun wheel building tests on a PR labelAug 22, 2025
@tacaswell
Copy link
Member

the appveyor failures look like we are doing some funny git-work there....

@QuLogic
Copy link
MemberAuthor

Rebased on latesttext-overhaul branch which merged the latestmain, so hopefully we'll see if the test coverage is working again.

Unfortunately, it's still a problem, but I've figured out that it's specifically about Harfbuzz and that I can reproduce it locally. I'm not sure what is buggy to cause the problem with only a few files, but as we don't care about subproject code coverage, I'll see if it can be disabled.

the appveyor failures look like we are doing some funny git-work there....

Yes, this was set up in#30231 /#30274 though I'm not sure why it's complaining on AppVeyor only.

throwstd::runtime_error("failed to set text face for layout");
}
for (auto [cluster, face] : face_substitutions) {
raqm_set_freetype_face_range(rq, face, cluster,1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the last argument may need to be greater than 1, because the cluster may correspond to more than a single byte on the bytestring. I guess what needs to be done is that when you recordface_substitutions.emplace_back(rglyph.cluster, fallback->face) you need actually apply the fallback face onto the entire span fromrq_glyphs[i].cluster torq_glyphs[i+1].cluster?

Adding test for font fallback may be enough to check that.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This turns out to be a good point. I have implemented your suggestion for now, but unfortunately, I'm not certain that libraqm exposes enough information to correctly implement things yet.

Specifically, if you have mixed LTR and RTL text (as in the existing complex layout test with textArabic: رَقْم), then glyphs are returned in LTRdisplay order. So you get glyphs for clusters 0, 1, 2, 3, 4, 5, 6, 7, 12, 10, 10, 8, 8. The run from 0-7 is in English and 8-12 in Arabic, but we get the latter in reverse order. In some cases, multiple glyphs are used for a single cluster and other times a single one for multiple indices.

Usingrq_glyphs[i].cluster torq_glyphs[i+1].cluster (with a fallback to the end of the string) happens to work because in that RTL section, we get the lowest index last and so set fallback on all indices above it even though we didn't "know" about the other in-between indices. So I think that somewhere in that run, if some character did exist, we might accidentally switch it to the fallback, but I haven't come up with a good way to test that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point; perhaps that can be fixed by doing something along the lines of

cluster_starts=sorted(rq_glyph.clusterforrq_glyphinrq_glyphs)span_end=cluster_starts[searchsorted(cluster_starts,span_start)+1]

?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes, doing some kind of sorting does seem to work as expected (at least based on my debugging), though I haven't thought up a good test to be sure there aren't any mixups. I've extended the test to include multiple fallbacks as well, just to be sure that is working.

@anntzer
Copy link
Contributor

I have some doubts about the correctness of the font fallback implementation (per the review above), which can probably be addressed by a test; other than that, looks good to me 👍

Latest Meson (1.9.0 at this time) is now erroring because of theHarfbuzz contains a differently-named wrap file that provides`freetype2`. This may be a bug upstream, but we really don't need theversion in the filename.
Also add some missing license entries in more places.
@QuLogic
Copy link
MemberAuthor

I've added an additional commit at the beginning to renamefreetype-2.13.3.wrap tofreetype2.wrap because Meson 1.9.0 seems to find some conflict with Harfbuzz'sfreetype2.wrap This may be a bug upstream, but we don't really need the version in the name (it made some sense with FreeType 2.6.1 because that was a custom copy, but thisfreetype2.wrap is from Meson's wrapdb.)

Then also corrected the fallback code for multi-codepoint clusters as pointed out by@anntzer.

@QuLogicQuLogicforce-pushed thelibraqm branch 2 times, most recently from146bd6b to25f50acCompareAugust 26, 2025 22:00
Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

A few minor things left only. Feel free to self-merge with or without.

std::unique_ptr<std::remove_pointer_t<raqm_t>, decltype(&raqm_destroy)>(
rq, raqm_destroy);

if (!raqm_set_text(rq, reinterpret_cast<const uint32_t *>(text.data()),
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to be done here (indeed, a separate PR may keep things more self-contained), but maybe PyFT2Font_set_text could change the type oftext to std::string (utf8, per pybind11) and you can use raqm_set_text_utf8 here? Mostly for conciseness; but I also suspect that most inputs are not internally represented as 4byte-wide PyUNICODEs so conversion to utf32 is required by the current code anyways.

We only care about our own source files, so anything in `subprojects` orthe `build` directory can be ignored, thus fixing the bugginess withHarfbuzz headers.
@QuLogicQuLogic merged commit04c8eef intomatplotlib:text-overhaulSep 3, 2025
46 of 47 checks passed
@github-project-automationgithub-project-automationbot moved this fromReady for Review toDone inFont and text overhaulSep 3, 2025
@QuLogicQuLogic deleted the libraqm branchSeptember 3, 2025 01:26
@QuLogicQuLogic mentioned this pull requestSep 26, 2025
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@anntzeranntzeranntzer approved these changes

@tacaswelltacaswelltacaswell approved these changes

Assignees

No one assigned

Projects

Status: Done

Milestone

v3.11.0

3 participants

@QuLogic@anntzer@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp