Skip to content

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Apr 29, 2025

Background

Vertical metrics are no longer rounded since #297 and so the rounding needs to happen in the renderer. The upside is that there's potential for more control, the downside is that it's another footgun. I initially created documentation and updated examples in #343 but after going through that I came to the conclusion that asking users to do that will be a major inconvenience, and likely will lead to incorrect results.

Solution

This PR here takes a different approach. We bring back quantization but make it optional. That way advanced users who know what they're doing can still get access to the raw fractions. However, for most users we can recommend just using our quantization which will automatically take care of the blurry text issue. The examples will also use the much simpler quantization option.

Implementation details

Now as pointed out in #297 our old quantization strategy was flawed where rounding errors accumulated and weren't dealt with. I didn't want to just bring that back so instead I played around with Chrome a bunch to see how it does line height handling. This PR now contains a solution that mimics Chrome.

We now only round baseline, min_coord, and max_coord in the exported metrics. There's more rounding internally, but only temporarily to calculate the aforementioned. So e.g. exported ascent will still be the raw fractional. For the line's y position, when the accumulated overflow reaches over half a pixel we will either overflow to the previous line or start with a gap, depending on the direction of the accumulation.

Examples

As seen in the example below (16.0px font size, 1.3em line height (20.8px)), we now match Chrome's behavior of overflowing the 1st, 4th, 9th, and 14th line over the previous line. Best spotted by looking at the edges of the red/green selection boxes. Also note that despite the perceived line height fluctuating between lines due positional changes, the rendered line height does not change. Easy to see via the selection boxes, which are always the same height. (They're colored spans in Chrome, but colored spans are the same height as selection boxes until line height is greater than ascent + descent, which is not the case in this example - making it a valid comparison.)

16 0px-1 3em-2fps

Here's another example (16.0px font size, 1.33125em line height (21.3px)) showing a scenario where gaps are formed due to the line height rounding going in the other direction. Also the line height is greater than ascent + descent, so the gaps are actually visible with selection boxes.

Chrome Swash
16 0px-21 3px-chrome 16 0px-21 3px-swash

Here's a bunch more examples:

Notes Chrome Swash
Mixing of fonts and sizes, 1.99em line height mixed-fonts-chrome mixed-fonts-swash
16px 0.83em (13.28px) 16 0px-0 83em-chrome 16 0px-0 83em-swash
19px 0.83em (13.28px) 19 0px-0 83em-chrome 19 0px-0 83em-swash

Mismatches

However, it's not always a perfect match. Fractional font sizes are rendered at a different size, which probably causes the layout changes seen in the 18.5px example. Also, when going to very small line heights, starting from around 0.65em Chrome has some additional logic to keep perceived line height higher. I'm not exactly sure what it is, but it accumulates by the time you get to tiny line heights like 0.25em. It's not related to the rounding of the line height, because in the 0.25em case the line height is exactly 4px. Anyway, not super important right now, but might be worth figuring out later.

Notes Chrome Swash
18.5px 0.83em (13.28px) 18 5px-0 83em-chrome 18 5px-0 83em-swash
16px 0.65em (10.4px) 16 0px-0 65em-chrome 16 0px-0 65em-swash
16px 0.25em (4px) 16 0px-0 25em-chrome 16 0px-0 25em-swash

Alternative to and closes #343.

@nicoburns nicoburns requested a review from valadaptive April 29, 2025 13:21
Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions (and other people should probably review), but I much prefer this approach to the one in #343. As a consumer of Parley, it gives me a much smoother upgrade path and saves me from needing to manage rounding myself.

@xStrom
Copy link
Member Author

xStrom commented Apr 29, 2025

For the record, this change also has a positive effect on the Vello Editor example, at least when using Vello 4.0 as it is in this repo. I haven't tested with Vello main.

vello-small

Copy link
Contributor

@valadaptive valadaptive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair warning: I haven't touched this code in a bit and I'm kinda tired, so my concerns may not be relevant.

The approach of accumulating the error separately seems overcomplicated to me. When rounding lines to pixels in egui, I just rounded the y-position of the line as it was passed. I am not entirely sure why the same thing can't be done here.

