FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

generalize lv-font creation by NeroBurner · Pull Request #1131 · InfiniTimeOrg/InfiniTime · GitHub

generalize lv-font creation - #1131

Merged
Riksu9000 merged 3 commits into
InfiniTimeOrg:developfrom
NeroBurner:auto-generate-fonts-generalized
May 16, 2022
Merged

generalize lv-font creation#1131
Riksu9000 merged 3 commits into
InfiniTimeOrg:developfrom
NeroBurner:auto-generate-fonts-generalized

Conversation

Copy link
Copy Markdown
Contributor

In #1097 new font
generation capabilites were added. Generalize the font creation to
make it possible to reuse the displayapp/fonts/CMakeLists.txt file
for InfiniSim and just add the new cmake file to the project and
link against the new infinitime_fonts target.

In the following a list of changes.

Allow non-global installed lv_font_conv executable installed with

npm install lv_font_conv@1.5.2

In CMake we search for lv_font_conv executable. Add the found
executable to the python script generate.py, to remove the need for
lv_font_conv to be in the path.

Search for python3 executable, if CMake version 3.12 is available.
Otherwise use python as hard coded executable.

Instead of adding the generated fonts to SOURCE_FILES variable, create
a static library infinitime_fonts instead. Link this library to the
executables instead.

Copy link
Copy Markdown
Contributor Author

@yehoshuapw could you review my changes

And maybe also the sister PR in InfiniSim InfiniTimeOrg/InfiniSim#29

NeroBurner requested review from Avamander and Riksu9000 May 10, 2022 22:09

Riksu9000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good as far as I can tell.

yehoshuapw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

overall, it looks excellent (and the CMake much better) - thanks.
I added a few comment for consistency.

Comment thread src/displayapp/fonts/generate.py Outdated
Comment thread src/displayapp/fonts/generate.py Outdated
Comment thread src/displayapp/fonts/generate.py Outdated
Riksu9000 added this to the 1.10.0 milestone May 12, 2022

Riksu9000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I noticed that this always re-generates the font, whereas before it didn't if it wasn't necessary.

Copy link
Copy Markdown
Contributor Author

valid concern, yes I need to fix that! Thanks for pointing that out

JF002 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That looks really good to me! I like the new static library that contains all the fonts! 👍
We'll be able to merge when the remaining reviews on the Python scripts are solved :)

In InfiniTimeOrg#1097 new font
generation capabilites were added. Generalize the font creation to
make it possible to reuse the `displayapp/fonts/CMakeLists.txt` file
for `InfiniSim` and just add the new cmake file to the project and
link against the new `infinitime_fonts` target.

In the following a list of changes.

Allow non-global installed `lv_font_conv` executable installed with

```sh
npm install lv_font_conv@1.5.2
```

In CMake we search for `lv_font_conv` executable. Add the found
executable to the python script `generate.py`, to remove the need for
`lv_font_conv` to be in the path.

Search for `python3` executable, if CMake version 3.12 is available.
Otherwise use `python` as hard coded executable.

Instead of adding the generated fonts to `SOURCE_FILES` variable, create
a static library `infinitime_fonts`. Link this library to the
executables instead.

Use `add_custom_target()` together with `add_custom_command()` to
generate the font.c files once (like the original PR does).
NeroBurner force-pushed the auto-generate-fonts-generalized branch from 81d9983 to 2839978 Compare May 14, 2022 18:40

Copy link
Copy Markdown
Contributor Author

@Riksu9000 fixed the always regenerating fonts. Now its generated only once (and if the fonts.json file is modified)

NeroBurner requested a review from Riksu9000 May 14, 2022 20:24
Riksu9000 merged commit 4434083 into InfiniTimeOrg:develop May 16, 2022
NeroBurner deleted the auto-generate-fonts-generalized branch May 16, 2022 09:48
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request May 16, 2022

Copy link
Copy Markdown

and maybe it's make sense to note, that "node" version must be a fresh one...
tried to build on Ubuntu 20LTS, and faced the issue:

TypeError: this.src.glyphs.map(...).flat is not a function

and it looks like the known issue like this and this.
so, i had to update the Node to make it work :)

yehoshuapw commented May 18, 2022
edited
Loading

Copy link
Copy Markdown
Contributor

TypeError: this.src.glyphs.map(...).flat is not a function

I actually knew that, (which is why the docker installs npm's repos), and really should have kept track for such an issue.. (and added some comment somewhere about it)

Added pr #1142 , which adds this info to the docs.
Also, Just added a PR directly to lvgl/lv_font_conv#77 - which might help.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL