Skip to content

New FreeType wrapper. #9763

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

Closed
wants to merge 1 commit into from
Closed

New FreeType wrapper. #9763

wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 13, 2017

A FreeType wrapper rewrite based on pybind11. See #5414 (which sadly appears to be abandoned) for the motivation. Also implements (essentially) a MEP14-style API (see the Layout class), which should make it fairly easy (based on my experience with mplcairo) to support complex text layout (Indic scripts #8765; Right-to-left languages) (if we accept to bring in new (optional?) dependencies). The layout class should also make mathtext rendering (a bit) faster: the current implementation requires the parser to go twice through the string (once just to compute the extents of result, once with the actual rendering); this implementation just asks the parser to pass a list of positioned glyphs and rectangles (single pass) and does the sizing itself (with an iteration over the glyphs/rectangles, rather than an additional parsing round).

This requires a C++17 compiler, but such compilers are now directly available on conda. Note, however, that Matplotlib's setupext.py does not play well with non-system compilers (#9737) so you can effectively only test this PR locally with a modern system compiler (this is also why the PR is marked [ci skip]). Solving #9737 is probably a strict prerequisite for this PR.

The new wrapper weights <1000 lines of code, which is ~3x shorter than the old ones.

Adaptation of the codebase to the new wrapper is mostly complete. Non-antialiased text rendering is currently missing (but should be easy to implement).

Although I tried to reproduce the (dubious) hinting_factor option that currently exists, I haven't actually managed to make the text layout as bad as ft2font sometime does (cf. the wiggly baselines mentioned in #5414) :-), and I don't think it's really worth trying to do that -- but that means that test images will need to be updated.

Unlike #5414, I haven't moved the wrapper to a library separate from matplotlib, because I'd rather take advantage of the FreeType-related build scripts (including local_freetype) rather than replicating them.

@tacaswell tacaswell added this to the v2.2 milestone Nov 13, 2017
@Kojoley
Copy link
Member

Kojoley commented Nov 15, 2017

This is really cool! Every time I look into freetype/agg bindings I want them to be written with something like this.

This requires a C++17 compiler

The C++17 standard has not yet released 😉

C++11 is fully supported only by VS 2017 (and except some bugs in by VS 2015), what makes it Python 3 only if it is going to stay a part of mpl and not an independent library.

Why not use Cython? I do not see a big difference in bringing pybind11 or cython dependency.

Layout::~Layout()
{
if (!moved) {
std::for_each(glyphs.begin(), glyphs.end(), FT_Done_Glyph);
Copy link
Member

Choose a reason for hiding this comment

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

If you moved out to an empty Layout the glyphs will be empty so no need in extra handling, you can just remove the flag and leave defaulted move constructor Layout(Layout&&) = default;, or if you are for some reason do not need to free the glyphs (after moving to Layout with some glyphs) you should just vall glyphs.clear() in move-ctor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::move will not empty the first vector (why would it? the vector is in an invalid state after the move, it may well still be the same).
And as you noticed the flag is there to avoid FT_Done_Glyph'ing twice the same glyph after a move occurred.

FT_CHECK(
FT_Glyph_To_Bitmap, &glyph, FT_RENDER_MODE_NORMAL, nullptr, false);
}
auto b_glyph = reinterpret_cast<FT_BitmapGlyph>(glyph);
Copy link
Member

Choose a reason for hiding this comment

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

strict aliasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

namespace matplotlib::ft2 {

Face::Face(std::string const& path, double hinting_factor) :
ptr{
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why shared_ptr is needed if you are initializing it in the object constructor...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's a bit more convenient if a copy constructor is available (also, pybind11 requires either the move or the copy ctor to exist). So you either need to use a raw pointer and call FT_Reference_Face in the copy ctor and then FT_Done_Face on the way out, or use a shared_ptr+custom deleter and the default copy/move ctors which do the job for you.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 15, 2017

If you want a backend binding written in the same style you should look at https://github.com/anntzer/mplcairo... (why did I not fix Agg? because I can't find half-decent docs for Agg anywhere...).

@anntzer anntzer force-pushed the ft2 branch 6 times, most recently from 13f925e to b1676d0 Compare November 17, 2017 08:29
@Tillsten
Copy link
Contributor

I think @anntzer is right, pybind is the better choice here. In cython, one has to replicate a lot from the header files in cython declarations, which is quite nightmarish for a library like freetype. Maybe cffi could deal with it, but in my experience it also requires a lot of wrangling with complicated headers.

@jklymak
Copy link
Member

jklymak commented Mar 7, 2018

Where is this at now that we are at MPL3.0?

@Kojoley
Copy link
Member

Kojoley commented Feb 3, 2020

I do not have problem with pybind11 or recent C++ standards, however while we can bundle/download pybind11 it is not a case for a compiler. I can take downgrading compiler version requirement work. Will this work for you?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 3, 2020

I don't mind making the thing C++14 compatible (this doesn't look too hard, as you mentioned); C++11 would be annoying at least because it lacks std::(experimental::)optional, haven't checked if more things are needed.
The annoying part of the work now is just the rebase, and the multiple additional rebases that will likely be needed while this wanders through reviewing ;)

@Kojoley
Copy link
Member

Kojoley commented Feb 3, 2020

Is std::optional used in the PR? It is a C++17 thing, but if it is needed for pybind11 we could try to borrow optional from Boost/libc++ (which implementation actually does not require even C++11), and even variant if it is also needed.

What about C++14: Python 3.5+ on Windows requires MSVC2015+, so the problem is in some linux distros, and we can just tell to use recent Clang targeted system standard library (however it is most likely not an option for distro maintainers).

@anntzer
Copy link
Contributor Author

anntzer commented Feb 3, 2020

std::optional is used but we can also fallback on the older std::experimental::optional, which is also supported by pybind11, dunno how far back this lets us go compiler-wise.

@jklymak jklymak marked this pull request as draft March 24, 2021 23:34
@ianthomas23 ianthomas23 mentioned this pull request Sep 9, 2022
13 tasks
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Apr 24, 2023
@anntzer anntzer closed this Apr 24, 2023
@anntzer anntzer deleted the ft2 branch September 6, 2024 21:49
@anntzer anntzer restored the ft2 branch September 6, 2024 21:49
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 12, 2024
This follows some of the idea from matplotlib#9763.

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 19, 2024
This follows some of the idea from matplotlib#9763.

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
@QuLogic QuLogic mentioned this pull request Sep 19, 2024
3 tasks
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 20, 2024
This follows some of the idea from matplotlib#9763.

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 20, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 21, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 23, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 24, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 2, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 10, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2024
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>
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2024
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>
tacaswell pushed a commit to QuLogic/matplotlib that referenced this pull request Oct 18, 2024
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>
tacaswell pushed a commit to QuLogic/matplotlib that referenced this pull request Oct 18, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MEP: MEP14 text handling status: inactive Marked by the “Stale” Github Action status: needs rebase topic: text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants