Improve glyph scaling correction #4747

miniksa · 2020-02-28T23:02:20Z

Summary of the Pull Request

Improves the correction of the scaling and spacing that is applied to
glyphs if they are too large or too small for the number of columns that
the text buffer is expecting

References

Supersedes Adding primitive support for glyph clusters and n-to-m mapping. #4438
Co-authored-by: Mili (Yi) Zhang milizhang@gmail.com
Related to Some kind of unicode regression #4704 (Avoid splitting surrogate pairs when breaking runs for scaling #4731)

PR Checklist

Detailed Description of the Pull Request / Additional comments

The _CorrectGlyphRun function now walks through and uses the
_glyphClusters map to determine the text span and glyph span for each
cluster so it can be considered as a single unit for scaling.
The total number of columns expected across the entire cluster
text/glyph unit is considered for the available spacing for drawing
The total glyph advances are summed to see how much space they will
take
If more space than necessary to draw, all glyphs in the cluster are
offset into the center and the extra space is padded onto the advance of
the last glyph in the range.
If less space than necessary to draw, the entire cluster is marked for
shrinking as a single unit by providing the initial text index and
length (that is back-mapped out of the glyph run) up to the parent
function so it can use the _SetCurrentRun and _SplitCurrentRun
existing functions (which operate on text) to split the run into pieces
and only scale the one glyph cluster, not things next to it as well.
The scale factor chosen for shrinking is now based on the proportion
of the advances instead of going through some font math wizardry
The parent that calls the run splitting functions now checks to not
attempt to split off text after the cluster if it's already at the end.
This was @DHowett-MSFT's crash.
The split run function has been corrected to fix the glyphStart
position of the back half (it failed to += instead of = which
resulted in duplicated text, sometimes).
Surrogate pair emoji were not allocating an appropriate number of
_textClusterColumns. The constructor has been updated such that the
trailing half of surrogate pairs gets a 0 column width (as the lead is
marked appropriately by the GetColumns() function). This was the
exception thrown.
The _glyphScaleCorrections array stored up over the calls to
_CorrectGlyphRun now uses a struct ScaleCorrection as we're up to 3
values.
The ScaleCorrection values are named to clearly indicate they're in
relation to the original text span, not the glyph spans.
The values that are used to construct ScaleCorrections within
_CorrectGlyphRun have been double checked and corrected to not
accidentally use glyph index/counts when text index/counts are what's
required.

Validation Steps Performed

Tested the utf82.txt file from one of the linked bugs. Looked
specifically at Burmese through Thai to ensure restoration (for the most
part) of the behavior
Ensured that U+1f3e0 emoji (🏠) continues to draw correctly
Checked Fixedsys Excelsior font to ensure it's not shrinking the line
with its ligatures
Checked ligatureness of Cascadia Code font
Checked combining characters U+0300-U+0304 with a capital A

…moji rendering. #4704

…sidered for the column space allocated. Applies excess pen advance space to the final one only.

…t, not glyph). Provide length to corrections as well. Change scaling down algorithm to be based on advances we already know. Correct remaining advance distances to deal with multiple glyph clusters.

…er to understand in regards to indexes/offsets.

miniksa · 2020-02-28T23:19:49Z

This represents me needing to check in the big Excel spreadsheet that I used to diagnose this.

miniksa · 2020-02-28T23:24:09Z

OK. I might just yeet in a change from UNICODE_SPACE to L'M' on the font width thing to make horrific things like Curlz MT work:

carlos-zamora

Is there any way we can add tests to cover the examples you've provided? (and the table you created)

src/renderer/dx/CustomTextLayout.cpp

DHowett-MSFT · 2020-02-28T23:33:33Z

src/renderer/dx/CustomTextLayout.cpp

+        for (auto i = 1; i < text.size(); ++i)
+        {
+            _textClusterColumns.push_back(0);
+        }


Suggested change

for (auto i = 1; i < text.size(); ++i)

{

_textClusterColumns.push_back(0);

}

_textClusterColumns.resize(text.size(), 0); // expand vector to text length, but filled with 0s