I would also prefer to round in as few places as possible. In this PR, there are places where we round both positional units (the y position) and length units (the leading, ascent, and descent). Rounding length units is not ideal because you can once again accumulate error when you add them up.

Another part, the clamping of the lines' min and max coords, seems unrelated to rounding and could affect layout in further ways. Would it be possible to split that into a separate PR instead?

y += 1.;
y_overflow -= 1.;
}
y_overflow += overflow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but this seems a bit overengineered? When rounding lines to pixel positions in egui, I just accumulated the Y position as a float, and only rounded the Y positions of individual lines. Is the error accumulated separately to make it easier to distribute leading or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results didn't match with Chrome, but I spent some more time now looking into it. Looks like Parley using f32 is a factor, when I do the math with f64 I got past some of the issues I was seeing earlier.

I'll spend some more time validating various test cases with f64 and if I can I'll simplify the code. Then later can just add a comment that using f32 causes differences from Chrome.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This separate y_overflow variable has been removed in the latest change I made.

// We clamp leading to zero for the purposes of min/max coords,
// which in turn clamps the selection box minimum height to ascent + descent.
let negative_leading_above = leading_above.min(0.);
let negative_leading_below = leading_below.min(0.);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to min clamps the leading between a negative value and 0. That matches the naming of the variables (negative_leading_above and negative_leading_below), but does not match the comment, which says that negative leading is incorrect for calculating min/max coords. Which one is correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. These variables are supposed to contain only negative values (which they do).
  2. Negative leadings are incorrect for min/max coords, but correct for baseline. Tiny line height means that the baseline is very high (shifted up by the negative leading), but the glyphs will still be drawn in their regular size, which means that min/max coords should not be affected by negative leading.

Now the code does use e.g. negative_leading_above to calculate the minimum coord. So I guess the wording can be improved, that it's not that it's incorrect to use negative leading in the calculation at all - it is that we need to counteract against it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the negative_leading_above and negative_leading_below variables in the latest change. The same functionality still exists, but is now implemented in a clearer way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are the extra 7 pixels of trailing vertical space coming from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From max_coord being larger. Probably negative leading in action, but I haven't yet looked into this test specifically to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These extra 7 pixels no longer manifest with the latest changes I made. 🎉

y_overflow += overflow;
// We also mimic Chrome in rounding ascent/descent.
line.metrics.ascent = line.metrics.ascent.round();
line.metrics.descent = line.metrics.descent.round();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are ascent and descent used for in rendering, and how do we know that Chrome rounds them?

This runs into the issue that a rounded sum of two numbers != a sum of two rounded numbers. e.g. if a line has an ascent of 9.4 and a descent of 3.4, its height will be 12.8, or 13 when rounded. But if you round the ascent and descent separately, you'll get 9 + 3 = 12--you "lose" one unit to rounding.

I'm not sure if that matters in this case, but it is something to be aware of.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested various font sizes (where I know the fractional ascent/descent) and looked at how Chrome quantized the height. Now it is possible that the rounding was done slightly differently, but the results matched with this.

I'll see about verifying a bunch of different ascent/descent numbers when I do the simplifying of the overflow rounding. To see if I can find a case that might give more insight.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, to answer your question on what are ascent and descent used for in rendering, they are used in calculating the height of the selection box.

I confirmed once again that this is how Chrome behaves. For example with Roboto 19.0px the ascent and descent are 17.626953 and 4.638672. When rounding before summing we get 23, when rounding after summing we get 22. Chrome renders the selection box with a height of 23px. Roboto 20.0px would be another example with 18.554688 and 4.8828125.

With the latest changes I also added a dedicated test for this called lines_integral_line_height_ascent_descent_rounding.

However, in the latest version I only round these for the purpose of calculating other integral metrics. The fractional originals remain intact in the exported data.

line.metrics.baseline = y + above;
y = line.metrics.baseline + below;
line.metrics.max_coord = y;
line.metrics.max_coord = y - negative_leading_below;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we had the (implicit) invariant that summing the max_coord - min_coord of every line would, modulo floating-point error, give you the total height of the text. That appears to not be the case here.

