displayio: Pass transparent_color to ColorConverter

[jensechu]

Signed-off-by: Jensen [email protected]

Summary

Add the ability to pass in a color into the ColorConverter module so it flags a pixel as opaque=false which I think will make it use the background value (sub layer's pixel value) when rendering on the screen.

Example: displayio.ColorConverter(transparent_color=VALUEHERE)

I was unable to get this to compile so I couldn't test it as there is no type for an uint32_t declared in the types for mp_arg_t. That seems expected because it might be weird anyone is passing in a uint32_t value. My thought was perhaps to just make the transparent_color arg an int instead and then convert it and move on to the comparison to the input_pixel to set it as not opaque.

I decided to open this PR to know if my head is even in the right direction or maybe I should forego this approach. 😄

[tannewt]

Thanks for the PR @jensechu! Instead of changing the constructor, how about adding make_transparent and make_opaque like Palette has? https://github.com/adafruit/circuitpython/blob/main/shared-bindings/displayio/Palette.c#L151-L179

We'll only be able to have one color at a time but then the API will be the same.

[jensechu]

@tannewt Thanks for all of the feedback! I added in two methods.

make_transparent which takes in a pixel value to make it transparent (opaque = false).
make_opaque which also takes in a pixel value and will delete the current ColorConverter's transparent_pixel if it is the same color. I'm not sure this is an appropriate way to do it so I am open to feedback!

I compiled everything and loaded it but I still see the pixels so I am hoping with your expertise you might be able to guide me to what the next step should be. 😄 I spent the better part of yesterday trying to figure out why but alas I was not able to.

[tannewt]

I compiled everything and loaded it but I still see the pixels so I am hoping with your expertise you might be able to guide me to what the next step should be. smile I spent the better part of yesterday trying to figure out why but alas I was not able to.

Hrm, I thought it might be an issue with what calls the ColorConverter but the code looks right: https://github.com/adafruit/circuitpython/blob/main/shared-module/displayio/TileGrid.c#L439-L450

The next step I'd do is print out the two color values in ColorConverter when a color doesn't match the transparent one. (Printing when they do match will confirm it's working too.) I suspect it could be a component ordering issue where you are giving an RGB value but it's BGR internally.

[jensechu]

@tannewt Thanks for the review!! I am so close to finishing this up but alas I ran into an error when adding the exceptions.

I keep running into:

../../shared-module/displayio/ColorConverter.c: In function 'common_hal_displayio_colorconverter_make_transparent':
../../shared-module/displayio/ColorConverter.c:133:9: error: implicit declaration of function 'mp_raise_RuntimeError' [-Werror=implicit-function-declaration]
  133 |         mp_raise_RuntimeError(translate("transparent_color value is already set"));
      |         ^~~~~~~~~~~~~~~~~~~~~
../../shared-module/displayio/ColorConverter.c:133:9: error: nested extern declaration of 'mp_raise_RuntimeError' [-Werror=nested-externs]

I am going to try and figure out how to fix this now but it wasn't immediately clear to me. I still wanted to push the rest up nonetheless. I will also re-run make translate once this is done. 👍

[jensechu]

I am not sure why I got this single failure. 😅
https://github.com/adafruit/circuitpython/pull/3529/checks?check_run_id=1245283514

[tannewt]

Here are some suggestions that should make it a bit smaller. Thanks!

[jensechu]

@tannewt Thank you again for all of your reviews. 😄 I initially kept the unused parameter but the linter was not happy with an unused parameter so I removed it. If you'd rather I use it superficially or something please let me know the best way and I can add it back in. Otherwise it is cleaned up.

[jensechu]

Also sorry my git history is a bit of a mess... I am happy to rebase and clean that up if needed. First time using the github UI to merge in suggestions. Not sure I will do that again haha.

[tannewt]

@tannewt Thank you again for all of your reviews.

No problem! Thank you for improving things. :-) (We're working on the metro m0 size issue now separately as well.)

I initially kept the unused parameter but the linter was not happy with an unused parameter so I removed it. If you'd rather I use it superficially or something please let me know the best way and I can add it back in. Otherwise it is cleaned up.

For unused parameters you can do (void) transparent_color; in the function body to pretend to use it.

Also sorry my git history is a bit of a mess... I am happy to rebase and clean that up if needed. First time using the github UI to merge in suggestions. Not sure I will do that again haha.

We're not sticklers on git history. Do whatever you'd prefer.

[jensechu]

@tannewt 😱 They all passed. Please let me know if I should change anything else and I am happy to. :)

[tannewt]

One more tiny doc improvement. Otherwise it looks really good! Thanks!

[jensechu]

I am cursed by this metro_m0_express board. 🤣

[tannewt]

@jepler any ideas besides turning off the lto partitioning?

[tannewt]

Thank you! Merging because the only failure is a known mac mpy-cross issue.

[jepler]

Thank you both!