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

os2: Fill Panose when writing font #761

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

Conversation

Finii
Copy link

@Finii Finii commented Sep 10, 2024

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

  • 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:

  • 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.

[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 opentypejs#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.

Signed-off-by: Fini Jastrow <[email protected]>
@Finii
Copy link
Author

Finii commented Sep 10, 2024

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 Finii marked this pull request as ready for review September 10, 2024 19:59
@Finii
Copy link
Author

Finii commented Sep 10, 2024

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)

@Connum
Copy link
Contributor

Connum commented Sep 11, 2024

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.

[why]
There is no check if the os2 table can be written to a buffer and
restored completely.

[how]
Do some spot checks for values.

Signed-off-by: Fini Jastrow <[email protected]>
@Finii
Copy link
Author

Finii commented Sep 11, 2024

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

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.

2 participants