-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix rounding errors with font scaling #2490
base: master
Are you sure you want to change the base?
Conversation
edit: This has been fixed. This patch introduces some issues with font rendering, even with Kerning is badly impacted, much like it is in #2164. Perhaps it is worth attempting to address the problem by performing the scaling only once, as late as possible? |
Ok, the latest commit does a much better job at fixing the rounding errors when Kerning is absolutely different with this PR on high DPI displays with Before: After: And here are some detail shots that show the kerning differences. The changes may be better, but I don't necessarily think it is any worse than before. Open these each in a new tab and switch between them. Or put each into a separate layer in photoshop and toggle layer visibility. Detail 1Before: After: Detail 2Before: After: |
b9ef406
to
507d516
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.
The current code is designed to produce crisp text and ok kerning for any pixels_per_point
scale. The new code fails pretty badly on non-integral pixels_per_point
(which are common).
There may be some kerning improvements to be made, but if it comes at the cost of crisp text it should be made optional imho.
It is not the keening that I am trying to address. That was just a side effect. The situation is that the code which rounds to pixels is causing the fonts to appear at the wrong scale when pixels_per_point is not 1.0. See the new tests in the PR. |
I appreciate that, but the point is that your PR breaks something else: crisp text. Getting both crisp text and kerning right is difficult, and I suspect the current code may be doing the best job that could be done, but I'd be happy to be proven wrong. |
I guess I'll have to look more closely, but on initial inspection I do not see any additional blurriness in the text when rendered with non-integer scale factors. What I do see, and what this PR wants to achieve, is correctly scaling the text proportionally to the scale factor. The following screens show the difference. (The window size was not changed, only the scale value.) BeforeWith With With Notice that the words "feature" and "syntect" are laid out on different lines depending on the value of AfterWith With With Now notice that the text layout looks proportionately identical across the board. Also notice that that semicolon on the I still can't make out the blurriness you mentioned, even at 0.9. Here is a comparison from both the before and after magnified by 10x: BeforeAfterNeither looks blurrier to me. The only difference is in kerning (as mentioned in the OP) and even then, I don't think either is objectively worse. For instance, in the "before" there is 1px of space between the "u" and "r" that doesn't exist in the "after". But also in the "after" there is 1px of space between "{" and "s" that doesn't exist in the "before". So, it appears to be a net neutral change. No better, no worse, just different. Ok, so that's the proportional font, what about the monospace font? BeforeAfterNow it is obvious that the font sizes are literally different. In the "after" image, the "H" and the "d" are more clearly defined as a result of it just being larger. The double quotes and "w" are less clearly defined, also as a result of the larger font. And kerning may be worse. (Particularly the extra space between the two "l"s and the extra space between the "o" and "r".) On the font size with the monospace font, the text is more accurately proportional to the width of the text input widget (i.e., the raison d'être for this PR): Before
After
The proportionality of the monospace font in this test varies by about 8% before this PR, but only about 2% after. It can never be perfect because it rounds each character to the pixel grid (for #382), but I'll take 2% over 8% any day! I'm happy to address any feedback that can demonstrate blurring, but I honestly don't see it, here. For the kerning issues in the monospace font ... Yeah, that's not ideal. But it accomplishes the goal of retaining proportionality (and character legibility), which is far more important in a monospace font than perfect kerning on a pixel grid. (See OP for rationale.)
Ain't it the truth! I think I've shown that the crispness has not been affected at all by this PR. Kerning is still problematic, but also, I believe the real solution to that is probably subpixel rendering or another antialiasing technology (hinting, et al). But that is very much outside the scope of this PR. |
Yes, that is intentional, unfortunately. It is the only way to both maintain the correct text bounding box regardless of scale, and also align each glyph to the pixel grid independently. In one opinion it is unacceptable to have uneven kerning, and in another opinion, it is unacceptable to change the size of text bounding boxes when moving the window between different displays. Since we are at an impasse, I propose a compromise with a new opt-in feature flag in The real fix is of course better antialiasing (subpixel rendering, hinting, super sampling, etc. that does not rely on rounding positions to the pixel grid) for which I will create a separate ticket for tracking. Meanwhile this PR can potentially go forward to fix rounding errors introduced by forcing glyph kerning to the pixel grid. Does this sound reasonable to you? I have tried the proposed feature flag locally and it is doing what I need it to. |
- Store the font size in _points_ instead of _pixels_. - Scale only for drawing. - Add a new feature `proportional_font_scaling`. - Maintains the correct kerning and fixes relative font sizes for any `pixels_per_point` with monospace fonts. - Enabling this feature will cause some unavoidable kerning issues with proportional fonts. - Add tests for font scaling with `pixels_per_point`. - Add note to `epaint` CHANGELOG.
2f3055a
to
75fa886
Compare
Yes, having flags for all these options would be great! I thin we can give full control if we have flags for:
It would be extra great if these were runtime flags. For instance, for high-resolution displays it could make sense to turn off both types of rounding to get fluid scaling and perfect kerning at the cost of slightly blurry text (which makes much less difference on high resolution displays) |
In terms of design, should the new flag state be attached to the |
That sounds good to me! Perhaps add a |
I am working on something like a terminal emulator. It needs to have a fixed width font that renders identically on all platforms. One of the problems I am facing right now is that with the
pixels_per_point
value at 2.0 on macOS and 1.0 on Windows, scaling the fonts ends up rendering slightly differently in these environments.Here are some screenshots with a grid of characters rendered on Windows and macOS. I initially developed this on macOS, so it is showing what I "expected" to see on Windows:
macOS:
Windows:
They both have exactly 800x600
LogicalSize
inner window size, e.g. it's 800x600 physical pixels on Windows, and 1600x1200 physical pixels on macOS. But clearly the font size is ever so slightly different enough that the cursor isn't clipped in the corner of the window on Windows, as it is on macOS.This PR attempts to address the bug.
Reveal outdated patch description
First, we notice that
Font::round_to_pixel()
is called with a point size that is already scaled bypixels_per_point
, and it internally resizes by dividing after rounding. This leads to rounding errors. For example, suppose the point size is 16.27 andpixels_per_point
is 2.0:Compare this when
pixels_per_point
is 1.0:If instead we
floor
the value, then we get the same result, regardless of the value ofpixels_per_point
:Secondly, the same issue occurs with the horizontal advance, so we change that
round
tofloor
also.Description of the new patch:
We defer the scaling of the font to as late as possible (e.g. when drawing glyphs). The fonts themselves now only store sizes in points (unscaled). Rounding of positions is still done, but it uses the font points coordinate space instead of physical pixels. This accounts for the kerning differences on high DPI displays as shown in screenshots below.
The result is that the rasterization looks identical on Windows where
pixels_per_point == 1.0
, but the rasterization on macOS (pixels_per_point == 2.0
) now more closely resembles what is seen on Windows:I'll leave this as a draft PR to open discussion. I could be wrong, but I believe this is the correct way to address the problem. I'll leave a list of items that I know need to be done before this can be accepted.
TODO:
Update comments and function names to reference flooring instead of rounding