Font is treated as a required argument in Glyph.draw()
Description
This makes it so font is not treated as a required argument in Glyph.draw.
Motivation and Context
This looks like a simple regression bug.
Checklist:
- [x] I did
npm run testand all tests passed green (including code styling checks). - [x] I have added tests to cover my changes.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the README accordingly.
- [x] I have read the CONTRIBUTING document.
The Font.defaultRenderOptions property is needed for variable font rendering, making this optional breaks variable fonts.
Did 1.3.4 not support variable fonts?
To be honest, I do not like this API. The user always retrieves a glyph from a font object, so logically the glyph should already know about the font it is associated with. A function where there is literally only one value you should be passing into an argument should be inferred by the library.
I would leave this optional because for the majority use case it is not necessary. It doesn't break anything, since the documentation states to pass in the font for variable font rendering.
Additionally, font is considered optional in getPath(), which is the function that retrieves the variation in the first place, no? The API is inconsistent.
https://github.com/opentypejs/opentype.js/blob/ab95cea6560b9a8ba20f6290603e7e6e8f4bd02b/src/glyph.mjs#L148
https://github.com/opentypejs/opentype.js/blob/ab95cea6560b9a8ba20f6290603e7e6e8f4bd02b/src/glyph.mjs#L157
I added this to getPath() as the function header says it was needed for fontHinting. Yes I also don't like the API, it was a quick fix to not change more on the core libreary than strictly necessary. The draw function back then didn't have the argument at all. (which I admit was also inconsistent, honestly likely because I didn't care about it)
PS: Genereally instead of "a && a.b" .. i'd just do "a?.b"
Glyph objects do not have a reference back to the font. You can copy a glyph from one font to another by adding the Glyph object to multiple font objects' GlyphSet.
Regarding a && b: this was previously necessary because we couldn't use the optional chaining operator for backwards compatibility. Indeed, with the new build process, we can make use of modern syntax.
PS: Genereally instead of "a && a.b" .. i'd just do "a?.b"
I agree, just following the existing code convention.
Glyph objects do not have a reference back to the font. You can copy a glyph from one font to another by adding the Glyph object to multiple font objects' GlyphSet.
I see these as two separate use cases:
- I want to load an existing font into memory in a browser, in order to draw it.
- I want to create a custom font using this library, then save it to an
.otffile.
I think the 2nd case is what you are referring to, which generally is not associated with the draw function being used. This scenario seems to be a weird mix where a use case not really designed for drawing messes with the draw function. Ideally, when a glyph is generated from a font, it would have a reference to all the information necessary to draw it.
If you build a font editor and want to render a preview of both fonts, you'll want to use the draw function.
Ok, but at the same time the library doesn't currently support writing to the hvar table anyways. Even if two fonts share the same glyph, can't make use of that "font" argument in the draw call. https://github.com/opentypejs/opentype.js/blob/master/src/tables/hvar.mjs#L41
Either way this discussion is a tangent to the PR. Since a glyph contains its own path data and can be drawn on its own, "font" shouldn't be a required argument in the draw call.
And it would be nice in the future if a glyph that was retrieved from a font can have a reference to the original font it came from as a "default" setting for drawing, which can be overwritten with the "font" argument.
I think most people are using this library simply to draw a font on a canvas, or generate a SVG shape from a font, or generate a 3D mesh from a font. Would be good to optimize the API for this use case.