opentype.js icon indicating copy to clipboard operation
opentype.js copied to clipboard

os2: Fill Panose when writing font

Open Finii opened this issue 1 year ago • 4 comments
trafficstars

Parsing a font from a buffer and writing it back to a buffer changes all Panose values to zero. This PR fixes this.

Description

[why] When an existing font file is read from a buffer and then written back into a buffer all Panose values will be zero.

The problem has been most likly been introduced with PR #630 (but I did not check that).

There are two disjunct data structures in the font.tables.os2 object that describe the panose values:

  • An array with the Panose values .panose = [ 1, 2, ...]
  • Dedicated properties for each Panose value e.g. .bFamilyType

The aforementioned PR seems to address only fonts created from scratch and not parsed from a buffer.

Writing out the font into a buffer will always use the dedicated Panose properties and ignore the .panose array.

Parsing a font does set the array but not the dedicated properties. They are not even existing then.

[how] When an existing font is parsed the .panose array is filled (as before). But now the dedicated properties are also created and filled with the individual values.

[note] The written font is always using the dedicated values. If a user changes the panose array that will have no effect. There are no checks if the data is consistent. Having the same data in two disjunct structures is not so nice to handle.

Motivation and Context

I want to read a font file and write it (modified) back to a file; as unchanged as possible.

How Has This Been Tested?

Manual tests with ttx ;)

Screenshots (if appropriate):

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] 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 Contribute README section.

Finii avatar Sep 10 '24 19:09 Finii

Well, now (with this PR) the user has both, the array and the individual properties (see image below). For writing only the individual properties are used (as before this PR).

To avoid confusion I would drop one of the two. Dropping the individual properties feels most natural for me as I always think about the Panose as indexed values. But dropping the array would be simpler and would not break any existing code (because changing the values never had any effect - only reading would be impossible if the array is dropped).

Anyhow, changing API with v2.0.0 is a good time to clean this up and drop one of the two. Well, thats just a suggestion.

image

Finii avatar Sep 10 '24 19:09 Finii

I believe how Panose is represented in other applications can be important, to see what users might expect.

showttf

image

ttfdump

image

fontforge

image

Access via Python, via List (Array)

image

Glyphs3

Screenshot 2024-09-10 at 22 26 05

(Glyphs3, the only commercial software here, does not believe in Panose and has hidden it away in a very raw form. See for example https://forum.glyphsapp.com/t/when-to-set-panose-numbers-typoascender-winascent-etc/10596)

Finii avatar Sep 10 '24 20:09 Finii

Having the same data in two disjunct structures is not so nice to handle.

Yeah, that's a good point... We have this issue in other places as well I think. Proxy objects would be a solution, or we should drop the raw values alltogether as you suggested.

Connum avatar Sep 11 '24 06:09 Connum

Added some test for os2 table parsing writing parsing (full round trip).

I struggled a bit with where to put the tests, as test/tables/ in principle maybe should only test stuff inside that modules; but we need other modules to check this completely, so maybe test in font, but then the tests are to specific to one table. Hope you get what I mean.

Anyhow, this is at least some starting point, feel free to move/remove the tests ;-D

Finii avatar Sep 11 '24 10:09 Finii