GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — New FreeType wrapper. by anntzer · Pull Request #9763 · matplotlib/matplotlib

Conversation

@anntzer

@anntzer anntzer commented Nov 13, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@anntzer anntzer added MEP: MEP14 text handling topic: text labels Nov 13, 2017
@anntzer
anntzer force-pushed the ft2 branch from 08a686b to 4b10b44 Compare November 13, 2017 11:10
@tacaswell tacaswell added this to the v2.2 milestone Nov 13, 2017
@Kojoley

Kojoley commented Nov 15, 2017

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Kojoley
Kojoley reviewed Nov 15, 2017
View reviewed changes
Comment thread src/ft2/_layout.cpp
Layout::~Layout()
{
if (!moved) {
std::for_each(glyphs.begin(), glyphs.end(), FT_Done_Glyph);

@Kojoley Kojoley Nov 15, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@anntzer anntzer Nov 15, 2017

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/ft2/_layout.cpp
FT_CHECK(
FT_Glyph_To_Bitmap, &glyph, FT_RENDER_MODE_NORMAL, nullptr, false);
}
auto b_glyph = reinterpret_cast<FT_BitmapGlyph>(glyph);

@Kojoley Kojoley Nov 15, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@anntzer anntzer Nov 15, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/ft2/_layout.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/ft2/_ft2.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/ft2/_ft2.cpp
namespace matplotlib::ft2 {

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

@Kojoley Kojoley Nov 15, 2017

Copy link
Copy Markdown
Member

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@anntzer anntzer Nov 15, 2017

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@anntzer
anntzer force-pushed the ft2 branch from 4b10b44 to 1c60ee9 Compare November 15, 2017 01:22
@anntzer

anntzer commented Nov 15, 2017

Copy link
Copy Markdown
Contributor Author
  • "C++17" = whatever C++17 spec gcc and clang currently support. I am fine with waiting for MSVC support too (http://en.cppreference.com/w/cpp/compiler_support#C.2B.2B17_features), there are other things that need to be fixed first (e.g. setupext).
  • https://pybind11.readthedocs.io/en/stable/faq.html#working-with-ancient-visual-studio-2009-builds-on-windows actually explicitly states that you can compile this module with a modern compiler and link it with old-compiler-generated modules.
  • I think C++17 + pybind11 is much more expressive than Cython (and in any case you can also call py::eval from pybind11 :-)). Feel free to try rewriting this with Cython...

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...).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Tillsten commented Nov 27, 2017

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the ft2 branch from b1676d0 to b59cb2d Compare December 3, 2017 02:13
@anntzer anntzer mentioned this pull request Dec 3, 2017
Cross-compiling matplotlib and dependencies for OpenWRT fails on import matplotlib.pyplot #9244
Closed
@tacaswell
tacaswell requested a review from mdboom December 3, 2017 21:46
@anntzer
anntzer force-pushed the ft2 branch 2 times, most recently from 6d4ae43 to 2b5d281 Compare December 3, 2017 22:03
@anntzer
anntzer force-pushed the ft2 branch 4 times, most recently from a3cc592 to aa04c71 Compare January 14, 2018 12:12
@anntzer anntzer modified the milestones: v2.2, v3.0 Jan 17, 2018
@anntzer anntzer mentioned this pull request Jan 25, 2018
Clipped tick labels #8609
Closed
@anntzer
anntzer force-pushed the ft2 branch from aa04c71 to 8cad163 Compare January 25, 2018 02:40
@jklymak jklymak mentioned this pull request Mar 7, 2018
WIP: New FreeType wrappers #5414
Closed
2 of 4 tasks
@jklymak

jklymak commented Mar 7, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the ft2 branch from b232ad3 to 13c2a88 Compare March 11, 2018 02:07
@anntzer
New FreeType wrapper.
95a9f28
TODO: ttc support violates the assumption that the filename suffices to
identify the face (when caching).  We need to generate a uniquer id or
just key off the face object.
@anntzer
anntzer force-pushed the ft2 branch from 13c2a88 to 95a9f28 Compare March 11, 2018 05:32
@anntzer anntzer modified the milestones: v3.0, v3.1 Jul 10, 2018
@jklymak jklymak mentioned this pull request Oct 6, 2018
replace ttconv for ps/pdf #12418
Closed
@tacaswell tacaswell modified the milestones: v3.1.0, v3.2.0 Feb 24, 2019
@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 19, 2019
@anntzer anntzer modified the milestones: v3.3.0, unassigned Aug 19, 2019
@Kojoley

Kojoley commented Feb 3, 2020

Copy link
Copy Markdown
Member

@anntzer would you like to revive this?

It should not be a big trouble to lower requirements to C++14 (does it use any C++17 features besides nested namespaces and structural bindings?) which is +- adequately supported by MSVC2015U3 (July 2016), GCC 5 (April 2015), Clang 3.5 (September 2014), and is a default in GCC for 4 years (since GCC 6), in Clang for 2 years (since Clang 6).

Distro GCC Released EOL EOL LTS
Ubunutu 14.04 LTS (Trusty Tahr) 4.9 2014-04-17 2019-04 2022-04
Ubunutu 16.04 LTS (Xenial Xerus) 5.3 2016-04-21 2021-04 2024-04
Ubunutu 18.04 LTS (Bionic Beaver) 7.4 2018-04-26 2023-04 2028-04
Debian Jessie 4.9 2015-04-25 2018-06-17 ~2020-06-30
Debian Stretch 6.3 2017-06-17 ~2020 ~2022
Debian Buster 8.3 2019-07-06 ~2022
RHEL 5 4.1 2007-04-15 2013-01-08 2020-11-30
RHEL 6 4.4 2010-11-10 2016-05-10 2024-05-30
RHEL 7 4.8 2014-06-10 2019-08-06 TBD
RHEL 8 8 2019-05 2024-05 2029-05

Sadly, if we want to support any non-latest RHEL we have to use no more than C++11. Probably it is also not a big deal, since I cannot immediately recognize anything from C++14 that is used here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Feb 3, 2020

Copy link
Copy Markdown
Contributor Author

Only if we can pre-agree that this (especially adding a requirement on a recent C++ compiler and adding pybind11 as build-time dependency) is a viable approach that could ultimately be merged (it's a lot of work and I don't particularly want to do twice the same proof of concept); even then it may take a bit of time because, well, it's a lot of work :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kojoley

Kojoley commented Feb 3, 2020

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Feb 3, 2020

Copy link
Copy Markdown
Contributor Author

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 ;)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kojoley

