Fixed metadata that enables fonts to be recognised as a family (tested!) #630
Conversation
I've found out that a few fields that might be passed to opentype.js were not being used while producing the font. I have been able to pinpoint those fields and where they should be applied to produce fonts that are recognised as a family. Tested only with a couple of fonts but changes were regonised by Windows and other apps. An issue regarding the fields hhea.caretSlopeRise and hhea.caretSlopeRun for italic and oblique fonts is still pending review but does not prevent font usage or recognition as a family (caret slope had a weird inclination while testing on Word but not on Affinity Designer). These same changes were applied to the copy of the library used by Glyphr Studio (v2) where the tests were conducted.
There was a problem hiding this comment.
See my one comment, otherwise it looks good.
However, while the current tests are now passing, there are no tests to test the new functionality itself. We should test for
- the values being calculated/set correctly
- the panose fallback working and its values and the resulting os2 properties taking on the corresponding values
- the
macStylevalue being written correctly
I've found out that a few fields that might be passed to opentype.js were not being used while producing the font. I have been able to pinpoint those fields and where they should be applied to produce fonts that are recognised as a family. Tested only with a couple of fonts but changes were regonised by Windows and other apps. An issue regarding the fields hhea.caretSlopeRise and hhea.caretSlopeRun for italic and oblique fonts is still pending review but does not prevent font usage or recognition as a family (caret slope had a weird inclination while testing on Word but not on Affinity Designer). These same changes were applied to the copy of the library used by Glyphr Studio (v2) where the tests were conducted.
|
I added two more comments for some minor things. If that and the |
|
@Connum I marked our conversations as resolved, but please let me know if you have any further comments! |
|
Could you please merge the master branch back in and resolve the conflicts? Then this would be ready to be merged back in! |
|
Sorry, my inexperience showing again. I'm not sure exactly how to merge the master branch back in. I see this at the base of the pull request: And in GitHub desktop I see this: |
|
It's all good! I wanted to do a rebase first, but squash and merge worked fine. Thanks for your contribution, really appreciated! |
|
Hey, @Connum, I want to give you a huge thank-you for helping me out. I'm essentially a UX Designer pretending to be a JavaScript developer 😅 so the extra time you put in reviewing my code and helping me out - I really appreciate it! |
|
@Connum, one last question you may know the answer two - when does opentype.js ship a 'release'? v1.3.1 was released in 2020, what is the criteria for releasing 1.3.2? I want to use this new feature that just got merged. Should I just use a random build, or is 1.3.2 a thing that will happen soon? |
|
That's a good question, and unfortunately this is still to be discussed. Only about a year ago was this project revitalised thanks to @ILOVEPIE 's initiative (see #464), but as it often happens, life and paid work got in the way and active work on the project was dormant for several months again. Only recently have I found some time to review PRs and issues again and fix bugs myself. There are already some bigger features and refactorings in the master since the last version was released, and several in line in the form of outstanding PRs. Some internally used methods have changed that might have been exposed and used externally, therefore the next release will most probably be 2.0.0. Meanwhile, I would use the current master as a github dependency ( Would be lovely to see more contributions in the future, I'm always happy to help if my time allows! |
|
This is great, thank you for all the context! |
| bFamilyType: options.panose[0] || 0, | ||
| bSerifStyle: options.panose[1] || 0, | ||
| bWeight: options.panose[2] || 0, | ||
| bProportion: options.panose[3] || 0, |
There was a problem hiding this comment.
This PR introduces names for the individual Panose values.
I think this is problematic.
When one sees a property bProportion one might assume it has something to do with the font proportion. But the Panose values are ... a mess. The meaning of a certain value depends on the 1st Panose value.
For example with the 4th Panose value panose[3] contains
Proportionfor Latin TextSpacingfor Latin Script and Latin PictureAspectfor Latin Decorative
So I would not call it bProportion as that is an oversimplification. I believe it is best to keep just the array and let interested uses decode it by for example this excellent compilation (by Alan Bernard Hughes) that I usually use for reference:
https://forum.high-logic.com/postedfiles/Panose.pdf
Edit: Fix wrong name
Addendum:
The names chosen above are from Panose-1 which was rather simple. But when it has been expanded to be Panose-2 the values became multiple meaning depending on Family Kind.
Just looked it up again in Haralambous: Fonts & Encodings, O'Reilly (2007)
|
not sure if I can be of any help here |
|
Sorry, "I think this is problematic." is kind of an exaggeration. The real "problem" is described in the new PR #761, which fixes the Panose write operation. |
[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]>
This is an updated version of #606 with a few additions that allow the code to pass
npm run test. @Connum, If this gets merged, we can also close #606 .From @Lmpessoa
I've found out that a few fields that might be passed to opentype.js were not being used while producing the font. I have been able to pinpoint those fields and where they should be applied to produce fonts that are recognised as a family. Tested only with a couple of fonts but changes were regonised by Windows and other apps.
An issue regarding the fields hhea.caretSlopeRise and hhea.caretSlopeRun for italic and oblique fonts is still pending review but does not prevent font usage or recognition as a family (caret slope had a weird inclination while testing on Word but not on Affinity Designer).
These same changes were applied to the copy of the library used by Glyphr Studio (v2) where the tests were conducted.
Types of changes
Checklist:
npm run testand all tests passed green (including code styling checks).