-
Notifications
You must be signed in to change notification settings - Fork 33
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
Don't round vertical metrics #297
Conversation
This seems reasonable to me. I see that the snapshots end up having quite significant changes as a result of this, which is a little bit concerning. Porting egui to Parley sounds exciting! Is there an upstream issue about this? |
This does affect line spacing, so every snapshot test with more than one line (and even some with one line if a single line's height is rounded differently) will be affected. One thing that is now occurring to me: wherever we round to pixels in screen-space, could that code assume the height is already rounded and do a truncating conversion? I'll try and look into that. I have a draft PR tracking my egui work at emilk/egui#5784. A tracking issue here may be helpful. |
@valadaptive would be willing to share that work in our office hours (#office hours on Zulip). It looks really exciting. They're at 8:00AM San Francisco time on Thursdays. |
Sure! (as my sleep schedule permits) |
The answer seems to be no; we actually take the ceiling of the width/height in the snapshot renderer in order to calculate the image dimensions and layout box dimensions. And there don't seem to be any other places where float-to-integer conversions or even any rounding happens. |
8e90b8b
to
e1030f0
Compare
My concern with this change is that you really want baselines to have an integral y coordinate to avoid blurry rendering. This is essential if you’re applying hinting because the adjustments are meaningless otherwise. And I chose to round the height rather than the computed baseline to guarantee consistent spacing between baselines (particularly in the common single font/size case). Otherwise, the accumulated fractional bits can lead to a 1 pixel jitter between lines and there’s nothing the user can do to correct for this. I think my preference here would be to round (or ceil) only the final line height calculation and leave the others unrounded. If we want to offer fully fractional layouts, then maybe we can make this configurable. |
The fact that browsers accept inconsistent line height seems like a compelling enough argument to move forward. Let’s go ahead and merge this. We’ll need to make sure all of our users (at least Blitz and Masonry) round baselines when rendering. |
This is admittedly a bit of a blind fix. I'll have to do this anyway as part of implementing vertical alignment, which requires more precise measurement of vertical metrics, but this change alone fixes some issues I had when porting egui to use Parley. I'd still like to make the case that this is a desirable change on its own merits.
The rounding code seems to date back 3 years, and doesn't seem to have been examined since. It causes vertical alignment errors when manually setting the line height, which egui does to align different spans (especially with different fonts):

These problems are greatly reduced if the line height isn't rounded:

The line height is also double rounded. First, the ascent, descent, and line height are rounded. Then, leading is calculated from the rounded metrics, added to the ascent and descent, and rounded again to get the min/max coords of the line. These coordinates are what are used to increment
y
, and are thus the actual line height used when rendering. This means the rounding error also accumulates.This calculated line height can be up to 1.5 pixels off per line. For instance, let's say there's an ascent of 12.3, a descent of 1.9, and a line height of 16.5. With the existing rounding, the calculated line height comes out to 18, which is 1.5 pixels higher than the true value.
If the API consumer wants to implement DPI scaling by scaling the rendered output and not the input font size (to avoid e.g. differences in line wrapping due to floating point error), they'll also have to end up rounding the vertical positions of the lines themselves in screen space. This adds yet another layer of rounding.
I think the correct place for rounding is in whichever renderer the API consumer uses--they can round the glyph positions once at whatever scale they want to, and avoid accumulating any rounding error.