Skip to content

Simplify glyph stream logic in ps backend #23965

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

Closed
tacaswell opened this issue Sep 20, 2022 · 5 comments
Closed

Simplify glyph stream logic in ps backend #23965

tacaswell opened this issue Sep 20, 2022 · 5 comments
Labels
Difficulty: Medium https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Good first issue Open a pull request against these issues if there are no active ones!
Milestone

Comments

@tacaswell
Copy link
Member

Seems good, but perhaps simpler to use a groupby here? Something like (untested)

diff --git a/lib/matplotlib/backends/backend_ps.py b/lib/matplotlib/backends/backend_ps.py
index f209e811f1..4d31fd881a 100644
--- a/lib/matplotlib/backends/backend_ps.py
+++ b/lib/matplotlib/backends/backend_ps.py
@@ -7,6 +7,7 @@ import datetime
 from enum import Enum
 import functools
 from io import StringIO
+import itertools
 import logging
 import os
 import pathlib
@@ -626,10 +627,13 @@ grestore
         if ismath:
             return self.draw_mathtext(gc, x, y, s, prop, angle)
 
+        stream = []  # list of (ps_name, x, char_name)
+
         if mpl.rcParams['ps.useafm']:
             font = self._get_font_afm(prop)
+            ps_name = (font.postscript_name
+                       .encode("ascii", "replace").decode("ascii"))
             scale = 0.001 * prop.get_size_in_points()
-            stream = []
             thisx = 0
             last_name = None  # kerns returns 0 for None.
             xs_names = []
@@ -643,37 +647,24 @@ grestore
                 kern = font.get_kern_dist_from_name(last_name, name)
                 last_name = name
                 thisx += kern * scale
-                xs_names.append((thisx, name))
+                xs_names.append((ps_name, thisx, name))
                 thisx += width * scale
-            ps_name = (font.postscript_name
-                       .encode("ascii", "replace").decode("ascii"))
-            stream.append((ps_name, xs_names))
 
         else:
             font = self._get_font_ttf(prop)
             self._character_tracker.track(font, s)
-            stream = []
-            prev_font = curr_stream = None
             for item in _text_helpers.layout(s, font):
                 ps_name = (item.ft_object.postscript_name
                            .encode("ascii", "replace").decode("ascii"))
-                if item.ft_object is not prev_font:
-                    if curr_stream:
-                        stream.append(curr_stream)
-                    prev_font = item.ft_object
-                    curr_stream = [ps_name, []]
-                curr_stream[1].append(
-                    (item.x, item.ft_object.get_glyph_name(item.glyph_idx))
-                )
-            # append the last entry
-            stream.append(curr_stream)
+                glyph_name = item.ft_object.get_glyph_name(item.glyph_idx)
+                stream.append((ps_name, item.x, glyph_name))
 
         self.set_color(*gc.get_rgb())
 
-        for ps_name, xs_names in stream:
+        for ps_name, group in itertools.groupby(stream, lambda entry: entry[0]):
             self.set_font(ps_name, prop.get_size_in_points(), False)
             thetext = "\n".join(f"{x:g} 0 m /{name:s} glyphshow"
-                                for x, name in xs_names)
+                                for _, x, name in group)
             self._pswriter.write(f"""\
 gsave
 {self._get_clip_cmd(gc)}

Edit: In fact set_font itself already checks if the font has already been set previously and skips redundant setfont calls, so we could just entirely get rid of the groupby logic too.

Originally posted by @anntzer in #23910 (comment)

@tacaswell tacaswell added this to the v3.7.0 milestone Sep 20, 2022
@tacaswell tacaswell added Good first issue Open a pull request against these issues if there are no active ones! Difficulty: Medium https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues labels Sep 20, 2022
@tacaswell
Copy link
Member Author

Labeling this as a good first issue because it is an refactor of a limited portion of the codebase and there is a proposed patch.

Labeling as medium because the code that needs to be refactored is a complex and care will need to be taken to ensure the refactor behaves identically to the existing code.

@story645
Copy link
Member

xref #23964 since that fixes the issue that spawned this one and therefore any PR here should possibly be aware of that one

@Abhijnan-Bajpai
Copy link
Contributor

If this is issue is still open, I would like to try working on this. Could I be assigned?

@oscargus
Copy link
Member

We do not* assign issues, but you are very welcome to work on it!

  • If you find any issues assigned, it is either a core developer assigning themselves as a reminder or assigning another core developer to fix things they have broken earlier.

@oscargus
Copy link
Member

oscargus commented Nov 3, 2022

Closed in #24287

@oscargus oscargus closed this as completed Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: Medium https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Good first issue Open a pull request against these issues if there are no active ones!
Projects
None yet
Development

No branches or pull requests

4 participants