-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Implement libraqm for vector outputs #30607
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
Conversation
| const auto load_flags = static_cast<FT_Int32>(flags); | ||
|
|
||
| FT2Font::LanguageType languages; | ||
| if (auto value = std::get_if<FT2Font::LanguageType>(&languages_or_str)) { |
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.
Actually not really related to this PR, but is there a reason not to use the std::visit(variant, overloaded {...}) pattern? (see example 4 of https://en.cppreference.com/w/cpp/utility/variant/visit2.html, or grep for overloaded in the mplcairo codebase)
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've not tried std::visit, but at least for std::get_if, one reason why we stuck with it instead of something else is that it wouldn't compile on macOS with the current deployment target we have. It's possible that it's fine now, but I don't believe we've bumped the target in a while.
| .. warning:: | ||
| This API uses the fallback list and is both private and provisional: do not use | ||
| it directly. |
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.
In the long term it would be nice if mplcairo could directly call this (instead of having to reimplement the whole thing itself as well); but I suspect the issue is the fact that this has to expose FT2Font objects and mplcairo would thus need to also use the same ft2Library instance as matplotlib, which... well I don't really know if that's something matplotlib wants to expose publically. (I'm mostly just thinking aloud here.)
(mplcairo could also just grab the filename of the FT2Font objects and reopen them, which is not too bad with caching, but I'm not sure this would work with variable fonts at least?)
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.
There would need to be some way to share the FreeType library itself, I think. One issue I had initially was between sharing the internal FreeType structures with another when I accidentally had libraqm link to an external one. I'm not sure how easy this would be, unless we moved to some common provider.
520fd60 to
14091a4
Compare
|
I realized tests were not fully enabled here, and found some issues with |
14091a4 to
e0f4ef1
Compare
e0f4ef1 to
77b8271
Compare
|
I've expanded the new feature tests to include |
77b8271 to
ed4098d
Compare
|
I've rebased this and added the image changes for review now that there are no other PR dependencies. Most of the changes are pretty much invisible, as the glyph locations shift by <1/64 of a pixel due to #30000 (comment) Sometimes this causes the rasterization for test comparison to shift a glyph by a whole pixel, since the DPI is rather low, but it's not too noticeable at a normal size. I don't know if we want to round to nearest 1/64th, or just run with this as is. The main ones you should look at are There are fortunately not too many image changes, but please don't merge with them included, as I'll move them to the other branch before doing so like the other PRs. |
ed4098d to
69899e5
Compare
69899e5 to
22e5b3d
Compare
|
Holding off merging in case @QuLogic wants to do image management. |
22e5b3d to
bd17cd4
Compare
|
Sorry for not reviewing further, and congrats on getting this in. |

PR summary
This PR is a followup to #30000 which did raster backends, now for vector backends. I'm not 100% on the kerning calculation, but the resulting differences are quite small; I think this may because of using a 16.16 value vs a 26.6 creating minor rounding differences.
This is based on #29695 to reduce conflicts.PR checklist