Skip to content
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

Traverse when there is no layer list #2441

Merged
merged 1 commit into from Nov 8, 2021
Merged

Traverse when there is no layer list #2441

merged 1 commit into from Nov 8, 2021

Conversation

rsheeter
Copy link
Collaborator

@rsheeter rsheeter commented Nov 6, 2021

Fixes #2438. Again :D In addition to unit test pyftsubset --output-file=/tmp/reem.ttf --gids=0,128,210,211,218,225,227,232,234,235,237,238,241,244,246,250,251,254,261,264,270,271,277,280,282,284,286,289,295,297,299,307,308,316,317,318,320,327,331,343,345,347,348,349,350,351,357,365,370,371,374,376,378,392,393,398,399 ReemKufiInk-Regular.otf now doesn't kerplode.

rsheeter added a commit to google/fonts that referenced this pull request Nov 6, 2021
def parseXmlInto(font, parseInto, xmlSnippet):
parsed_xml = [e for e in parseXML(xmlSnippet.strip()) if not isinstance(e, str)]
for name, attrs, content in parsed_xml:
parseInto.fromXML(name, attrs, content, font)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use four space indentation

@@ -687,6 +687,33 @@ def test_splitMarkBasePos():
]


class ColrV1Test(unittest.TestCase):
def setUp(self):
self.font = FakeFont(['.notdef', 'meh'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four space indentation please

@anthrotype anthrotype merged commit de58709 into main Nov 8, 2021
@anthrotype
Copy link
Member

@rsheeter merged. I am going to the fix indentation issue

@anthrotype anthrotype deleted the i2438 branch November 8, 2021 12:24
<BaseGlyphPaintRecord index="0">
<BaseGlyph value="meh"/>
<Paint Format="1"><!-- PaintColrLayers -->
<NumLayers value="0"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaledhosny @rsheeter hm, a NumLayers=0 is very odd, as the whole PaintColrLayers is no-op basically. Did nanoemoji produce that? Maybe the input SVG was completely empty (e.g. a whitespace glyph). I think in this case, we don't want to emit anything, i.e. the BaseGlyphPaintRecord.Paint should be None, not a no-op PaintColrLayers with NumLayers=0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that indeed the case. The glyph in question is space glyph and the SVG produced by picosvg is:

<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 130.0 1500.0" width="130.0" height="1500.0">
  <defs/>
</svg>

@rsheeter
Copy link
Collaborator Author

rsheeter commented Nov 8, 2021

Interesting; we should probably file a bug for that in nanoemoji as well. Still good we don't crash here.

@anthrotype
Copy link
Member

@rsheeter I'm already working on a PR (found another bug while working on it 😅)

rsheeter added a commit to google/fonts that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReemKufiInk crashes pyftsubset
3 participants