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

Font is treated as a required argument in Glyph.draw() #727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

opengraphica
Copy link

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:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@opengraphica opengraphica changed the title Font is treated as a required argument Font is treated as a required argument in Glyph.draw() Jun 9, 2024
@ILOVEPIE
Copy link
Contributor

The Font.defaultRenderOptions property is needed for variable font rendering, making this optional breaks variable fonts.

@opengraphica
Copy link
Author

opengraphica commented Jun 14, 2024

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.

@opengraphica
Copy link
Author

opengraphica commented Jun 14, 2024

Additionally, font is considered optional in getPath(), which is the function that retrieves the variation in the first place, no? The API is inconsistent.

options = Object.assign({}, font && font.defaultRenderOptions, options);

if(font && font.variation) {

@axkibe
Copy link
Contributor

axkibe commented Jun 14, 2024

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"

@Connum
Copy link
Contributor

Connum commented Jun 14, 2024

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.

@opengraphica
Copy link
Author

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:

  1. I want to load an existing font into memory in a browser, in order to draw it.
  2. I want to create a custom font using this library, then save it to an .otf file.

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.

@Connum
Copy link
Contributor

Connum commented Jun 15, 2024

If you build a font editor and want to render a preview of both fonts, you'll want to use the draw function.

@opengraphica
Copy link
Author

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.

@opengraphica
Copy link
Author

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.

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.

4 participants