The clamping behavior seems to affect what counts as the "logical bounds" of the text, which could affect layout in the various frameworks that use Parley. For instance, decreasing the line height past a certain point will have no effect on the logical height of the text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change you have a higher chance of getting the total height of the text. I added this precisely because how incorrectly tiny max_coord - min_coord got with small line heights.

Which is to say, what you state is correct - decreasing the line height past a certain point will have no effect on the logical height of the text - if we don't counteract the negative leading, then decreasing the line height will incorrectly have an effect on the coords.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way, after this change, of getting the "stylistic line height" of a given line? Right now, you can just do max_coord - min_coord, but under this PR, that might give you a number larger than the actual line height.

I need this for egui--I'll need to split up text into multiple layouts, and position each layout after one another vertically by their line heights.

Copy link
Member Author

@xStrom xStrom Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's:

  • line height (which is accessible via line_height) - can be quite small (e.g. 0.8em)
  • selection box height (max_coord - min_coord)

In order to position multiple layouts you would use line_height, that is you sum it for all the lines. You can't sum the already rounded line height though, so we should maybe not round the publicly available line height and/or provide an accurate precomputed total height sum of the layout via Layout::height().

Now there is the interesting question on whether to support rounding error carry-over in the multiple layout case. Which is to say, you give the error remainder from the previous layout to the next layout as a starting point.

I'll look into this a bit to see how easily we could support such precision in the quantized mode. (In the raw fractional mode you would do all of that bookkeeping yourself of course.)

I guess comparing to CSS this is a question of using <span /> vs <div />. I'll double check, but I'm pretty sure the error carry-over happens with spans but not across div (block) boundaries. So if we say that a single layout is a block element, then positioning multiple layouts won't handle error carry-over and there will be extra fractional gaps between the layouts.

How important is the error carry over between layouts for your use case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but IIRC line_height is a style property. Should I just manually accumulate the maximum line height from every styled span within the given line? How will this work once vertical alignment is implemented?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just manually accumulate the maximum line height from every styled span within the given line?

We already do this so you can just access the line's line_height which is the maximum of any item on that line.

How will this work once vertical alignment is implemented?

Hard to say for sure because the implementation doesn't exist, but I like to think that we can keep the line's line_height calculation working even with vertical alignment. That might even be a prerequisite to properly implement it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest changes I made, the answer to this question is to use the Layout::height method. It returns the sum of all the lines' line_height. If quantize is false then this will give you precisely what you need in vertically stacking multiple Layout.

However, if you're depending on the built-in quantization with quantize set to true, then this approach won't work due to the expectation of y == 0 being a pixel boundary. This PR has grown rather large already, so while I think it's worth adding as a feature, it's an easy line to draw for splitting the work into a future PR.

@xStrom
Copy link
Member Author

xStrom commented May 2, 2025

Alright, I ended up rewriting the line metrics code. The previous version was a bit messy and confusing, while the new one should be much clearer. Generally the rounding results are the same, but achieved in a simpler way.

Gone is the separate y_overflow tracking variable and instead we just use y.round(), though I had to upgrade y to f64 to achieve parity with the separate tracking (and Chrome). Similarly the layout's total height is now also calculated with f64, though ultimately still stored as f32 to keep the API unchanged.

When quantizing is requested we now only round baseline, min_coord, and max_coord in the exported metrics. There's more rounding internally, but only temporarily to calculate the aforementioned. So e.g. exported ascent will still be the raw fractional.

The much bigger change is that I improved the testing framework and then added a bunch more tests based on Chrome's results. This allowed me to more fearlessly refactor the metrics code and it will also better protect against unintended changes in the future.

