[write-fonts] Add Glyph::Empty #759
Conversation
| match self { | ||
| Glyph::Simple(glyph) => glyph.contours().is_empty(), | ||
| Glyph::Composite(glyph) => glyph.components().is_empty(), | ||
| } |
There was a problem hiding this comment.
This still seems valid even in the presence of Empty?
There was a problem hiding this comment.
This was only used to decide whether or not to write data into glyf. The Empty variant just writes no data, and so 'just works' (and when converting to the enum from an instance of SimpleGlyph, we turn an empty glyph into the Empty variant.
write-fonts/src/tables/glyf.rs
Outdated
| @@ -57,18 +59,11 @@ impl Glyph { | |||
| /// The bounding box for the glyph | |||
| pub fn bbox(&self) -> Bbox { | |||
| match self { | |||
| Glyph::Empty => todo!(), | |||
There was a problem hiding this comment.
Hmm... this is a big footgun. Un-lgtm!
There was a problem hiding this comment.
ahaha oops. This is actually something to discuss, and maybe @dfrg has input? Basically the bounding box of an empty glyph is not (0,0), (0,0), because that implies that the point (0,0) is contained. I think for correctness this should probably return an Option? This has been a source of bugs in other geometry code I've written in the past.
There was a problem hiding this comment.
I think returning an option here makes sense. We can always unwrap_or_default if we want the all zero bbox for some reason.
This is a cleaner way of representing the idea of a glyph with no outlines, but which should still be added to loca. I played around with also adding this in read-fonts, but it doesn't really play well with codegen or parsing, there, because we expect a format enum to have a table for each variant, and in the empty case we have no data from which to parse a table.
This is a cleaner way of representing the idea of a glyph with no outlines, but which should still be added to loca.
I played around with also adding this in read-fonts, but it doesn't really play well with codegen or parsing, there, because we expect a format enum to have a table for each variant, and in the empty case we have no data from which to parse a table.
Glyph::Emptyvariant? #561JMM