u8g2 icon indicating copy to clipboard operation
u8g2 copied to clipboard

Fix font_calc_vref_center off-by-one error

Open Finomnis opened this issue 3 years ago • 2 comments

The goal of vref_center is to move the font in the middle between ascent and descent.

For that, the distance between ascent and descent is calculated and divided by two. Then, descent is added to correct for the fact that we don't need to move the font from the bottom, but from the baseline.

The formula in the code is (ascent - descent) / 2 + descent.

The problem is that ascent - descent is not the height of the font, as both ascent and descent are inside the font bounding box. Just like the range 1-3 (e.g. 1,2,3) does not contain 2 values, as (3-1) would suggest. Instead, for integers, if both ends are included in the range, you need to add one.

So the improved formula is (ascent - descent + 1) / 2 + descent.

I came across this when I noticed that vertical centering seems to look 'off'. Additional to the fact that this is mathematically more precise, I feel like it does make a visual difference, especially for smaller fonts. Although it should only matter for fonts where ascent - descent is odd.

EDIT: I first closed the PR because it seemed just too ridiculous. But then I reopened it, because after a little more trial and error, I'm fairly certain that this bug causes an off-by-one error in u8g2_font_fourmat_tf, which is a 5 pixel font. And I think if it only has five pixels, being off by one is a serious problem after all.

Finomnis avatar Aug 20 '22 22:08 Finomnis

Thanks for the PR. I don't have access to my PC at the moment, so I can't test anything. However, I don't know whether it makes sense to include this PR.

  • It will break existing code
  • There is a dependency to the ref height setting which needs to be analyzed

olikraus avatar Aug 21 '22 12:08 olikraus

Yah, I figured. I'm just in the process of porting u8g2's font subsystem to Rust and stumbled across this, so I thought I'd report it here. But yah, I understand this a breaking change. Better, but not compatible with workarounds people probably implemented in the client code

Finomnis avatar Aug 21 '22 13:08 Finomnis

Closed for now as it was rejected because it would break stuff.

Finomnis avatar Jun 21 '23 20:06 Finomnis