Skip to content

Remove usage of 0-width/height bitmaps and tilegrids #58

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

Closed
makermelissa opened this issue Jul 2, 2020 · 17 comments
Closed

Remove usage of 0-width/height bitmaps and tilegrids #58

makermelissa opened this issue Jul 2, 2020 · 17 comments

Comments

@makermelissa
Copy link
Collaborator

I discovered this when working on Blinka Displayio and looking at the circuitpython code as well, it appears a divide by zero error is happening in the background, but it's silently recovering from that. In order to avoid that, we'd like to add warnings if 0-width/height bitmaps are used, but it should be removed from here first as that would break this library.

It's being used here: https://github.com/adafruit/Adafruit_CircuitPython_Display_Text/blob/master/adafruit_display_text/label.py#L113

I believe it's also using 0-width or height later in the code as well, though I can't pinpoint the exact location.

I put a workaround in place for Blinka Displayio for now. See adafruit/Adafruit_Blinka_Displayio#27 (review) for more info.

@tannewt
Copy link
Member

tannewt commented Jul 2, 2020

@kmatch98 Any thoughts? I think you added the zero-width bitmap. Is it just setting a background color?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 2, 2020

I believe it is only for setting the background color. The way it works now was introduced because the original background_color functionality was working with terminalio.FONT but not working very well with custom fonts. #49 has more details.

I think it's set to 0 width initially because the label may not have any text yet at this point.

It looks like the bitmap and tilegrid get created inside of _create_background_box() with the box sizes here:

background_bitmap = displayio.Bitmap(box_width, box_height, 1)

It may be possible to get rid of the placeholder 0 sized bitmap and Tilegrid in the __init__() without causing any issues.

But I think inside of _create_background_box() it could still come up as 0 if there is no text in the label and no padding. We may need to add logic to handle the "no text" case separately to avoid making the zero size bitmap.

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 2, 2020

I just issued a pull request to alleviate this to prevent the zero-sized bitmap.

I also observed some strange position changes when the text is changed after a label is created. I will create a separate issue for that.

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 2, 2020

@ foamyguy I didn't add a check for zero size of the box_width and box_height before updating the background bitmap size.

Do you think that will be necessary? If so, what exception should it provide? I think I agree that handling "no text" would be a good approach, but what if you create a label with text and then set the text to blank. I feel like that shouldn't cause any error, just should display nothing. Your suggestions are welcome.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 2, 2020

@kmatch98 I do think it will be necessary.

If I understand correctly the I think the desire is that Bitmap object will be raising an exception when you try to create one with 0 width or height. @tannewt can you confirm if I am understanding correctly what the ultimate desired change is?

Inside of Label I think we will need to check if the size is going to be zero, and if so just skip making the Bitmap altogether. That way the code inside of label will never be creating a 0 size Bitmap which means we should never run into the the exception if/when it does get added to the Bitmap class. (Or perhaps we could use Try/Except but this is a bit of a chicken/egg problem because the exception hasn't been implemented yet I think.)

I do agree as well that we will want to make sure to handle it correctly no matter when or how the text gets set to empty so that no exceptions get raised based on the user setting text, even if they set it to empty.

@tannewt
Copy link
Member

tannewt commented Jul 3, 2020

@FoamyGuy Yup, I intend on raising an exception if width or height is zero.

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 9, 2020

Resolved in #59

@makermelissa
Copy link
Collaborator Author

It appears this is still an issue. Likely due to my term usage on this issue. I added a 0-width/height check to tilegrid and here's the error:

  File "/home/pi/.local/lib/python3.7/site-packages/adafruit_display_text/label.py", line 362, in text
    self._update_text(str(new_text))
  File "/home/pi/.local/lib/python3.7/site-packages/adafruit_display_text/label.py", line 283, in _update_text
    y=position_y,
  File "/home/pi/.local/lib/python3.7/site-packages/displayio/tilegrid.py", line 107, in __init__
    raise ValueError("Tile width and height must be greater than 0")
ValueError: Tile width and height must be greater than 0

@makermelissa makermelissa changed the title Remove usage of 0-width/height bitmaps Remove usage of 0-width/height bitmaps and tilegrids Jul 9, 2020
@makermelissa
Copy link
Collaborator Author

Here's the changed code in tilegrid.py:

        if not tile_width:
            tile_width = bitmap_width
        if not tile_height:
            tile_height = bitmap_height
        if tile_width < 1 or tile_height < 1:
            raise ValueError("Tile width and height must be greater than 0")

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 9, 2020

@makermelissa maybe it is a different issue than before, or we solved the wrong problem with the last PR!

Can you show the code you are running to create the label? For example, are you updating a label to have no text, label.text=‘’?

@makermelissa
Copy link
Collaborator Author

Sure, I'm basically running the code from https://learn.adafruit.com/pyportal-electioncal-us/code-pyportal-with-circuitpython with some small modifications (removed board.NeoPixel from initialization and updated code to get the current working directory in CPython) so that it runs on the Blinka PyPortal library.

@makermelissa
Copy link
Collaborator Author

Here's the full error:

Traceback (most recent call last):
  File "electioncal.py", line 37, in <module>
    gfx = electioncal_graphics.Electioncal_Graphics(pyportal.splash, am_pm=True)
  File "/home/pi/electioncal_graphics.py", line 43, in __init__
    self.url_text.text = "Visit us at https://electioncal.us"
  File "/home/pi/.local/lib/python3.7/site-packages/adafruit_display_text/label.py", line 362, in text
    self._update_text(str(new_text))
  File "/home/pi/.local/lib/python3.7/site-packages/adafruit_display_text/label.py", line 283, in _update_text
    y=position_y,
  File "/home/pi/.local/lib/python3.7/site-packages/displayio/tilegrid.py", line 107, in __init__
    raise ValueError("Tile width and height must be greater than 0")
ValueError: Tile width and height must be greater than 0

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 9, 2020

It looks like maybe this is getting caused by the glyph width here:

Perhaps the width and/or height of certain glyphs can be 0?

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 9, 2020

In the font file "Arial-12.bdf", the spaces are represented with zero-width glyphs, but use an x_offset to create the space.

So, in essence a blank tileGrid is created (with zero width), and the next tileGrid is created with an x-position offset to the right by x_offset.

One option is we can decide to not create any zero-width tileGrids.

Let me propose something....

@makermelissa
Copy link
Collaborator Author

That sounds like a workable plan. :)

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 9, 2020

Ok, just issued a PR, please check it out and see if it will resolve your tileGrid error.

@makermelissa
Copy link
Collaborator Author

Fixed by #64

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

4 participants