search

adafruit / Adafruit-GFX-Library

Adafruit GFX graphics core Arduino library, this is the 'core' class that all our other graphics libraries derive from
https://learn.adafruit.com/adafruit-gfx-graphics-library
Other
2.4k stars 1.55k forks source link

broken opaque text on many GFX drivers #384

Closed BillyDonahue closed 2 years ago

BillyDonahue commented 2 years ago

(New issue to boost my late comment on PR #383)

Does this change break existing Adafruit_GFX-derived classes? It kind of looks like it does.

Those classes won't have overrides for the new setAddrWindow and writeColor, but the base class implementation of these functions do nothing.

So writing opaque text with the GFX internal font should, I think, not work after this change? You'd need a GFX subclass that implements these new virtual functions if you want opaque text to continue to work.

_Originally posted by @BillyDonahue in https://github.com/adafruit/Adafruit-GFX-Library/pull/383#discussion_r823180537_

makermelissa commented 2 years ago

Do you have some example code to demonstrate this?

BillyDonahue commented 2 years ago

Hi Melissa.

No I don't have repro code I just saw the code as it came through and reasoned about it.

Sorry, I don't have a dev setup going at the moment. I could put something together if you feel it's necessary. But maybe I can convince you just from looking at the code.

Clearly the base class implementations of the addrSetWindow and writeColor do nothing at all.

https://github.com/adafruit/Adafruit-GFX-Library/blob/2fbbfa224f6c79db9a64d754837818caa8f3d7a6/Adafruit_GFX.cpp#L259-L262

https://github.com/adafruit/Adafruit-GFX-Library/blob/2fbbfa224f6c79db9a64d754837818caa8f3d7a6/Adafruit_GFX.cpp#L274-L275

If we follow the relevant part of drawChar, its only side effects are calls to those two functions. If those functions do nothing, then drawChar does nothing.

https://github.com/adafruit/Adafruit-GFX-Library/blob/2fbbfa224f6c79db9a64d754837818caa8f3d7a6/Adafruit_GFX.cpp#L1181-L1192

The only way this works is if a GFX-derived class overrides those functions. I see that Adafruit_SPITFT has had functions called writeColor and setAddrWindow for a couple of years. But now they both are overrides of Adafruit_GFX base member functions. What's happening is that Adafruit_SPITFT-derived devices will work because they have functional overrides of the functions called by drawChar, but all other devices like SSD1306 or the GrayOLEDs or GFXCanvases are all broken.

makermelissa commented 2 years ago

It's not about convincing. I was just wondering if you had example code that I could just run because that makes it easier to fix the issue. I did actually run the graphicstest example code on 3 different boards before merging and didn't see what you were describing.

ladyada commented 2 years ago

hmm - @makermelissa did ya try an OLED?

makermelissa commented 2 years ago

No I didn't. I'll give that a try today.

makermelissa commented 2 years ago

Just tested and It works fine on the SSD1351.

ladyada commented 2 years ago

SSD1351 is a windowed 'SPITFT' subtype. use a monochrome OLED

makermelissa commented 2 years ago

Ok, will do.

makermelissa commented 2 years ago

Ok, after trying to get an OLED working and finally figuring out I had put it in I2C mode, I was able to test this and compare to the code before the last change. The only thing I noticed is when the demo prints 3.141592, it didn't appear with the latest changes, so there is something.

ladyada commented 2 years ago

whats it look like?

makermelissa commented 2 years ago

Before Changes: IMG_4006

After Changes: IMG_4003

makermelissa commented 2 years ago

Everything else looks fine.

BillyDonahue commented 2 years ago

Ok, after trying to get an OLED working and finally figuring out I had put it in I2C mode, I was able to test this and compare to the code before the last change. The only thing I noticed is when the demo prints 3.141592, it didn't appear with the latest changes, so there is something.

Ok that's the bug then. The "3.141592" is the only text in the demo that uses a bgcolor.

https://github.com/adafruit/Adafruit_SSD1306/blob/master/examples/ssd1306_128x32_i2c/ssd1306_128x32_i2c.ino#L315-L332

There's another bug here, and maybe it's worth breaking out into a separate Github Issue, but it's caused by this same PR, so I'll mention it.

BUG: This new opaque text drawing loop doesn't take screen rotation into account. The old code always went through writePixel or writeFillRect which ultimately become drawPixel calls, and drawPixel applies the screen rotation transform to the (x,y) input coordinates:

https://github.com/adafruit/Adafruit-GFX-Library/blob/master/Adafruit_GFX.cpp#L1837-L1853

The writeColor functions in existing drivers don't apply screen rotation. I don't think it's expected of them, and writeColor is meant as more of a lower-level transaction primitive, I think. Anyway, the test demo doesn't actually try different screen rotations, so you could miss it.

makermelissa commented 2 years ago

@ladyada want me to just revert the PR or dive in and see if it's fixable?

ladyada commented 2 years ago

@makermelissa lets revert the optimization for now and re-release. more important we keep things working so the support folks don't get burdened.

cc @bitbank2

makermelissa commented 2 years ago

I never actually released with the bug, so I think a revert is probably all that's needed.

bitbank2 commented 2 years ago

Thanks for the additional testing. It makes sense that there are some incompatibilities with the monochrome OLED/LCD code because they don't support the same display memory rotation like color LCDs do. It was still worth a try :)