Skip to content
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

[BUG] Extui font-scaling fails on 320x240 #18076

Closed
RudolphRiedel opened this issue May 22, 2020 · 7 comments
Closed

[BUG] Extui font-scaling fails on 320x240 #18076

RudolphRiedel opened this issue May 22, 2020 · 7 comments

Comments

@RudolphRiedel
Copy link
Contributor

@marciot

I am playing with adding a bunch of new displays.

The font-scaling does fail with 320x240 in multiple places:

"Increment"
"Run Speed"
"Sensorless Homing"
"Extraction" and the Accleration values on the "Acceleration" screen.
The temperatures in the "Change Filament" screen.

And it is not that these are printing a little too big, these are printed way larger than anything else.

Disabling TOUCH_UI_FIT_TEXT helps but the fonts in fonts.h seem to be selected a bit large.
For font_tiny and font_xsmall I went to font number 20 but while this works it is not really viable anymore.

Overall 320x240 do not seem to be a viable option and maybe the minimum supported should be 480x272?

@marciot
Copy link
Contributor

marciot commented May 23, 2020

The font-scaling does fail with 320x240 in multiple places:

"Increment"
"Run Speed"
"Sensorless Homing"
"Extraction" and the Accleration values on the "Acceleration" screen.
The temperatures in the "Change Filament" screen.

@RudolphRiedel: It would be hackish, but we could modify the language files to shorten those strings when the higher resolution was selected. Probably not worth the effort, however.

And it is not that these are printing a little too big, these are printed way larger than anything else.

There's probably a bug in the font scaling code, which is here

Maybe it is picking the wrong font if it can't find one that fits.

Overall 320x240 do not seem to be a viable option and maybe the minimum supported should be 480x272?

Yeah, I imagine 320x240 is not really feasible for the Marlin UI.

I did intended the FTDI code to be reusable in other projects, so if you want to add support for other 320x240 displays, go ahead. I don't think it will be wasted effort necessarily, just maybe not useful in Marlin, per se.

@RudolphRiedel
Copy link
Contributor Author

There's probably a bug in the font scaling code, which is here

Maybe it is picking the wrong font if it can't find one that fits.

I had a quick look at this before I reported this but could not really figure out what is wrong there as I am not even clear on what exactly is fed into this function in the end.

Yeah, I imagine 320x240 is not really feasible for the Marlin UI.

I did intended the FTDI code to be reusable in other projects, so if you want to add support for other 320x240 displays, go ahead. I don't think it will be wasted effort necessarily, just maybe not useful in Marlin, per se.

Is it possible that this even was my idea back when I contacted you first?
I planned to put a 3.5" 320x240 in my CR-10 but now I would rather use a 4.3" or even a 5".

As I have my own code library for EVE displays that already supports a whole lot more displays I have no need for this outside of Marlin.

So only adding 4.3", 5.0" and perhaps 7.0" displays might be the better option then.

@marciot
Copy link
Contributor

marciot commented May 23, 2020

I had a quick look at this before I reported this but could not really figure out what is wrong there as I am not even clear on what exactly is fed into this function in the end.

It takes three arguments, w and h, which are the width and height of the box for the text in pixels and the text string itself. The code then loops through various font sizes, measures the text at that font size and then on line 329 checks to see whether the text fits, breaking out of the loop if it does:

for (;font >= 26;) {
        int16_t width, height;
        // Measure text and put height and width in width and height variables
        ...
        // See if the actual text size is smaller than the box that is supposed to contain it
        if (width < w && height < h) break;
        font--;
      }

Do you see the problem? The last font size it checks is 26. If it does not fit, it decrements by one and breaks out of the loop. Thus, it ends up using font size 25, which in the FTDI is actually a very big font. So the fix is to change the line that reads:

for (;font >= 26;) {

To:

for (;font > 26;) {

As I have my own code library for EVE displays that already supports a whole lot more displays I have no need for this outside of Marlin.

Sure, you don't have to, if you don't want to.

So only adding 4.3", 5.0" and perhaps 7.0" displays might be the better option then.

That makes sense to me.

@RudolphRiedel
Copy link
Contributor Author

It takes three arguments, w and h, which are the width and height of the box for the text in pixels and the text string itself.

Yes, but to figure out with what values exactly this is called is a bit tricky.

for (;font >= 26;) {
Do you see the problem? The last font size it checks is 26. If it does not fit, it decrements by one and breaks out of the loop. Thus, it ends up using font size 25, which in the FTDI is actually a very big font.

Missed that, thanks.

So the fix is to change the line that reads:

for (;font >= 26;) {

To:

for (;font > 26;) {

I tried this and it works but the strings are still a little too big.

So I tried using the original line and added this after the loop:
if(font < 26)
{
font = 20;
}

While this works it is not really better, the different font makes it ugly and not better to read.

I see to submit your fix then but right now I better not make a pull request for my currently experimental branch. :-)

My two adapter boards are getting closer to beeing released, I need another revision for both of these though.
That 3.5" is a CFAF320240F from Crystalfontz.

grafik

@boelle
Copy link
Contributor

boelle commented Jun 21, 2020

@RudolphRiedel still an issue?

@RudolphRiedel
Copy link
Contributor Author

The pull request does apply the fix Marcio mentioned here.
While this was a real issue the fix only puts a bandaid on it since the resolution of 320x240 still is too low for a couple of the longer strings.
So the real conclusion is that 320x240 is just barely viable for Extui and recommend resolutions are 480x272 and 800x480.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants