-
Notifications
You must be signed in to change notification settings - Fork 44
Add option to quantize vertical layout metrics. #344
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
Conversation
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.
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.
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.
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?
parley/src/layout/line/greedy.rs
Outdated
y += 1.; | ||
y_overflow -= 1.; | ||
} | ||
y_overflow += overflow; |
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.
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?
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 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.
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.
This separate y_overflow
variable has been removed in the latest change I made.
parley/src/layout/line/greedy.rs
Outdated
// 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.); |
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 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?
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.
- These variables are supposed to contain only negative values (which they do).
- 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.
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.
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.
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.
Where are the extra 7 pixels of trailing vertical space coming from?
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.
From max_coord
being larger. Probably negative leading in action, but I haven't yet looked into this test specifically to confirm.
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.
These extra 7 pixels no longer manifest with the latest changes I made. 🎉
parley/src/layout/line/greedy.rs
Outdated
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(); |
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.
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.
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.
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.
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.
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.
parley/src/layout/line/greedy.rs
Outdated
line.metrics.baseline = y + above; | ||
y = line.metrics.baseline + below; | ||
line.metrics.max_coord = y; | ||
line.metrics.max_coord = y - negative_leading_below; |
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.
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.
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.
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.
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.
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.
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.
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?
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.
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?
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.
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.
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.
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.
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 When quantizing is requested we now only round 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 Thanks to these changes I was able to implement much more robust tests in Overall I'm quite happy with the state of this code now and it's ready for another review. |
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.
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 |
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.
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.
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.
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.
@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"? |
Yes, the text on the buttons is top-aligned rather than center-aligned. I was using |
For There's an argument to be made that 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. |
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
, andmax_coord
in the exported metrics. There's more rounding internally, but only temporarily to calculate the aforementioned. So e.g. exportedascent
will still be the raw fractional. For the line'sy
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.)
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.
Here's a bunch more examples:
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.
Alternative to and closes #343.