matplotlib / matplotlib Public
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
Implement Font-Fallback in Matplotlib #20740
base: main
Are you sure you want to change the base?
Conversation
To actually trigger this workflow, I'll push similar commits similar to #20549.. Also, there's a lot of print/debug statements throughout, I'll remove them once this moves ahead.. |
9aee34b
to
9fabbc7
Compare
EDIT: I've merged the FT2Font changes together for both PRs |
0fa0625
to
e868fbc
Compare
On Linux and Mac. Add a font_manager test that loads the font and uses it. This could be useful in testing the multi-font support (matplotlib#20740 etc). I couldn't get the font installed on Windows. There is a Chocolatey installer that installs all of the Noto fonts, which takes a really long time to run.
@aitikgupta Do these have a review team looking at them? |
@jklymak I don't think anybody is actively reviewing these font handling PRs at the moment.. but they're ready for review and not drafts (I'll resolve the conflicts) |
Going ahead and removing the test, since font distributions for different Linux versions across CI are different.. and thus their kerning information is different -- different visual outputs. We can test it with a self-shipped CJK font, which is guaranteed to be present across all platforms.. but that'd be another problem because their file sizes are huge. |
(Unfortunately, I will not have the bandwidth to review this in the coming days.) |
Hi @aitikgupta we were all discussing this PR yesterday - did you have an opportunity to rebase and address the review comments? |
Hey @jklymak, I'm currently out sick. Will get back to this as soon as I can, probably a week or two. Apologies! |
No rush, get better! |
Hey @QuLogic, addressed all the comments (added comments for further discussions), and brought the PR up to speed! |
Use _findfont_cached directly in find_fonts_by_props rather than having one more caching layer.
Default in 128, go to 1024.
Seems to never be called or documented?
The caches/mappings still existing at the c++ layer, but do not expose them to Python. This may make a come back, but after we have sorted out the reference counting.
Correctly reference counting for the Python visible objects passed in to init.
Rebased to get CI to re-run and to make sure nothing has changed too badly under us. |
I also had a local commit with diff --git a/src/ft2font_wrapper.cpp b/src/ft2font_wrapper.cpp
index 0017617b75..8ced8e1e92 100644
--- a/src/ft2font_wrapper.cpp
+++ b/src/ft2font_wrapper.cpp
@@ -263,7 +263,7 @@ struct PyFT2Font
Py_ssize_t shape[2];
Py_ssize_t strides[2];
Py_ssize_t suboffsets[2];
- std::vector<PyObject *> fallbacks;
+ PyObject* fallback_tuple;
};
static unsigned long read_from_file_callback(FT_Stream stream,
@@ -386,22 +386,32 @@ static int PyFT2Font_init(PyFT2Font *self, PyObject *args, PyObject *kwds)
open_args.flags = FT_OPEN_STREAM;
open_args.stream = &self->stream;
- if (fallback_list && PyList_Check(fallback_list)) {
- Py_ssize_t size = PyList_Size(fallback_list);
-
- for (Py_ssize_t i = 0; i < size; ++i) {
- // this returns a borrowed reference
- PyObject* item = PyList_GetItem(fallback_list, i);
- // Increase the ref count, we will undo this in dealloc
- // this makes sure things do not get gc'd under us!
- Py_INCREF(item);
- self->fallbacks.push_back(item);
- // TODO: check whether item is actually an FT2Font
- FT2Font *fback = reinterpret_cast<PyFT2Font *>(item)->x;
- // Also (locally) cache the underlying FT2Font objects
- fallback_fonts.push_back(fback);
- }
+ if (PyList_Check(fallback_list)) {
+ // we got a list
+ self->fallback_tuple = PyList_AsTuple(fallback_list);
+ } else if (PyTuple_Check(fallback_list)) {
+ // we got a tuple
+ PyErr_SetString(PyExc_TypeError, "Tuple not supported yet");
+ goto exit;
+ } else if (fallback_list == Py_None) {
+ // we got None
+ self->fallback_tuple = PyTuple_New(0)
+ } else {
+ // we got anything else!
+ PyErr_SetString(PyExc_TypeError, "Must be list, tuple, or None");
+ goto exit;
+ }
+ Py_ssize_t size = PyTuple_GET_SIZE(self->fallback_tuple);
+ fallback_fonts.resize(size);
+
+ for (Py_ssize_t i = 0; i < size; ++i) {
+ // this returns a borrowed reference
+ PyObject* item = PyTuple_GET_ITEM(self->fallback_tuple, i);
+ // TODO: check whether item is actually an FT2Font
+ FT2Font *fback = reinterpret_cast<PyFT2Font *>(item)->x;
+ // Also (locally) cache the underlying FT2Font objects
+ fallback_fonts.push_back(fback);
}
if (PyBytes_Check(filename) || PyUnicode_Check(filename)) {
@@ -437,9 +447,7 @@ exit:
static void PyFT2Font_dealloc(PyFT2Font *self)
{
delete self->x;
- for (size_t i = 0; i < self->fallbacks.size(); i++) {
- Py_DECREF(self->fallbacks[i]);
- }
+ Py_DECREF(self->fallback_tuple);
Py_XDECREF(self->py_file);
Py_TYPE(self)->tp_free((PyObject *)self);
in it, but I do not remember it's state.... |
I think we should ignore the git cleanliness issue until we get this all the way sorted and then do a final re-write of history, the context of exactly what @aitikgupta was thinking is the work was going in is still way to valuable to risk me squashing it away ;) |
|
and even after fixing a few compiler errors, that diff definetily causes segfaults, so not obviously a good idea yet.... |
Co-authored-by: Aitik Gupta <aitikgupta@gmail.com>
|
PR Summary
This PR modifies the internal structure of
FT2Font
(the interface between fonts and Matplotlib) in favor of implementing Font Fallback for Matplotlib, and allow Agg backend to use the new codepath.It builds on the previous PR: #20549, which was the 'first-step', i.e., "parsing multiple families".. this PR implements using those families for font fallback.
This would help us in multi-language support, for example (Previous / After):
^the fonts are chosen such that the difference is visually noticeable.
A flowchart explaining the text rendering algorithm with font fallback:

Here's the script:
Fixes #18883, #15260
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).