unbroken runs containing ligatures occasionally cause font size flickering #696
Assignees
Labels
Area-Fonts
Related to the font
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.
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
oising
added
Area-Rendering
|
@miniksa I'm going to use this as the master external issue tracking what used to be MSFT:21252989 "unbroken runs containing ligatures occasionally cause font size flickering". It looks like the same problem. |
DHowett-MSFT
changed the title
|
Just to reinforce the issue, it doesn't happen only with this font. LigConsalata (an Inconsolata variant with ligatures) seems to be broken as well on any line with ligatures. How it looks when it is broken (in this case the "->" ligature breaks the line) How it looks when there is no ligature around: What I found interesting is that when the line breaks and the rest of it doesn't have any ligature it displays fine that part of the line (as can be seems on the first print). I suppose it re-render the whole terminal line whenever there's a ligature and that's where it is breaking on those fonts. I tested both on windows and WSL2, and the same happens on both. Fira Code doesn't seem to break any line, though. In case it helps, that's where I got the LigConsolata font: |
|
That's spectacular. My guess at it is that something is detecting the run is suddenly "complex" and then mucking up what sub-segments of the run have what properties. I'll get to this. |
29 tasks
5 tasks
ghost
pushed a commit
that referenced
this issue
Jan 21, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request This change tries to fix column size calculation when shaping return glyphs that represents multiple characters (e.g. ligature). <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References This should fix #696. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [ ] Closes #xxx * [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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments Currently, it seems like CustomTextLayout::_CorrectGlyphRun generally assumes that glyphs and characters have a 1:1 mapping relationship - which holds true for most trivial scenarios with basic western scripts, and also many, but unfortunately not all, monospace "programming" fonts with programming ligatures. This change makes terminal correctly processes glyphs that represents multiple characters, by properly accumulating the column counts of all these characters together (which I believe is more close to what this code originally intended to do). There are still many issues existing in both CustomTextLayout as well as the TextBuffer, and the correct solution to them will likely demand large-scale changes, at least at the scale of #3578. I wish small changes like this can serve as a stop gap solution while we take our time to work on the long-term right thing. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Builds and runs. Manual testing confirmed that it solves #696 with both LigConsalata and Fixedsys Excelsior.
ghost
added
Resolution-Fix-Committed
DHowett-MSFT
pushed a commit
that referenced
this issue
Jan 24, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request This change tries to fix column size calculation when shaping return glyphs that represents multiple characters (e.g. ligature). <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References This should fix #696. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [ ] Closes #xxx * [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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments Currently, it seems like CustomTextLayout::_CorrectGlyphRun generally assumes that glyphs and characters have a 1:1 mapping relationship - which holds true for most trivial scenarios with basic western scripts, and also many, but unfortunately not all, monospace "programming" fonts with programming ligatures. This change makes terminal correctly processes glyphs that represents multiple characters, by properly accumulating the column counts of all these characters together (which I believe is more close to what this code originally intended to do). There are still many issues existing in both CustomTextLayout as well as the TextBuffer, and the correct solution to them will likely demand large-scale changes, at least at the scale of #3578. I wish small changes like this can serve as a stop gap solution while we take our time to work on the long-term right thing. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Builds and runs. Manual testing confirmed that it solves #696 with both LigConsalata and Fixedsys Excelsior. (cherry picked from commit 027f122)
|
🎉This issue was addressed in #4081, which has now been successfully released as Handy links: |
This was referenced Feb 12, 2020
DHowett-MSFT
pushed a commit
that referenced
this issue
Feb 12, 2020
ghost
added
In-PR
ghost
added
Resolution-Fix-Committed
ghost
pushed a commit
that referenced
this issue
Mar 2, 2020
## 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 #4438 Co-authored-by: Mili (Yi) Zhang <[email protected]> - Related to #4704 (#4731) ## PR Checklist * [x] Closes #696 * [x] Closes #4375 * [x] Closes #4708 * [x] Closes a crash that @DHowett-MSFT complained about with `"x" * ($Host.UI.RawUI.BufferSize.Width - 1) + "`u{241b}"` * [x] Eliminates an exception getting thrown with the U+1F3E0 emoji in `_CorrectGlyphRun` * [x] Corrects several graphical issues that occurred after #4731 was merged to master (weird repeats and splits of runs) * [x] I work here. * [x] Tested manually versus given scenarios. * [x] Documentation written into comments in the code. * [x] I'm a core contributor. ## 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 `ScaleCorrection`s 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
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Your Windows build number:
10.0.18362.86
What you're doing and what's happening: (Copy & paste specific commands and their output, or include screen shots)
I'm using Fixedsys Excelsio font, and this happened:
What's wrong / what should be happening instead:
The text turned to small text after typing "--" with Fixedsys Excelsio font
This font in VSCode
The text was updated successfully, but these errors were encountered: