Closed deshipu closed 2 years ago
Thank you for the PR. Would you feel comfortable adding the type annotations?
Here are some examples of the annotations: https://github.com/adafruit/Adafruit_CircuitPython_ImageLoad/blob/main/adafruit_imageload/pnm/pbm_ascii.py#L30-L36
@matt-land I intentionally didn't add type annotations, because I think they make the code much harder to read and are very wasteful on memory-constrained systems such as CircuitPython. Plus, I really don't see the point of adding them to a function that pretty much takes and returns arbitrary types.
@matt-land Speaking of which, I think the example annotations you linked to are wrong. First, the function takes classes as the arguments, and returns the instances of those classes, and your annotations say it both takes and returns instances, second, and more importantly, the classes from displayio are not the only classes you can pass to them – in fact, the whole point of passing the classes is that you can use any type you want, as long as it has the same protocol. So not only are those annotations wasteful, but they are also wrong, and will raise errors on perfectly good code.
I think they should be removed.
I intentionally didn't add type annotations, because I think they make the code much harder to read and are very wasteful on memory-constrained systems such as CircuitPython
Note that the MicroPython core simply discards type annotations. They are not in the compiled bytecode. It is true that the boilerplate conditional import
of libraries for annotation will add somewhat to the mpy
size, but the imports are not executed in CircuitPython (or shouldn't be if the code is written right) and don't add to RAM consumption.
Even if it adds only a little, I'm working on systems where a dozen bytes of flash space makes a difference, and as evidenced by the example linked, those type annotations don't actually prevent the exact kind of error they were supposed to prevent. They are harmful, at least in this particular case, where you would need to write quite a bit of code using Protocol
to actually do the check correctly.
Even if it adds only a little, I'm working on systems where a dozen bytes of flash space makes a difference, and as evidenced by the example linked, those type annotations don't actually prevent the exact kind of error they were supposed to prevent. They are harmful, at least in this particular case, where you would need to write quite a bit of code using
Protocol
to actually do the check correctly.
Let's not argue over the value of type annotations here. @matt-land's request was reasonable given that this library and many of our others have type annotations. @deshipu you are welcome to maintain your own version without type annotations for your constrained case. In CircuitPython libraries, they bring value in editor convenience and validation. If they are incorrect, then we should fix them, not throw them out completely.
If you want to hand that out to me @deshipu, let me know!
Just revisiting this, let me know if you want to hand that over to me, either to add to this PR or add to a follow up one.
Thanks @deshipu for working on this! It is great to have support for PNG images on the horizon!
The only feedback I have really is that it may be nice to include an example in the examples dir that shows how to use the new functionality, I'm happy to work on that as well. I am willing to help however I can to keep this moving along. I am excited about the new functionality.
I tested this successfully on a Hack Tablet (Adafruit CircuitPython 8.0.0-alpha.1-58-g2fbafdd36-dirty on 2022-08-19; ESP32-S3-DevKitC-1-N8R8 with ESP32S3
) Including with PNGs that contain transparency.
I tested using these images: png_imageload_test_files.zip
and this code file:
import time
import busio
import board
import adafruit_focaltouch
import displayio
import dotclockdisplay
import framebufferio
from rainbowio import colorwheel
import adafruit_imageload
import vectorio
import rainbowio
from displayio_annotation import Annotation
time.sleep(2)
box_size = 50
corner_box_size = 20
displayio.release_displays()
SCL_pin = board.IO41 # set to a pin that you want to use for SCL
SDA_pin = board.IO42 # set to a pin that you want to use for SDA
IRQ_pin = board.IO40 # select a pin to connect to the display's interrupt pin ("IRQ")
datapin_list = [
board.IO3, board.IO4, board.IO5, board.IO6,
board.IO7, board.IO8, board.IO9, board.IO10,
board.IO11, board.IO12, board.IO13, board.IO14,
board.IO15, board.IO16, board.IO17, board.IO18,
]
fb = dotclockdisplay.DotClockFramebuffer(
width=800, height=480,
hsync=board.IO47, vsync=board.IO48,
de=board.IO45,
pclock=board.IO21,
data_pins=datapin_list,
pclock_frequency=24 * 1000 * 1000,
hsync_back_porch=100, # 100
hsync_front_porch=40,
hsync_pulse_width=5,
vsync_back_porch=25,
vsync_front_porch=10,
vsync_pulse_width=1,
pclock_active_neg=1,
bounce_buffer_size_px=1000 * 15,
)
print('Finished creating DotClockFrameBuffer.')
print('Creating FramebufferDisplay...')
display = framebufferio.FramebufferDisplay(fb)
display.force_refresh = True
print(display.force_refresh)
main_group = displayio.Group()
display.show(main_group)
palette = displayio.Palette(1)
palette[0] = 0x125690
circle = vectorio.Circle(pixel_shader=palette, radius=25, x=25, y=25)
main_group.append(circle)
second_image, second_palette = adafruit_imageload.load("adabot_png.png")
second_tile_grid = displayio.TileGrid(second_image, pixel_shader=second_palette)
second_palette.make_transparent(0)
main_group.append(second_tile_grid)
print(f"before {time.monotonic()}")
image, palette = adafruit_imageload.load("blinka_test_png_transparency.png")
tile_grid = displayio.TileGrid(image, pixel_shader=palette)
tile_grid.x = 0
tile_grid.y = 0
palette.make_transparent(0)
main_group.append(tile_grid)
print(f"after {time.monotonic()}")
while True:
pass
I re-tested this with a more basic example on the Feather S2 TFT here is the test code:
import board
import displayio
import adafruit_imageload
from vectorio import Rectangle
display = board.DISPLAY
image, palette = adafruit_imageload.load("test_image.png")
palette.make_transparent(0)
bg_palette = displayio.Palette(1)
bg_palette[0] = 0xffffff
rect = Rectangle(pixel_shader=bg_palette, width=display.width, height=display.height, x=0, y=0)
tile_grid = displayio.TileGrid(image, pixel_shader=palette)
tile_grid.x = display.width // 2 - tile_grid.tile_width // 2
tile_grid.y = display.height // 2 - tile_grid.tile_height // 2
group = displayio.Group()
group.append(rect)
group.append(tile_grid)
board.DISPLAY.show(group)
while True:
pass
I'm going to merge this one now and then work on a follow up PR with the type annotations and this example included.
This is a first stab at adding PNG support. For now there are no filters, so only indexed images will work.