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

Drop the FT2Font intermediate buffer.#30059

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

Open
anntzer wants to merge1 commit intomatplotlib:text-overhaul
base:text-overhaul
Choose a base branch
Loading
fromanntzer:ft-direct-render

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedMay 16, 2025
edited
Loading

Directly render FT glyphs to the Agg buffer. In particular, this naturally provides, with no extra work, subpixel positioning of glyphs (closing#29551) (this could also have been implemented in the old framework, but would have required careful tracking of subpixel offets).

Note that rendering of mathtext boxes (e.g., fraction bars) is currently missing, but should be "easy" (the main question being whether/how to ensure proper hinting/alignment to pixel edges in the horizontal or vertical output cases). Likewise, all baseline images should be regenerated. Finally, the new APIs added to FT2Font are likely up to bikeshedding (but they are all private).

In practice this should get included (if we agree to go this way) into the FreeType update (#29816). This would also supersede the patch advertised at#29816 (comment).

test modified from#29551 to also include mathtext:

importmatplotlib.pyplotaspltimportmatplotlib.animationasmanimfig=plt.figure(figsize=(2,2))ts= [fig.text(0,0,'$ABC123$'),fig.text(0,0.2,'ABC123'),]change=0.001defupdate(*args,**kwargs):globalchangefortints:x,y=t.get_position()ifnot (0<=x<=1and0<=y<=1):change*=-1t.set_x(x+change)t.set_y(y+change)returntsani=manim.FuncAnimation(fig,update,interval=20,frames=int(2/change),cache_frame_data=False)ani.save('text.gif')

PR summary

PR checklist

@story645
Copy link
Member

story645 commentedMay 16, 2025
edited
Loading

Would you be open to using a tracking issue or project board for the font work (sample categories: draft, waiting on other PRs, ready for review, please review first)?

I think breaking everything up into small PRs is fantastic but I'm finding it a bit overwhelming to keep track of the order for reviewing things (and figure I'm not alone here) and how these PRs relate to each other.

@anntzer
Copy link
ContributorAuthor

anntzer commentedMay 16, 2025
edited
Loading

Sure, I'll do that. I'm sorry for the mess, but this is basically me trying to remember all the patches or even just ideas I have accumulated over the years that went nowhere because they would thrash all the baseline images, and that I now try to present given that I see an opportunity with the FreeType upgrade. Also, some of them are notfully complete patches but things that I believe can be made complete with relatively little effort (for some value of "little effort"...) simply because I do not have the time now to finalize them but want to put them up for discussion as the underlying idea should be clear.

Edit: seethe wiki; attn@QuLogic as well too.

story645 reacted with heart emoji

@story645
Copy link
Member

story645 commentedMay 16, 2025
edited
Loading

I'm sorry for the mess, but

No apologies needed, I commend you for taking advantage of the freetype upgrade. And thanks for the wiki, though some reason can't link cleanly☹️

@anntzer
Copy link
ContributorAuthor

link fixed

story645 reacted with thumbs up emoji

@anntzeranntzerforce-pushed theft-direct-render branch 3 times, most recently fromd44360f to66fdae0CompareMay 17, 2025 09:55
@anntzer
Copy link
ContributorAuthor

Edit: fraction bar rendering has been added too.

@QuLogic
Copy link
Member

A few warnings from compiling locally:

[2/4] Compiling C++ object src/ft2font.cpython-313-x86_64-linux-gnu.so.p/ft2font_wrapper.cpp.o../../src/ft2font_wrapper.cpp: In lambda function:../../src/ft2font_wrapper.cpp:1785:22: warning: unused variable ‘error’ [-Wunused-variable] 1785 |             if (auto error = FT_Load_Glyph(face, idx, static_cast<FT_Int32>(flags))) {      |                      ^~~~~../../src/ft2font_wrapper.cpp:1788:22: warning: unused variable ‘error’ [-Wunused-variable] 1788 |             if (auto error = FT_Render_Glyph(face->glyph, FT_RENDER_MODE_NORMAL)) {      |                      ^~~~~../../src/ft2font_wrapper.cpp: In lambda function:../../src/ft2font_wrapper.cpp:1804:26: warning: unused variable ‘error’ [-Wunused-variable] 1804 |                 if (auto error = FT_Glyph_To_Bitmap(&g, FT_RENDER_MODE_NORMAL, &origin, 1)) {      |                          ^~~~~

@anntzer
Copy link
ContributorAuthor

This is semi-intentional: if these checks fail (error > 0) then the error should really be raised using throw_ft_error, not with a plain raise as I currently do, but throw_ft_error is not visible from ft2font_wrapper.cpp so some code needs to be moved around, e.g. make throw_ft_error an inline function defined in ft2font.h, or move the new implementations into real C++ FT2Font methods in ft2font.cpp and add wrapper lambdas in ft2font_wrapper.cpp, or (would be my preferred choice) switch error checking to the FT_CHECK macro used in mplcairo (see mplcairo's macros.h).
I left the warning as is just to keep this as a side discussion point, but it can trivially be suppressed by not capturing the error value too.

@QuLogic
Copy link
Member

On a side note, I'm in favour of this in general as removing the intermediate buffer will make emoji/colour glyphs easier since they won't need to go through it.

anntzer reacted with thumbs up emoji

@QuLogic
Copy link
Member

On the call, we were discussing whether it made sense to target all these font-related (and requiring full image rebuild) PRs to a separate feature branch, which would contain the final test image updates. Do you have any thoughts on retargetting this PR (instead of folding it in to#29816)?

@anntzer
Copy link
ContributorAuthor

anntzer commentedMay 30, 2025
edited
Loading

On the call, we were discussing whether it made sense to target all these font-related (and requiring full image rebuild) PRs to a separate feature branch, which would contain the final test image updates. Do you have any thoughts on retargetting this PR (instead of folding it in to#29816)?

Sure, I don't really mind either way? Should the branch live on this repo and be ultimately pruned out, or on yours?

@anntzer
Copy link
ContributorAuthor

Fixed#30059 (comment) by using FT_CHECK (thus also needs#30123).

@QuLogicQuLogic changed the base branch frommain totext-overhaulJune 5, 2025 01:47
@QuLogicQuLogic added this to thev3.11.0 milestoneJun 5, 2025
@anntzer
Copy link
ContributorAuthor

anntzer commentedJun 10, 2025
edited
Loading

I didn't look carefully but I believe I can just get rid of the changes in#29199 when rebasing because my PR essentially already does the same thing, correct? (no problem with doing the rebase, of course)

@QuLogic
Copy link
Member

I think so; it just fixes rotation, which you should be doing already.

@anntzer
Copy link
ContributorAuthor

anntzer commentedJun 11, 2025
edited
Loading

Actually, brutally rebasing on top of#29199 and rerunning the new test_rotation_mode_anchor suggests that there may be slight off-by-1 or so errors in positioning with the new code (nothing huge like what#29199 fixed, but still some slight asymmetry remaining between the rotated text strings). To be investigated...
I won't push the rebase for now just to keep a bookmark on the remaining issue (but feel free to do so if you want to move forward first).

@QuLogic
Copy link
Member

Yes, I did see that, but wasn't too sure it was wrong or not. Please rebase on thetext-overhaul branch for future updates (though I don't know if there'll be much change there right now.)

@anntzer
Copy link
ContributorAuthor

I convinced myself that the shift is not obviously wrong (that test uses center_baseline alignment; with e.g. baseline alignment I don't see such a shift) so I went ahead and gen'd the baseline images.

Unrelatedly, I noticed that a large number of baseline image changes e.g. in test_axes could have been avoided if they used remove_text=True (it's ticklabels on tests that have nothing to do with ticklabels); perhaps many of them could get that option set?

@QuLogic
Copy link
Member

Unrelatedly, I noticed that a large number of baseline image changes e.g. in test_axes could have been avoided if they used remove_text=True (it's ticklabels on tests that have nothing to do with ticklabels); perhaps many of them could get that option set?

If you can put together a list, then please open a PR; I did similar for, e.g., layout tests:#29872

parse=self.mathtext_parser.parse(
s,self.dpi,prop,antialiased=gc.get_antialiased())
c=cos(radians(angle))
s=sin(radians(angle))
Copy link
Member

Choose a reason for hiding this comment

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

There's already ans as input to the function, so this could be a bit confusing (also type checking probably wouldn't like it.)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

@QuLogic
Copy link
Member

BTW, not sure how you're updating the images, but I tend to start with the images fromthetext-overhaul-figures branch, update the results and then only commitwhat's different from thetext_overhaul-figures branch. This usually ends up being much fewer images, though I don't know if that's the case here.

@anntzer
Copy link
ContributorAuthor

Unrelatedly, I noticed that a large number of baseline image changes e.g. in test_axes could have been avoided if they used remove_text=True (it's ticklabels on tests that have nothing to do with ticklabels); perhaps many of them could get that option set?

If you can put together a list, then please open a PR; I did similar for, e.g., layout tests:#29872

I think the following tests can have their text removed fairly uncontroversially (so there's actually not so many of them):

test_agg_filteragg_filter_alphaagg_filter_alpha_giftest_axesimshow_clipmarkeverymarkevery_lineo_marker_path_snapoffset_points (replace text by arrow)rc_markerfillvline_hline_zordertest_axes3derrorbar3dvoxels-xyztest_collectionscap_and_joinstyletest_imageimage_clipimage_cliprecttest_linesscaled_linestest_patchesannulusclip_to_bboxtest_pngpngsuitetest_triangulationtripcolor1

(putting the list here but I won't work on the PR at least for now)

BTW, not sure how you're updating the images, but I tend to start with the images fromthe text-overhaul-figures branch, update the results and then only commit what's different from the text_overhaul-figures branch. This usually ends up being much fewer images, though I don't know if that's the case here.

I got a bit confused on how to do that (I cherry-picked text-overhaul-figures tip then committed the extra differences, but then trying to rebase out the cherry-picked commit results in conflicts when trying to apply the extra commit.
If the patch here looks good to you, can I leave the baseline image handling to you?

@QuLogic
Copy link
Member

BTW, not sure how you're updating the images, but I tend to start with the images fromthe text-overhaul-figures branch, update the results and then only commit what's different from the text_overhaul-figures branch. This usually ends up being much fewer images, though I don't know if that's the case here.

I got a bit confused on how to do that (I cherry-picked text-overhaul-figures tip then committed the extra differences, but then trying to rebase out the cherry-picked commit results in conflicts when trying to apply the extra commit.

What you have to do is checkout the updated images, triage the new images in, reset and add the new ones. Something like this:

$ git checkout text-overhaul-figures -- lib/matplotlib/tests/baseline_images lib/mpl_toolkits/*/tests/baseline_images# run tests# update images with `triage_tests.py`$ git diff --name-only > actual-changed-images  # Save list of modified images.$ git reset  # Don't commit the other changed images.$ git add $(cat actual-changed-images)  # Add the actual modified images to commit$ git commit$ git checkout -- .  # Throw away everything else.

If the patch here looks good to you, can I leave the baseline image handling to you?

I haven't fully reviewed the patch, but I did rebase it for conflicts, so please verify that wasn't messed up. I uploaded the images as well, assuming I haven't messed that up.

@anntzer
Copy link
ContributorAuthor

anntzer commentedSep 25, 2025
edited
Loading

I checked the rebase, it looks good to me (thanks for doing it). One last thing missing from this is a proper antialiasing toggle (_render_glyph should take abool antialiased flag and switch to FT_RENDER_MODE_MONO when antialiasing is off, similarly to what's done in FT2Font.draw_glyph{,s}_to_bitmap).

Edit: antialiasing control has been implemented.

Directly render FT glyphs to the Agg buffer.  In particular, thisnaturally provides, with no extra work, subpixel positioning of glyphs(which could also have been implemented in the old framework, but wouldhave required careful tracking of subpixel offets).Note that all baseline images should be regenerated.  The new APIs addedto FT2Font are also up to bikeshedding (but they are all private).
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@QuLogicQuLogicQuLogic left review comments

Assignees

No one assigned

Projects

Status: In Progress

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@anntzer@story645@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp