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

First review! -> for GF #8

Closed
emmamarichal opened this issue May 29, 2024 · 5 comments
Closed

First review! -> for GF #8

emmamarichal opened this issue May 29, 2024 · 5 comments
Assignees

Comments

@emmamarichal
Copy link
Contributor

Hello @jrgdrs!

I'm Emma, and I'm in charge of the Wittgenstein onboarding.

I took a first look to the font, here are my comment/remarks:

Variable fonts:

We would appreciate to onboard it as a variable font, since the two masters are compatible. However, we can't export instances that have been extrapolate (there is no ExtraLight master). I tried to generate a ExtraLight master from the instance, but it creates some interpolations issues, especially with open corners (we can't accept that, the export will crash).

Screenshot 2024-05-29 at 13 56 29

So, what we can do here:

  • onboard it now, first with a weight axis from Regular to Black and add the other instance/master later.
    We can still let the extralight and light instance as statics, in the repo (but not in GF)
  • wait until you add a compatible ExtraLight master

Other

  • five.osf.tosf -> five.tosf (to be applied to all numbers concerned)
  • 🔥 FAIL Ensure dotted circle glyph is present and can attach marks: you have to add ogo anchor to the dottedCircle
  • There is an issue of interpolation in degree -> see Interpolation.pdf in QA.zip
  • There are some inferior and .inferior -> .inferior and .superior are not reachable by any features.

    -> .numr.dnomsuperiorinferior
should be suffisant.

Complete QA:
QA.zip

You will find here html proofs, the fontbakery report and the interpolation errors.


Let me know if you have any questions, and when these points are fixed. I'll then set up some last few things (build process, last QA, etc.) and send you a final PR from my fork :)
Cheers!

@jrgdrs jrgdrs self-assigned this May 29, 2024
@jrgdrs
Copy link
Owner

jrgdrs commented May 29, 2024

Hello @emmamarichal

thank you for your onboarding support. I would be happy if you can continue the process and have carried out the fixes.

EL

ExtraLight was a misunderstanding i think. I don't want to provide an EL cut, only the range from Regular (400) to Black (900). The EL was included in the Glyphs version, but I thought that i had switched off via the option so that it would not be exported. (When working on the interpolations, I like to display also the extrapolations, just as deviations and errors are much better visible. However, EL was not intended for exporting. That is why I have now removed the layer from Glyphs completely.

Other

=> renamed the 10 tabular oldstyle figures from *.osf.tosf to *.tosf
=> added anchor for ogo and also for bottom to the dottedCircle (25cc)
=> for interpolation purposes corrected starting points of inner circle of the degree symbol (00b0) and did also some spacing correction but left the forms this bit sick
=> removed 20 glyphs named *.inferior and *.superior (with dot in name) and left *inferior and *superior (without dot in name) and adapted the feature tables accordingly

So i updated the repo and provided a new build named v1.44

Hope that works for you. Best Regards, Jörg

@kenmcd
Copy link

kenmcd commented May 29, 2024

So i updated the repo and provided a new build named v1.44

No variable fonts.
and
No Medium (500).

Like the font!

@emmamarichal
Copy link
Contributor Author

Thanks a lot @jrgdrs! :) I sent you a PR, with the fonts exported. I let you read my comment!

@jrgdrs
Copy link
Owner

jrgdrs commented May 30, 2024

Hello @emmamarichal,

did the merge but now the build exits with an error at building the variable font variants:

fontmake: Error: In 'master_ufo/Wittgenstein.designspace': Generating fonts from Designspace failed: 

Couldn't merge the fonts, because some values were different, but should have
been the same. This happened while performing the following operation:
GPOS.table.FeatureList.FeatureRecord[0].Feature.LookupCount

The problem is likely to be in Wittgenstein Black:
Expected to see .LookupCount==3, instead saw 2

Got the error also in the past, that's why I rolled back in the past and configured just without "buildVariable". I have no clue what's the reason. Tried also to use hints from googlefonts/fontmake#894 and equalized the zero-width-joiners with but without success. Do you have an idea or is it necessary for me to try an exercise as ufo investigator?

Did also some spacing adaptions at the glyphs spacing after merging, so pls use the last version in repo.

Best Regards, Jörg

@jrgdrs
Copy link
Owner

jrgdrs commented May 31, 2024

Hello @emmamarichal,

Thank you, that's brilliant, the script has solved the knot. I have merged your pull request and deployed release 1.5.

Best regards and have a nice weekend, Jörg

@jrgdrs jrgdrs closed this as completed Jun 13, 2024
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

No branches or pull requests

3 participants