GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — New FreeType wrapper. by anntzer · Pull Request #9763 · matplotlib/matplotlib 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Layout::~Layout() | ||
| { | ||
| if (!moved) { | ||
| std::for_each(glyphs.begin(), glyphs.end(), FT_Done_Glyph); |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
strict aliasing
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| namespace matplotlib::ft2 { | ||
|
|
||
| Face::Face(std::string const& path, double hinting_factor) : | ||
| ptr{ |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
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...). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
6d4ae43 to
2b5d281
Compare
a3cc592 to
aa04c71
Compare
|
Where is this at now that we are at MPL3.0? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 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).
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.