Kojoley commented Feb 3, 2020

Copy link
Copy Markdown
Member

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Feb 3, 2020

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jklymak jklymak added the status: needs rebase label Mar 24, 2021
@jklymak
jklymak marked this pull request as draft March 24, 2021 23:34
@anntzer anntzer mentioned this pull request Sep 1, 2022
Use pybind11 for C/C++ extensions #23787
Closed
@ianthomas23 ianthomas23 mentioned this pull request Sep 9, 2022
[MNT]: Pybind11 transition plan #23846
Closed
13 tasks done
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions

github-actions Bot commented Apr 24, 2023

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@QuLogic QuLogic mentioned this pull request Sep 6, 2024
Convert ft2font extension to pybind11 #28785
Merged
1 task done
@anntzer
anntzer deleted the ft2 branch September 6, 2024 21:49
@QuLogic QuLogic mentioned this pull request Sep 19, 2024
FT2Font extension improvements #28842
Merged
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kojoley Kojoley Kojoley left review comments
@mdboom mdboom Awaiting requested review from mdboom

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

MEP: MEP14 text handling status: inactive Marked by the “Stale” Github Action status: needs rebase topic: text

Projects

None yet

Milestone

future releases

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@anntzer @Kojoley @Tillsten @jklymak @tacaswell @story645
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.