-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
lv_img_conv_py: minimal python port of node module #1863
Conversation
Build size and comparison to main:
|
fa92ff9
to
4edfbae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall, performance is a lot better as well. Haven't tested on hardware but according to your earlier tests I trust the output
Mostly code cleanliness suggestions/doc suggestions, only architectural one is whether you want to import the file directly or run it as a subprocess
.github/workflows/main.yml
Outdated
@@ -31,6 +31,10 @@ jobs: | |||
uses: actions/checkout@v3 | |||
with: | |||
submodules: recursive | |||
- name: Install python3-pil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Install resource build dependencies" would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the docker file is updated this install should be removed as it is already done in the used docker image 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed as recommended, probably should remove in future 🤔
but keeping it for now to allow a smooth CI until docker image is updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial fixes, gonna do the rest at a later time
.github/workflows/main.yml
Outdated
@@ -31,6 +31,10 @@ jobs: | |||
uses: actions/checkout@v3 | |||
with: | |||
submodules: recursive | |||
- name: Install python3-pil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the docker file is updated this install should be removed as it is already done in the used docker image 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm far from a Python expert, so I trust you on that regard 👍
This scripts makes the setup of the build environment much easier and it works fine on my setup and in the build Docker container so... LGTM :-)
Create a minimal python port of the node.js module `lv_img_conv`. Only the currently in use color formats `CF_INDEXED_1_BIT` and `CF_TRUE_COLOR_ALPHA` are implemented. Output only as binary with format `ARGB8565_RBSWAP`. This is enough to create the `resources-1.13.0.zip`. Python3 implements "propper" "banker's rounding" by rounding to the nearest even number. Javascript rounds to the nearest integer. To have the same output as the original JavaScript implementation add a custom rounding function, which does "school" rounding (to the nearest integer) Update CMake file in `resources` folder to call `lv_img_conf.py` instead of node module. For docker-files install `python3-pil` package for `lv_img_conv.py` script. And remove the `lv_img_conv` node installation. --- gen_img: special handling for python lv_img_conv script Not needed on Linux systems, as the shebang of the python script is read and used. But just to be sure use the python interpreter found by CMake. Also helps if tried to run on Windows host. --- doc: buildAndProgram: remove node script lv_img_conv mention Remove node script `lv_img_conv` mention and replace it for runtime-depency `python3-pil` of python script `lv_img_conv.py`.
Used by script `lv_img_conv.py`, should be provided by docker image, but until then explicitly install in workflow.
b511b93
to
672e305
Compare
code review incorporated, squashed and pushed, gonna merge as soon as possible |
User MorsMortium informed me on Matrix, that there is a full port available at https://github.com/W-Mai/lvgl_image_converter Still using this minimum port, and if we need more we could either switch to the full port or use the full port as inspiration to implement the newly needed parts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
Install new build dependency introduced with move to minimum python implementation of `lv_img_conv` node js script InfiniTimeOrg/InfiniTime#1863
Install new build dependency introduced with move to minimum python implementation of `lv_img_conv` node js script InfiniTimeOrg/InfiniTime#1863
Install new build dependency introduced with move to minimum python implementation of `lv_img_conv` node js script InfiniTimeOrg/InfiniTime#1863
Create a minimal python port of the node.js module
lv_img_conv
. Onlythe currently in use color formats
CF_INDEXED_1_BIT
andCF_TRUE_COLOR_ALPHA
are implemented.Output only as binary with format
ARGB8565_RBSWAP
(andARGB8888
for debugging).This is enough to create the
resources-1.13.0.zip
.In the simulator the pinecone in Casio is visible, and the navigation symbol is visible as well in the simulator when the resource zip is loaded
The new
lv_img_conv.py
script needspython3-pil
python module as runtime dependency.The runtime dependency is added to the docker container image.