alternatively, std::fill_n(std::back_inserter(_textClusterColumns), count, 0);

I used fill over fill_n just because I had an inkling that the optimizer might be sad about that one too.

nnnn fill isn't quite right because of the insertion nature of this. trying fill_n and hoping for the best.

fill_n with back_inserter works, but now Dustin has another idea of using .resize which sounds even better to me.

DHowett-MSFT · 2020-02-29T01:26:12Z

Crash:

"`u{0e04}`u{0e49}`u{0e33}"

DHowett-MSFT · 2020-02-29T01:27:21Z

0e04 is a base, 0e49 and 0e33 are combiners. It only reproduces with both combiners.

DHowett-MSFT · 2020-02-29T02:05:57Z

... it only crashes in opt builds lol

DHowett-MSFT · 2020-02-29T02:25:48Z

diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp
index b40bd9140..c977789f5 100644
--- a/src/renderer/dx/CustomTextLayout.cpp
+++ b/src/renderer/dx/CustomTextLayout.cpp
@@ -651,9 +651,9 @@ try
             // Move the X offset (pixels to the right from the left edge) by half the excess space
             // so half of it will be left of the glyph and the other half on the right.
             // Here we need to move every glyph in the cluster.
-            std::for_each_n(_glyphOffsets.begin() + clusterGlyphBegin,
-                            clusterGlyphLength,
-                            [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; });
+            std::for_each(_glyphOffsets.begin() + clusterGlyphBegin,
+                          _glyphOffsets.begin() + clusterGlyphBegin + clusterGlyphLength,
+                          [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; });
 
             // Set the advance of the final glyph in the set to all excess space not consumed by the first few so
             // we get the perfect width we want.

it's a bug in VS 16.4's optimizer -- glyphClusterLength gets stored in register $rdi and subsequently gets the heck clobbered out of it when for_each_n begins.

zadjii-msft

This looks like it makes to me. I guess I'll hold my sign-off while we wait on the opt crash to get fixed.

…monospaced fonts work by consequence. Monospace fonts should be fine because M and 0x20 should be the same width...

… with it stomping one of the registers allocated for the N count.

miniksa · 2020-03-02T17:20:18Z

diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp
index b40bd9140..c977789f5 100644
--- a/src/renderer/dx/CustomTextLayout.cpp
+++ b/src/renderer/dx/CustomTextLayout.cpp
@@ -651,9 +651,9 @@ try
             // Move the X offset (pixels to the right from the left edge) by half the excess space
             // so half of it will be left of the glyph and the other half on the right.
             // Here we need to move every glyph in the cluster.
-            std::for_each_n(_glyphOffsets.begin() + clusterGlyphBegin,
-                            clusterGlyphLength,
-                            [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; });
+            std::for_each(_glyphOffsets.begin() + clusterGlyphBegin,
+                          _glyphOffsets.begin() + clusterGlyphBegin + clusterGlyphLength,
+                          [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; });
 
             // Set the advance of the final glyph in the set to all excess space not consumed by the first few so
             // we get the perfect width we want.

it's a bug in VS 16.4's optimizer -- glyphClusterLength gets stored in register $rdi and subsequently gets the heck clobbered out of it when for_each_n begins.

Dustin found out that for_each doesn't suffer from this on Friday and texted me about it but didn't update the thread. So this is me updating the thread for transparency and record that I'm changing it from for_each_n to for_each as that doesn't suffer from the optimizer bug.

…rted, so go back to fill_n with the back_inserter as Dustin originally suggested.

…'t want an underflow if columns is weirdly 0.

…a fill operator.

ghost · 2020-03-02T19:21:04Z

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

@leonMSFT

… more text than the column value we were also given. (#4781) ## Summary of the Pull Request Adjusts column padding code in `CustomTextLayout` to only pad out for surrogate pairs, not anything that reports two columns. ## References - See also #4747 ## PR Checklist * [x] Closes #4780 * [x] I work here. * [x] Manual tests. * [x] No doc, this fixes code to match comment. Oops. * [x] Am core contributor. Also discussed with @leonMSFT. ## Detailed Description of the Pull Request / Additional comments For surrogate pairs like high Unicode emoji, we receive two wchar_ts but only one column count (which is usually 2 because emoji are usually inscribed in the full-width squares.) To compensate for this, I added in a little padding function at the top of the `CustomTextLayout` construction that adds a column of 0 aligned with the second half of a surrogate pair so the text-to-glyph mapping lines up correctly. Unfortunately, I made a mistake while either responding to PR feedback in #4747 or in the first place and I made it pad out extra 0 columns based on the FIRST column count, not based on whether or not there is a trailing surrogate pair. The correct thing to do is to pad it out based on the LENGTH of text associated with the given column count. This means that full width characters which can be represented in one wchar_t, like those coming from the IME in most cases (U+5C41 for example) will have a column count of 2. This is perfectly correct for mapping text-to-glyphs and doesn't need a 0 added after it. A house emoji (U+1F3E0) comes in as two wchar_ts (0xD83C 0xDFE0) with the column count of 2. To ensure that the arrays are aligned, the 2 matches up with the 0xD83C but the 0xDFE0 needs a 0 on it so it will be skipped over. (Don't worry, because it's a surrogate, it's naturally consumed correctly by the glyph mapper.) The effect was that every OTHER character inserted by the IME was scaled to 0 size (as an advance of 0 was expected for 0 columns). The fix restores it so those characters don't have an associated count and aren't scaled. ## Validation Steps Performed - Opened it up - Put in the house emoji like #4747 (U+1f3e0) - Put in some characters with simplified Chinese IME (fixed now) - Put in the utf83.txt sample text used in #4747

GTMxCode · 2020-04-26T09:58:57Z

Is there any way I can revert this? I want my double wide glyphs to display and not be shrunken down into what amounts to as dots.. an we get a setting for this?

zadjii-msft · 2020-04-27T14:07:29Z

@GTMxCode I'm pretty sure we're working on some of the fallout from this in #900 - could you update that thread with what font & glyphs you're seeing render at the wrong size? We'd rather just fix the issue you're having with those characters than add a setting to disable a bug fix 😄

## Summary of the Pull Request This fixes the RTL regression caused in #4747. We create the rectangle taking the direction (through the BiDi Level) into account, and then the rendering works again. The GlyphRun shaping could still probably use some work to be a polished thingy, and there are still issues with RTL getting chopped up a lot when there's font fallback going on, but this fixes the regression, and it's now functional again.  ## References #4779 #4747  ## PR Checklist * [x] Closes #4779 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments The baseline is actually direction dependent. So when it was being initialized, the unconditional baseline as left broke it, setting the box off to right of the text. We just check if the `GlyphRun->bidiLevel` is set, and if so, we adjust it so that the baseline lines up with the right, not with the left.  ## Validation Steps Performed ![image](https://user-images.githubusercontent.com/16987694/80968891-681cbc00-8e21-11ea-9e5c-9b7cf6d78d53.png)

miniksa added 11 commits February 26, 2020 11:19

Don't split surrogate pairs when breaking runs for scaling. Affects e…

f892200

…moji rendering. #4704

merge #4438 by @milizhang

98b483c

Allows sum of all advances for glyphs used in these columns to be con…

b490847

…sidered for the column space allocated. Applies excess pen advance space to the final one only.

Use struct for scale corrections. Move them to use correct index (tex…

98dcba9

…t, not glyph). Provide length to corrections as well. Change scaling down algorithm to be based on advances we already know. Correct remaining advance distances to deal with multiple glyph clusters.

Put 0s for the columns on a piece of text bigger than one char.

ec1d4fc

Add a bunch of comments. Delete a bunch of old code.

c7d5a95

Don't need surrogate fix anymore with col fix.

799a144

Rewrite algorithm and add tons of comments to attempt to make it easi…

cf8d0d4

…er to understand in regards to indexes/offsets.

fix analysis warnings.

f5c90cb

typo

e557af4

Merge branch 'master' into dev/miniksa/layout_2

ddadcc5

miniksa self-assigned this Feb 28, 2020

miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Feb 28, 2020

miniksa requested review from carlos-zamora, DHowett-MSFT, leonMSFT and zadjii-msft February 28, 2020 23:04

carlos-zamora reviewed Feb 28, 2020

View reviewed changes

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved

src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved

DHowett-MSFT approved these changes Feb 28, 2020

View reviewed changes

djdv mentioned this pull request Mar 1, 2020

Some kind of unicode regression #4704

Closed

zadjii-msft reviewed Mar 2, 2020

View reviewed changes

If we use M for the width instead of UNICODE_SPACE, then a lot of non…

3c1cd50

…monospaced fonts work by consequence. Monospace fonts should be fine because M and 0x20 should be the same width...

miniksa added 2 commits March 2, 2020 09:16

Use fill instead of loop per PR feedback.

7b3f6e1

Changing for_each_n to for_each avoids VS 16.4 compiler/optimizer bug…

087e920

… with it stomping one of the registers allocated for the N count.

miniksa added 4 commits March 2, 2020 09:22

Carlos-based spell checking.

99533e4

premature change to fill was a logical problem. this needs to be inse…

cdb58e5

…rted, so go back to fill_n with the back_inserter as Dustin originally suggested.

Just... for safety sake. Let's clamp the subtraction here to 0. I don…

a1b8b77

…'t want an underflow if columns is weirdly 0.

Dustin had an even better idea via Teams to use .resize since it has …

f0c87bd

…a fill operator.

zadjii-msft approved these changes Mar 2, 2020

View reviewed changes

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 2, 2020

ghost merged commit 7f43b40 into master Mar 2, 2020

ghost deleted the dev/miniksa/layout_2 branch March 2, 2020 19:21

This was referenced Mar 2, 2020

Adding primitive support for glyph clusters and n-to-m mapping. #4438

Closed

Stop padding text columns for fullwidth characters #4781

Merged

j4james mentioned this pull request Mar 20, 2020

Hebrew Characters do not render at all #5045

Closed

miniksa mentioned this pull request Apr 22, 2020

Font rendering issues in Terminal #610

Closed

schorrm mentioned this pull request May 4, 2020

Fix RTL regression (fixes #4779) #5734

Merged

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve glyph scaling correction #4747

Improve glyph scaling correction #4747

miniksa commented Feb 28, 2020 •

edited by DHowett-MSFT

Loading

miniksa commented Feb 28, 2020

miniksa commented Feb 28, 2020

carlos-zamora left a comment

DHowett-MSFT Feb 28, 2020

DHowett-MSFT Feb 28, 2020

miniksa Mar 2, 2020

miniksa Mar 2, 2020 •

edited

Loading

miniksa Mar 2, 2020

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

zadjii-msft left a comment

miniksa commented Mar 2, 2020

ghost commented Mar 2, 2020

GTMxCode commented Apr 26, 2020 •

edited

Loading

zadjii-msft commented Apr 27, 2020

Improve glyph scaling correction #4747

Improve glyph scaling correction #4747

Conversation

miniksa commented Feb 28, 2020 • edited by DHowett-MSFT Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa commented Feb 28, 2020

miniksa commented Feb 28, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett-MSFT Feb 28, 2020

Choose a reason for hiding this comment

DHowett-MSFT Feb 28, 2020

Choose a reason for hiding this comment

miniksa Mar 2, 2020

Choose a reason for hiding this comment

miniksa Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

miniksa Mar 2, 2020

Choose a reason for hiding this comment

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

DHowett-MSFT commented Feb 29, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa commented Mar 2, 2020

ghost commented Mar 2, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

GTMxCode commented Apr 26, 2020 • edited Loading

zadjii-msft commented Apr 27, 2020

miniksa commented Feb 28, 2020 •

edited by DHowett-MSFT

Loading

miniksa Mar 2, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

GTMxCode commented Apr 26, 2020 •

edited

Loading