Skip to content

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

Open
opengraphica wants to merge 2 commits into
opentypejs:masterfrom
opengraphica:issue/required-font-draw
Open

Font is treated as a required argument in Glyph.draw()#727
opengraphica wants to merge 2 commits into
opentypejs:masterfrom
opengraphica:issue/required-font-draw

Conversation

@opengraphica

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@opengraphica

opengraphica commented Jun 14, 2024

Copy link
Copy Markdown
Author

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

opengraphica commented Jun 14, 2024

Copy link
Copy Markdown
Author

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

axkibe commented Jun 14, 2024

Copy link
Copy Markdown
Contributor

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

Connum commented Jun 14, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Connum commented Jun 15, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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