-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
New FreeType wrapper. #9763
Conversation
This is really cool! Every time I look into freetype/agg bindings I want them to be written with something like this.
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 |
Layout::~Layout() | ||
{ | ||
if (!moved) { | ||
std::for_each(glyphs.begin(), glyphs.end(), FT_Done_Glyph); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict aliasing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually the documented approach: https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_Render_Glyph
namespace matplotlib::ft2 { | ||
|
||
Face::Face(std::string const& path, double hinting_factor) : | ||
ptr{ |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
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...). |
13f925e
to
b1676d0
Compare
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. |
6d4ae43
to
2b5d281
Compare
a3cc592
to
aa04c71
Compare
Where is this at now that we are at MPL3.0? |
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? |
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. |
Is 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). |
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. |
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. |
This follows some of the idea from matplotlib#9763. Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
This follows some of the idea from matplotlib#9763. Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
This follows some of the idea from matplotlib#9763. Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.