dont use bitmap and tilegrid background for builtin fonts #71
Conversation
|
Sounds like you have made good progress. I will check it out and let you know if I see any issues. |
|
This is passing all of the CI checks now. I tested out a few other basic scripts and some of the pyportal projects that use text everything is working correctly as far as I can tell. |
|
I reviewed the code and for the BDF-style proportional fonts, the bounding box is mis-shapen when using negative padding (see on the ascenders/descenders above). I think this is because you updated the A possibly related note: I have found a typeface that is script and the bounding boxes are overlapping. In essence all the glyphs are angled and they bunch up, so the rectangular bounding boxes are quite large with a lot of empty space in them. So, for this kind of script font you have to be careful with drawing anything other than the "ink" parts of the text. You will see that with your current code the backgrounds "overlap" some of the inked text (see the 'f' in fox for a good example). I think that if you apply the fix above you will resolve this. Let me know if you want a sample of this script font for your tests (Fayette), and I will figure out how to get it to you. Fayette_64 without background Otherwise, everything else is ok with my tests. I tried this version with:
If you make any edits and want me to check it out, I'll be glad to take another look. Thanks for making these updates. I hope it will speed up for using the BuiltinFont. |
|
@kmatch98 good catch, thank you for finding that! I think the latest commit should fix it by checking the type in the constructor before setting the background palette color. |
adafruit_display_text/label.py
Outdated
| @@ -103,8 +103,9 @@ def __init__( | |||
| self.palette[0] = 0 | |||
| self.palette.make_transparent(0) | |||
| else: | |||
| self.palette[0] = background_color | |||
| self.palette.make_opaque(0) | |||
| if isinstance(self.font, BuiltinFont): | |||
There was a problem hiding this comment.
Three things, the first I know how to fix, the second needs more looking, and third item is minor.
- When I ran this, the palette is opaque (black), but should be transparent. For BDF loaded fonts,
palette[0]should always be transparent.
I changed starting at line 101 to this:
self.palette = displayio.Palette(2)
if isinstance(self.font, BuiltinFont) and (background_color is not None):
self.palette[0] = background_color
self.palette.make_opaque(0)
else:
self.palette[0] = 0
self.palette.make_transparent(0)
self.palette[1] = color
- Font changing has background errors: When I start with BuiltinFont and then change to BDF fonts, then back and forth, the background doesn't update properly. I am using
padding_****=-4to better highlight these errors.
I think it has to do with palette not updating between the two font types. (Maybe move the palette updates from the __init__ into the function _update_background_color?) At the bottom is the code I'm using for debugging.
- I was looking through the
fontsetter, and it looks like this line is unnecessary:
self.height = self._font.get_bounding_box()[1]
For item #2 above, here is my debug code for changing the fonts:
"""
This examples shows the use color and background_color
"""
import time
import board
import displayio
import terminalio
# from adafruit_st7789 import ST7789
from adafruit_ili9341 import ILI9341
from adafruit_display_text import label
from adafruit_bitmap_font import bitmap_font
# Setup the SPI display
print("Starting the display...") # goes to serial only
displayio.release_displays()
spi = board.SPI()
tft_cs = board.D9 # arbitrary, pin not used
tft_dc = board.D10
tft_backlight = board.D12
tft_reset = board.D11
while not spi.try_lock():
spi.configure(baudrate=32000000)
spi.unlock()
display_bus = displayio.FourWire(
spi,
command=tft_dc,
chip_select=tft_cs,
reset=tft_reset,
baudrate=32000000,
polarity=1,
phase=1,
)
print("spi.frequency: {}".format(spi.frequency))
DISPLAY_WIDTH = 320
DISPLAY_HEIGHT = 240
# display = ST7789(display_bus, width=240, height=240, rotation=0, rowstart=80, colstart=0)
display = ILI9341(
display_bus,
width=DISPLAY_WIDTH,
height=DISPLAY_HEIGHT,
rotation=180,
auto_refresh=True,
)
display.show(None)
# font=terminalio.FONT # this is the Builtin fixed dimension font
#font = bitmap_font.load_font("fonts/BitstreamVeraSans-Roman-24.bdf")
font = terminalio.FONT
font2 = bitmap_font.load_font("fonts/Arial-16.bdf")
#font = bitmap_font.load_font("fonts/Fayette-HandwrittenScript-64.bdf")
text = []
text.append("none") # no ascenders or descenders
text.append("pop quops") # only descenders
#text.append("MONSTERs are tall") # only ascenders
#text.append("MONSTERs ate pop quops") # both ascenders and descenders
#text.append("MONSTER quops\nnewline quops") # with newline
display.auto_refresh = True
myGroup = displayio.Group(max_size=6)
display.show(myGroup)
text_area = []
myPadding = -4
for i, thisText in enumerate(text):
text_area.append(
label.Label(
font,
text=thisText,
color=0xFFFFFF,
background_color=0xFF00FF,
background_tight=False,
padding_top=myPadding,
padding_bottom=myPadding,
padding_left=myPadding,
padding_right=myPadding,
max_glyphs=60, ##
scale=1
)
)
this_x = 10
this_y = 10 + i * 40
text_area[i].x = 10
text_area[i].y = 3 + i * 50
text_area[i].anchor_point = (0, 0)
text_area[i].anchored_position = (this_x, this_y)
myGroup.append(text_area[i])
print("background color is {}".format(text_area[0].background_color))
test_string='The quick brown fox'
starti=len(test_string)
i=starti
while True:
#time.sleep(2)
text_area[0].font=font
text_area[0].text = test_string # change some text in an existing text box
# Note: changed text must fit within existing number of characters
# when the Label was created
#time.sleep(2)
text_area[0].background_color=None
time.sleep(1)
text_area[0].text = 'None'
text_area[0].background_color=0xFF00FF
time.sleep(1)
text_area[0].font=font2
time.sleep(1)
#i=i-1
#if i <= 0:
# text_area[0].text = 'None Test'
# i=starti
# time.sleep(1)
|
I added a couple of comments, I added it code review mode, first time I did that, so I'm not sure if I used it the way it's supposed to be used. Anyway, ping me if my comments aren't clear. |
…nd correctly in init
|
@kmatch98 I've added your fix inside of I think your example code is behaving as expected now. |
|
I reviewed these changes and verified the fixes to the issues I identified. It did make the font setter a little more complicated, but you figured it out. Keep up the good work! |
|
Why are you changing behavior based on the font type? They both do work interchangeably right? Would it be better to have configuration for the type of background box? Per-glyph or overall? Thanks for all of your work on this! |
|
@tannewt using the bitmap background is required for the background color to work correctly with custom font files. Once we made the change to using bitmap for the background color, KJW pointed out that it resulted in lower performance when moving the label around the screen. With the bitmap it takes longer to draw so it appears to move slower visually. Without the bitmap it appears to draw faster visually. This change makes it so that if you use a built-in font it will not use the bitmap (since it's not required in order to look correct). That way you get back the drawing performance in that case. But it keeps the bitmap background when it's needed due to a custom font being used. In some cases user doesn't have much of a choice. If they want to use a custom font and have a background color that looks correct for instance they must use the bitmap background. However I will say there can be some slight visual differences between the bitmap background vs. the original background strategy depending on the string in the label. Especially with multiline labels I've noticed the original background looks different (wraps each line individually instead of the entire box). So as you mentioned I do think it's worthwhile to give the user more direct selection of the background strategy. Really the main additional case that I think would be good to cover that we aren't now is if the user wants to use the built-in font, and they want the bitmap style behavior with multi-lines, and they don't care about the performance hit of the bitmap. Currently we don't have a way for them to do this with the label by itself. |
What was broken with custom font files? I don't understand the difference between built in and custom fonts. They both produce two color bitmaps for each glyph. The performance issue is likely because you'll double refresh the screen if you have a bitmap and tilegrids for each character. |
|
@tannewt #49 has more info and an example photo of how the previous background color strategy was working with custom fonts. Essentially it created a visible background box for every glyph with margin showing between all of them, rather than 1 single box that encompassed everything. Explicitly creating a bitmap for the background color and adding it to the group was the solution we used to allow custom fonts to be able to have background colors that looked correct. |
|
@tannewt The structure of In short here is what I see as the differences: In contrast, the custom loaded BDF fonts can have variable height, width and spacing for each glyph. So to accommodate proportional fonts, each glyph is stored somehow in its own bitmap. So, we can’t use a TileGrid in the same way it is used for the Regarding background boxes, the BuiltinFont is relatively easy to deal with since we can just adjust the Background color in the TileGrid palette. In contrast, with variable sized glyphs we chose to make a separate rectangle Bitmap behind the text that can be sized based on the glyph sizing (with some added options for “tight” background boxes and adjustable “padding”). [one last clarification: The current version of As for the performance impact, it’s surprising that adding one more bitmap layer behind the text has such an impact. Anyway, I thought I had mentioned the difference in the Font type construction in another issue but I couldn’t find it, so I thought I should document it here, even if only tangentially related to the topic at hand. |
|
@FoamyGuy regarding issue #44. In advance of starting any work on I have made revisions to the main branch that provide the ability to define the I considered submitting a PR vs the main branch, but I tried to compare my update versus the main branch and I didn't understand why it said there were so many changes. If you are amenable, we can merge this with your version with the background fixes. Alternately, since my changes are relatively simple, we can wait until later once your background PRs are merged. |
|
@kmatch98 I think it is best to keep anchor point changes separate so this PR won't have to get any bigger. I can help try to figure out what's up with the diff. |
|
Ok I think that’s best. I can deal with the diff after your next merge. |
|
I believe the new commit should resolve #72 |
Ah! This is how built in fonts work too! You just can't tell because it is monospaced and a fixed size. I wouldn't treat BuiltInFont specially because it'll make the code more complicated than need be. Instead, use a separate background object for it too. Then, focus on optimizing the background object so all text is better off. |
| if not isinstance(self.font, BuiltinFont): | ||
| if self._background_color: | ||
| self._update_background_color(self._background_color) |
There was a problem hiding this comment.
| if not isinstance(self.font, BuiltinFont): | |
| if self._background_color: | |
| self._update_background_color(self._background_color) | |
| if not isinstance(self.font, BuiltinFont) and self._background_color: | |
| self._update_background_color(self._background_color) |
| if ( | ||
| isinstance(new_font, BuiltinFont) | ||
| or not isinstance(new_font, BuiltinFont) | ||
| and self.background_color is None | ||
| ): |
There was a problem hiding this comment.
| if ( | |
| isinstance(new_font, BuiltinFont) | |
| or not isinstance(new_font, BuiltinFont) | |
| and self.background_color is None | |
| ): | |
| if self.background_color is None: |
Okay I will try out vecotrio polygons to see what the performance is like.
Okay, I think we can probably close this PR then. The bulk of this PRs changes were specifically for treating built-in fonts differently in order to get back the performance that was possible previously when moving the label around the screen. I will make a separate PR with a fix for #72 |
This change should revert back to the old background behavior without bitmap and tilegrid when
terminalio.FONT(or any BuiltinFont) is in use.I tested briefly with Kevin's circle animation script form here: https://github.com/kevinjwalters/circuitpython-examples/blob/master/clue/clue-label-test.py but modified to use only a single label and library instance.
It seems like this does bring back the previous animation performance.
I still intend to do some more thorough testing over the next few days. This almost certainly still needs black, pylint and perhaps sphinx checks.
@kmatch98 and/or @kevinjwalters if you have a moment please check out this version and let me know what you think.