check ascent for None before returning

[FoamyGuy]

With recent changes to bitmapt_font we started using ascent and descent values from the font if they exist. At the time wasn't sure of a font that didn't have those options and it didn't occur to me that I could delete them easily for testing.

This is a modified version of the font with ascent and descent removed. LeagueSpartan-Bold-16_mod.bdf.txt

It turns out our check for having those was insufficient in some cases. We were using hasattr but this seems to still return True for fonts without these values, but it's value is None which raises an exception when an attempt to divide it is made.

These changes resolve the issue by checking for None in addition to hasattr. So it will correctly try to fallback to measuring them using the chosen characters.

I tested this fix successfully with Blinka_Displayio, and PyPortal and the font linked above.

[jepler]

Thanks for looking into this.

On the one hand, if you're actually going to worry about this, then you should check for ascent and descent properties separately.

On the other hand, unless "in the wild" files exhibit this, you're better off just not adding the code. I can delete any lines I care to from the middle of .bdf files but not all the deletions merit special error recovery paths in display_text.

[FoamyGuy]

I was looking into the font where I first ran into this issue with and it I believe it was one I used this tool to convert: https://github.com/bcr/blinka-cli

I ran this command:

blinka font --printable --fontpath LeagueGothic-Regular.ttf --fontsize 22

and I end up with a bdf that does not have ascent and descent. which results in the error:

code.py output:
Traceback (most recent call last):
  File "code.py", line 16, in <module>
  File "/lib/adafruit_display_text/label.py", line 136, in __init__
  File "/lib/adafruit_display_text/label.py", line 270, in _update_text
TypeError: unsupported types for __floordiv__: 'NoneType', 'int'

The fix in this PR allows it to run successfully.

Perhaps it would be more appropriate to try to fix it in that tool instead of in display_text library.

I'm probably not familiar enough with different fonts to know whether it's likely to find more fonts like this in the wild. The font does seem to work fine in spite of not having these values (with the updated code). Is there a BDF spec somewhere that we could reference to figure out whether these values are required? Maybe we should be raising an exception if they are absent?

I do think as long as we do have the fallback code it would make sense to update the if statement like this so that it will get used. As it is now I think it may never use the fallback code unless there is some way for the font to get generated by bitmap_font with hasattr being False maybe that would happen from the old version of bitmap_font?

Definitely agree that checking for both would be better as well. I'll make a new commit to check for descent also tonight.

[jepler]

Thanks for the additional background.

This font is also unable to be processed by https://adafruit.github.io/web-bdftopcf/, which gives the error

reading input font
BDF Error on line 26: "STARTPROPERTIES 19" followed by only 17 properties
bdf input, corrupt

this could come from editing the file manually. When I corrected the number 19 to 17,

BDF Error on line 26: missing 'FONT_ASCENT' or 'FONT_DESCENT' properties
bdf input, corrupt

It might be worth talking to @bcr to find out whether they're interested in updating their utility to work with the "modern" font requirements we're introducing in CircuitPython.

[bcr]

blinka-cli definitely does not currently emit FONT_ASCENT or FONT_DESCENT (I don't think it emits any properties.) This is certainly something that can be added. If anyone would like to suggest a best practices document for BDF files I would certainly use it.

[FoamyGuy]

The font linked above was the one I edited by hand after figuring out missing ascent/descent was the issue, but before realizing where I got the original problem font that I discovered this with.

This one came direct from the CLI tool. It seems to omit the STARTPROPERTIES section. League-Gothic-Regular-22.bdf.txt It also has an error when trying to convert with that converter.

Looking into the history of this format a bit it looks like maybe Adobe's original spec may not have included some of these extra properties and they were added later by X11 team perhaps?

In the long run if we can count on these being very likely included in fonts we could shed this fallback behavior altogether to reduce the complexity of the library and save a bit of space.

[FoamyGuy]

@bcr I found this one which I think this is the doc for the version of this font that introduced these properties: https://www.x.org/docs/BDF/bdf.pdf

[bcr]

I summarized my thoughts so far on BDF here https://github.com/bcr/blinka-cli/blob/master/blinka/commands/font.md -- basically the approach I took was "look at the CircuitPython source and reverse-engineer the properties that are used." For some reason ascent and descent were added, so I think that blinka-cli should definitely be updated.

As far as what is "canon" as far as specs go, yeah, your reference is probably as good as any. Indeed, there is an ancient (1993) Adobe spec about this, and then some some subset of information on Wikipedia.

My general approach has been to adhere to common practice in the target field (like for instance, CircuitPython won't process a font without an M in it because of metrics reasons. I don't think the BDF spec requires a font have an M.)

In this case it seems like the BDF use in CircuitPython changed, so we should implement the change in blinka-cli.

[FoamyGuy]

Yep, this change was very recent, in fact it was originally motivated by wanting to use a font without those characters. I think this will ultimately allow us to get away from the character requirements like M j ' altogether.

[jepler]

Thanks for chiming in, @bcr!

I'm the one who intially pushed for the addition of ascent/descent properties (while @FoamyGuy got it over the finish line and has continued fixing problems with it, awesome and thank you!, and also the newer support for the PCF font format in CircuitPython.

The PCF font format may be specific to the X Window System, and I tend to view "whatever the X utility program bdftopcf accepts" as the true definition of what bdf is. However, that point of view is too inflexible. It's also worth looking at what other bdf creators such as FontForge do; I added support for what appear to be several fontforge-isms not supported in standard bdftopcf to web-bdftopcf...

[bcr]

I think we can all agree that @FoamyGuy ends up doing the heavy lifting for us :laughing:

At the end of the day, I'd like to emit fonts that are the best fit for CircuitPython. I will look closer at PCF, I can probably emit that directly also. I don't particularly care what format we use -- for CircuitPython I think we have some size constraints that are interesting to optimize, and it looks like PCF is a better idea.

In the short term I can make sure that at least the BDF to PCF converter works with the BDF files I make, and we'll make sure CircuitPython works with the BDF files I make also.

[kmatch98]

Fully agreed on point 1 above!

[bcr]

This should fix the ascent and descent properties on the blinka-cli side https://github.com/bcr/blinka-cli/pull/17

[FoamyGuy]

I tested this the new cli version this morning successfully. I'm not aware of any other sources of fonts without these properties. I'm inclined to close this for now with an eye towards removing the fallback altogether if we don't have any other folks having trouble pop up.

Thank you all!