robotools / fontMath

A collection of objects that implement fast font, glyph, etc. math.
MIT License
42 stars 17 forks source link

Check if height is set #322

Closed schriftgestalt closed 3 months ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #322 (7b9494b) into master (777dd88) will decrease coverage by 0.08%. Report is 2 commits behind head on master. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage   89.50%   89.43%   -0.08%     
==========================================
  Files          13       13              
  Lines        2354     2356       +2     
  Branches      301      303       +2     
==========================================
  Hits         2107     2107              
  Misses        178      178              
- Partials       69       71       +2     
Flag Coverage Δ
unittests 89.38% <50.00%> (-0.08%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
Lib/fontMath/mathGlyph.py 84.36% <50.00%> (-0.35%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

anthrotype commented 1 year ago

not sure what you're doing, but in UFO glyphs height is never None, it defaults to 0 (I know that is arguable but I don't think fontMath is the place to fix this)

schriftgestalt commented 1 year ago

glyphsLib (or fontMake or whatever) is not setting it correctly in all cases. And I still think it should be able to be None (I know we discussed this).

anthrotype commented 1 year ago

both ufoLib2 and defcon defaults height to 0

https://github.com/fonttools/ufoLib2/blob/45597f82b7f2fa8f61748eed0cb29c6fabd07c48/src/ufoLib2/objects/glyph.py#L70 https://github.com/robotools/defcon/blob/32c956cabe3ca6aaeea69ddeaade5c6d0903ae91/Lib/defcon/objects/glyph.py#L123

can you check what type of glyph objects are being passed on to MathGlyph?

schriftgestalt commented 1 year ago

As far as I can see, those are ufoLib2.objects.glyph.Glyph.

anthrotype commented 1 year ago

that's weird, that line I linked above ensures the height is initialized to 0 if not set. Something else might be setting it to None... glyphsLib?

schriftgestalt commented 1 year ago

I’ll dig into this.

schriftgestalt commented 1 year ago

I had a messed up ufoLib2. it had this:

height: Optional[float] = None

instead of

height: float = 0

I think it should be like I had it, but I’m quiet now.

benkiel commented 3 months ago

Closing per discussion above.