For the testing framework I added the option to specify the output size (still defaults to layout's size) and exposed the internal render_and_check_snapshot method to be able to supply selection geometry. I also changed some default colors so that subtle changes at the intersection of shapes would be more easily noticed, most notably the selection box colors now fluctuate between lines, making a bit of a red/green zebra pattern.

Thanks to these changes I was able to implement much more robust tests in test_lines.rs. These tests do a bunch of classic assertions to make sure metrics behave like expected and then finally perform a visual snapshot check on top. I also added a lines.html file which I used to get metrics from Chrome. It can be used to either verify the tests in test_lines.rs or to add new similar tests.

Overall I'm quite happy with the state of this code now and it's ready for another review.

@xStrom xStrom requested a review from valadaptive May 2, 2025 21:25
Copy link
Contributor

@valadaptive valadaptive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the rounding still seems kinda weird to me, but if the general implementation (not just the output) matches Chrome, I'm not too concerned about things like double-rounding messing things up.

@@ -454,7 +456,7 @@ impl<'a, B: Brush> BreakLines<'a, B> {
}
}
}
let mut y = 0.;
let mut y: f64 = 0.; // f32 causes test failures due to accumulated error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the tests here just for comparing to Chrome's output?

Not sure if we need to match Chrome exactly here, especially if Chrome uses f64 consistently for all their layout code but we're only using it for areas where we're currently testing against Chrome.

It may be better to just stick to one floating-point type and use it consistently; I'm fine punting on that for later, maybe opening an issue about it, and leaving this as-is for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not dead set on always matching Chrome, but I think it's a good starting point. Then if we decide to deliberately diverge - that's fine if it's reasoned.

As for f64, Chrome uses LayoutUnit and friends, which aren't even simple numbers:

// `FixedPoint` is a fixed-point math class template, with the number of bits
// for the fractional part as a template parameter.
//
// `LayoutUnit` is an instantiated class of the `FixedPoint`, storing multiples
// of 1/64 of a pixel in an `int32_t` storage.
// See: https://trac.webkit.org/wiki/LayoutUnit
//
// `TextRunLayoutUnit` stores multiples of 1/65536 of a pixel in the same
// storage, so it has less integral part than `LayoutUnit` (16/16 bits vs 26/6
// bits). Suitable for a run of text, but not for the whole layout space.
//
// `InlineLayoutUnit` stores the same precision as `TextRunLayoutUnit` using an
// `int64_t` storage. It can provide the text precision, and represent the whole
// layout space (and more, 48 bits vs 26 bits), but it's double-sized.

The reason why I think using f64 in a few places is fine, matches the Chrome comment for TextRunLayoutUnit: Suitable for a run of text, but not for the whole layout space. Which is to say, using f32 for smaller numbers and a few calculations doesn't suffer from error accumulation as much as repeated math on the height of the whole layout, which is what needed to be changed to f64 in Parley to match Chrome's behavior.

In the future, it might even be worth looking into if we want to mimic the whole 1/64 & 1/65536 system.

@xStrom xStrom added this pull request to the merge queue May 7, 2025
Merged via the queue into linebender:main with commit bd6a384 May 7, 2025
21 checks passed
@xStrom xStrom deleted the round-opt branch May 7, 2025 19:48
@valadaptive
Copy link
Contributor

This somehow broke egui label alignment...
image

@nicoburns
Copy link
Contributor

This somehow broke egui label alignment... image

@valadaptive Do you have a screenshot of what this is supposed to look like? Is the issue that the text on the "buttons" is not vertically aligned with each other? Are the button using "inline boxes"?

@valadaptive
Copy link
Contributor

valadaptive commented May 20, 2025

Yes, the text on the buttons is top-aligned rather than center-aligned. I was using min_coord and max_coord for the text bounding box, but now that leading is clamped, I need to calculate the min/max coords myself instead to get the text centered properly. Not sure why clamping the leading causes the labels to become off-center (since it should apply equally to the top and bottom).

@xStrom
Copy link
Member Author

xStrom commented May 20, 2025

For vertical-align: top you want to use the line's y coordinate, while for vertical-align: text-top you want to use baseline - ascent. Neither of these are currently being provided in a pre-calculated form, so that kind of sucks.

There's an argument to be made that min_coord should be changed to just baseline - ascent (without leading) and that we introduce a different property for the selection box that includes the clamped leading.

Probably makes sense to solve this within the context of #291 - so that we calculate the properties to be able to do vertical alignment inside Parley, but also provide the coordinates for those who want to achieve the same type of alignment outside of Parley.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants