Fix Woff2 bounds decompression and size measurement calculations. #380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -552,7 +556,9 @@ private static IReadOnlyList<GlyphLayout> LayoutText(TextBox textBox, TextOption | |||
| advanceX, | |||
| data.ScaledAdvance, | |||
| GlyphLayoutMode.Vertical, | |||
| i == 0)); | |||
| i == 0 && j == 0)); | |||
There was a problem hiding this comment.
Prevents individual metrics within a composite glyph adding to the advance.
| for (int i = removalIndices.Length - 1; i >= 0; i--) | ||
| { | ||
| int match = removalIndices[i]; | ||
| codePointCount += this.glyphs[match].Data.CodePointCount; | ||
| CodePoint currentCodePoint = this.glyphs[match].Data.CodePoint; |
There was a problem hiding this comment.
If we do not update the code point, then we can end up with a codepoint that is then skipped during rendering.
| for (int i = count; i > 0; i--) | ||
| { | ||
| int match = index + i; | ||
| codePointCount += this.glyphs[match].Data.CodePointCount; | ||
| CodePoint currentCodePoint = this.glyphs[match].Data.CodePoint; | ||
| if (!UnicodeUtility.IsDefaultIgnorableCodePoint((uint)codePoint.Value) || UnicodeUtility.ShouldRenderWhiteSpaceOnly(codePoint)) |
There was a problem hiding this comment.
When substituting a ligation, we need to ensure that the remaining codepoint is renderable.
| FontRectangle bounds = GetBounds(glyphLayouts, dpi); | ||
| return new FontRectangle(0, 0, MathF.Ceiling(bounds.Right - topLeft.X), MathF.Ceiling(bounds.Bottom - topLeft.Y)); | ||
| return new FontRectangle(0, 0, MathF.Ceiling(bounds.Width), MathF.Ceiling(bounds.Height)); |
There was a problem hiding this comment.
We know the bounds width/height here so should use it directly.
| @@ -1166,35 +1184,6 @@ private static void SubstituteBidiMirrors(FontMetrics fontMetrics, GlyphSubstitu | |||
| : metric.FontMetrics.VerticalMetrics; | |||
| float ascender = metricsHeader.Ascender * scaleY; | |||
|
|
|||
| // Adjust ascender for glyphs with a negative tsb. e.g. emoji to prevent cutoff. | |||
| if (!CodePoint.IsWhiteSpace(codePoint)) | |||
There was a problem hiding this comment.
This was a hack based upon incorrect assumptions. The proper fix is to use a larger line spacing to meet the font design.
tocsoft
approved these changes
Feb 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites
Description
Fixes #375 #373 #367
We were calculating the bounds bitmap length calculation incorrectly which led to the wrong bounds being read.
Further investigation into determining the correct bounds led to the discovery that the bounds calculation could be simplified and that the size was being incorrectly calculated.
I've added some additional visual tooling in the samples project that helps to verify measurement results. This helped me to discover that we were not rendering the results of some ligature substitutions if the remaining glyph code point was non-rendering. The output is pretty cool.
I also fixed the line breaking algorithm to include a missed breaking opportunity and exactly match browser output.