Large font support #3332
Large font support #3332
Conversation
There was a problem hiding this comment.
Font with height 56 lines may be unjustified (having matrix with height larger than 56 lines is very uncommon and requires special handling) as it eats plenty of flash storage.
I also consider reading directly from flash memory a bad practice (Espressif recommends using pgm_read…()).
And finally since code size for regular WLED, which may include a usermod or two, is already reaching beyond 95% of file system size I'd like to see this enhancement as a custom compile option.
wled00/FX.cpp
Outdated
| case 6: letterWidth = 12; letterHeight = 16; break; | ||
| case 7: letterWidth = 12; letterHeight = 24; break; | ||
| case 8: letterWidth = 16; letterHeight = 32; break; | ||
| case 9: letterWidth = 25; letterHeight = 57; break; |
There was a problem hiding this comment.
I see no reason to use characters with height above 32 as matrices with such dimensions become unmanageable.
Also font memory footprint becomes very large and we want to avoid that.
wled00/FX_2Dfcn.cpp
Outdated
| case 48: bits = pgm_read_byte_near(&console_font_6x8[(chr * h) + i]); break; // 6x8 font | ||
| case 63: bits = pgm_read_byte_near(&console_font_7x9[(chr * h) + i]); break; // 7x9 font | ||
| case 60: bits = pgm_read_byte_near(&console_font_5x12[(chr * h) + i]); break; // 5x12 font | ||
| case 24: bits = &console_font_4x6[(chr * h * num_bytes) + (i * num_bytes)]; break; // 5x8 font |
There was a problem hiding this comment.
The idea to manipulate flash memory directly is bad and against recommended practices. Use bits as it was intended - a regular variable, not pointer.
| int wb = w % 8; | ||
| if(wb == 0) | ||
| wb = 8; // get width of the first byte to process | ||
| for (int k = 1; k <= num_bytes; k++) { // loop through all bytes of the character |
There was a problem hiding this comment.
Please check fx-blending branch to see planned enhancements to scrolling text and drawCharacter() function and accomodate that.
You may need to wait until fx-blending is merged to update this PR.
| #include "src/font/console_font_12x16.h" | ||
| #include "src/font/console_font_12x24.h" | ||
| #include "src/font/console_font_16x32.h" | ||
| #include "src/font/console_font_25x57.h" |
There was a problem hiding this comment.
You may want to make large fonts optional compile.
Please accommodate that.
| void Segment::drawCharacter(unsigned char chr, int16_t x, int16_t y, uint8_t w, uint8_t h, uint32_t color, uint32_t col2) { | ||
| if (!isActive()) return; // not active |
There was a problem hiding this comment.
Even though we added measures elsewhere this should remain here to prevent crash if segment changes during writing to it.
wled00/FX_2Dfcn.cpp
Outdated
| if (w <= 8) { | ||
| num_bytes = 1; | ||
| } | ||
| else if(w <= 16){ | ||
| num_bytes = 2; | ||
| } | ||
| else if (w <= 24){ | ||
| num_bytes = 3; | ||
| } | ||
| else if (w <= 32){ | ||
| num_bytes = 4; | ||
| } | ||
| else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
A simple switch () may be cleaner.
Removed the console_font_25x57 font, created the WLED_ENABLE_LARGE_FONTS define to conditional compile the large font code and fonts, merged changes needed from the fx-blending branch and other minor cleanup.
This reverts commit 0ddf5c4.
|
Another way to achieve huge fonts on large matrices is using Grouping 2 or 3, which will render to a smaller virtual segment and scale up. Since that approach leads to blockier character edges though, this PR is of course valid. Just pointing out there's a way to render large fonts at all without additional flash use. |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
@blazoncek @w00000dy I think we should exclude pull requests from the "auto-close" stale.yaml. Or maybe lets mark then "stale", but don't auto-close them after a week. |
softhack007
added
the
keep
Could be done like this. Let's see what @blazoncek says. |
|
Let's see if The issue with this particular PR is that it adds a few more bytes to flash footprint which may be problematic in the near future. |
By looking into the log we can see that it does not remove the So in cases like this one you would have to manually remove the |
|
@softhack007 why would you want to |
@blazoncek yes I see some potential for this PR plus the other "tiny font" PR #3337 - if its it's made optional for compile AND requested changes are implemented. At least on devices like S3 with more program space in flash. So not for genral use, but for larger fixtures, e.g. setups using HUB75 (PR #3777). That's why I feel it should not be closed yet. |
blazoncek
added
waiting for feedback
Added 4 larger font sizes by adding four console_font_XXxYY.h files and modifying the routine Segment::drawCharacter() to handle fonts that used more than 1 byte to define a horizontal row of pixels.