Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fixed Issue #8068 - SVG encoding#8415
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is the same asenc0_list = [e.split('/') for e in enc0.encoding] ?
Why do this splitting?
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.
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 withi = character code andc = character name and creates a dictionarycharacter 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how has thisever 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is the block whereif charmap_name == "ADOBE_STANDARD" and font_bunch.encoding:
Sincefont_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should probably be merged up into the conditionals above to simplify the logic?
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.
Thecharcode is set right before we reach this conditionif charcode is not None:
Therefore, it seems like the right spot to decide which font method to use to load thecharcode.
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.
your right, I missed that there was a path to get a non-emptyenc that did not setuse_glyph toTrue.
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, this is very problematic, the code path above where you setuse_glyph is a caching mechanism so the second time around this may have the wrong value ofuse_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note: one needs to doe.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