-
Notifications
You must be signed in to change notification settings - Fork 14
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
Generate missing .notdef like ufo2ft #423
Conversation
7963776
to
a86d9e8
Compare
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.
A couple little notes but looks good, happy to see this addition :)
|
||
Self { | ||
name: NOTDEF.clone(), | ||
codepoints: HashSet::from([0]), |
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.
I'd double-check with @anthrotype but I'm not sure .notdef has any codepoints. Some fonts include a NUL glyph in addition to notdef, although I'm not sure about (and am curious to learn more!) the semantics/rationale for this.
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.
Oswald cmap and glyphorder are now identical so I think this is right. I will double-check with Cosimo just in case it's only sometimes right or not as simplistic as I made it. A lot of these little details turn out to be fiddly.
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.
Colin was right, the codepoints
for the generated .notdef glyph must be left empty, .notdef should be understood as the absence of a character mapping; 0x0000 is a different unicode codepoint denoting the NULL character (like in NULL-terminated C strings), it used to be among the recommended first 4 glyphs in old TrueType but current OT spec no longer does that and modern fonts don't contain NULL (and neither the carriage return CR character), but they all contain .notdef (un-cmapped) glyph as the first one (index=0).
(0x0030, 2), | ||
(0x031, 3), | ||
(0x032, 4) | ||
(0x0000, 0), |
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.
hm .notdef should not be mapped to anything, 0x0000 is the NULL character which is something else, a control character that is no longer required to be defined (there used to be a recommendation in the TrueType spec about the first 4 glyphs which was removed later on, so nowadays a font no longer needs to have a glyph for NULL 0x0000 character).
|
||
assert_eq!( | ||
GlyphId::new(0), | ||
font.cmap().unwrap().map_codepoint(0u32).unwrap() |
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.
again, this is wrong, NULL character (0x0000) should not be mapped to ".notdef" glyph, i.e. the first glyph (gid=0) in the font. They are different things.
], | ||
[0x20, 0x21, 0x2d, 0x3d] | ||
[0x00, 0x20, 0x21, 0x2d, 0x3d] |
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.
nope, {0x00: GlyphId(0)} should be absent from the cmap, unless the source explicitly contained this for whatever reason..., but in this case fontc wrongly generated that.
For WghtVar.glyphs, which does not define notdef, we get this:
Fixes #149
Fixes #420