Fixed Issue #8068 - SVG encoding #8415
Conversation
Added test encoding issue in SVG backend
|
Does #8286 also fix the same problem? |
|
@tacaswell Hi, sorry for the late reply. |
| for i, c in enumerate(enc0.encoding)} | ||
|
|
||
| # Make a list of each glyph by splitting the encoding | ||
| enc0_list = [] |
There was a problem hiding this comment.
this is the same as enc0_list = [e.split('/') for e in enc0.encoding] ?
Why do this splitting?
There was a problem hiding this comment.
No, that'll make a 2D list, but we need 1D.
The encoding that is generated looks like this ['Grave/Acute/Circumflex/Tilde/Dieresis/Hungarumlaut/Ring.....']
Thus splitting at "/" gives us individual character names.
Not only that, each index actually corresponds to its character code
(eg: enc0_list[142] = 'uni20A9' which is the Won character)
Therefore, line 363-364, enumerates the list with i = character code and c = character name and creates a dictionary character code => font index
Later, in the code the glyph is retrieved using this dictionary
if enc:
charcode = enc.get(glyph, None)
Hope this makes sense :)
There was a problem hiding this comment.
how has this ever worked if that is the case?
| enc0_list += e.split("/") | ||
|
|
||
| # Encoding provided by the font file mapping names to index | ||
| enc = {i: font.get_name_index(c) or None |
There was a problem hiding this comment.
This is in a block for when charmap_name == "ADOBE_STANDARD", why change to not use the standard encoding?
I think the fix should probably be fixed above to select a better character map for the file?
There was a problem hiding this comment.
Yes, this is the block where if charmap_name == "ADOBE_STANDARD" and font_bunch.encoding:
Since font_bunch already has Unicode values in them, we don't need to specially use the adobe standard file.
Thus, it was removed completely.
There was a problem hiding this comment.
Then the conditional should be changed, not just silently internally ignored.
| @@ -382,7 +383,10 @@ def get_glyphs_tex(self, prop, s, glyph_map=None, | |||
| charcode = glyph | |||
|
|
|||
| if charcode is not None: | |||
| glyph0 = font.load_char(charcode, flags=ft2font_flag) | |||
| if use_glyph: | |||
There was a problem hiding this comment.
This should probably be merged up into the conditionals above to simplify the logic?
There was a problem hiding this comment.
The charcode is set right before we reach this condition if charcode is not None:
Therefore, it seems like the right spot to decide which font method to use to load the charcode.
There was a problem hiding this comment.
your right, I missed that there was a path to get a non-empty enc that did not set use_glyph to True.
There was a problem hiding this comment.
Actually, this is very problematic, the code path above where you set use_glyph is a caching mechanism so the second time around this may have the wrong value of use_glyph?
|
Can you explain this changes a bit better? Assume I know nothing about how font encoding works :) Could you include the changes from #8286 in this PR (or explain why they are wrong!)? |
|
@tacaswell I hope the replies to your comments give you more insight of the changes :) |
|
The test failures look real. I am still extremely uncomfortable with this change because I do not understand it yet. This seems to be drastically changing how this code works (by consuming the encoding from the font rather than forcing it to use the adobe character map) but is still leaving a bunch of the old machinery around leaving the code in a very confused state. |
| # Make a list of each glyph by splitting the encoding | ||
| enc0_list = [] | ||
| for e in enc0.encoding: | ||
| enc0_list += e.split("/") |
There was a problem hiding this comment.
Note: one needs to do e.decode("ascii").split("/") to test this PR on Py3.
|
I think this has been superseded by #12928 (which owes much to this PR, thanks :)). |
SVG backend now supports special characters like won symbol with usetex.
#8068