Skip to content
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

Remember to have block and line drawing characters fill their cells properly #455

Closed
PhMajerus opened this issue May 7, 2019 · 41 comments · Fixed by #5743
Closed

Remember to have block and line drawing characters fill their cells properly #455

PhMajerus opened this issue May 7, 2019 · 41 comments · Fixed by #5743
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) 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

@PhMajerus
Copy link

PhMajerus commented May 7, 2019

Just a reminder for the new Windows Terminal, remember to make sure block and line drawing characters fill their cells properly at all zoom levels.
This is really important for ANSI-art and TUI apps.

ANSIart conhost
ANSIart WinTerm1
ANSIart WinTerm2

See color bleeding and horizontal lines in the larger Windows Terminal tab.
Also, some characters are randomly replaced by � (<?>) characters when changing zoom level or when printing out lot of VT markup.

@zadjii-msft
Copy link
Member

You know, I coulda swore @miniksa had a PR for this just last week, but I dunno what happened to it...

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Work-Item It's being tracked by an actual work item internally. (to be removed soon) labels May 7, 2019
@zadjii-msft zadjii-msft added this to the Windows Terminal v1.0 milestone May 7, 2019
@PhMajerus
Copy link
Author

PhMajerus commented May 7, 2019

Some more examples, comparing mlterm (running in Ubuntu on WSL with X410) with Windows Terminal

AnsiTestPattern mlterm
AnsiTestPattern WinTerm

See it exhibits not only the block and line characters alignments and joints issues, but also identical colors rendering as different colors (the whole ANSI-art is using only the 16 palettized VT colors).
Note also in the center, black (\e[40m) rendering as acrylic transparency.

You can get the test file from the following repository : https://github.com/PhMajerus/ANSI-art/blob/master/TestPattern%20ANSI.ans

@PhMajerus
Copy link
Author

PhMajerus commented May 11, 2019

The identical colors rendering as different colors mentioned above has been diagnosed in #677 and hopefully fixed in #688.

@ghosty141
Copy link
Contributor

I just tested it and it looks like the colors are now matching correctly. So, this is gonna be fixed in #688.

WindowsTerminal_2019-05-11_14-13-00

@ghosty141
Copy link
Contributor

ghosty141 commented May 11, 2019

I did find another bug though, look at how some characters get rendered incorrectly if I didn't execute another command before showing the test pattern.
For example:
show pic -> incorrect chars
clear -> show pic -> incorrect chars
clear -> any command (even clear) -> show pic -> correct output.

2019-05-11_14-44-24

One more thing, try zooming in (Ctrl mwheel) and at the right level of magnification the output looks perfectly fine.

@PhMajerus
Copy link
Author

PhMajerus commented May 11, 2019

@ghosty141 thanks for fixing and testing it, that's one thing out of the way.

What you're observing are the two other issues I described in the first message.

The terminal cells and the size of the block drawing characters do not match at all zoom levels, making them overlap or not fill their cell, creating visual horizontal lines.
This is easily observed if you display █▀▄░▒▓, their heights won't be consistent. Strangely, trying these at different sizes in Word with the Consolas font shows them always filling the exact same rectangle, only Conhost and Terminal seems to stretch these characters at some sizes, sometimes by as much as 2x.

The last issue with the � characters appearing a bit randomly seems like a bug in Terminal itself, and is probably in the rendering engine as it depends on the zoom level and at the same time seems to always appear at the exact same place for the exact same buffer to render.
Note that � could be U+FFFD, the "replacement character". So it could be drawn by some API when requested to draw invalid data.

@miniksa miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) and removed Work-Item It's being tracked by an actual work item internally. (to be removed soon) labels May 14, 2019
@miniksa miniksa self-assigned this May 14, 2019
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@miniksa miniksa added Product-Terminal The new Windows Terminal. and removed Mass-Chaos labels May 17, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 18, 2019
@miniksa
Copy link
Member

miniksa commented May 21, 2019

@PhMajerus, I'm aware of the randomish U+FFFD issue. I have an outstanding task to try to debug why that's happening. I'll get to it eventually.

@shanselman
Copy link
Member

Interesting, is this related also to the way that F7 in cmd.exe renders itself with spaces/lines?
image

@miniksa
Copy link
Member

miniksa commented May 21, 2019

F7 is implemented in the COOKED_READ logic for CMD.exe and actually changes which symbols it uses for the outline based on codepage... or at least it used to.

@miniksa
Copy link
Member

miniksa commented Apr 27, 2020

OK, so, the line gap thing isn't really working. So now I'm going crazy in Excel finding something that I can use to adjust the baseline, the overall height, or whatnot either for the entire font or for the glyphs only in the U+2500-U+259F range (with the ascenderOffset flag inside the CustomTextLayout)

@miniksa
Copy link
Member

miniksa commented Apr 27, 2020

Another note: Anonymous Pro has line drawing chars but no box drawing chars, so that's a fallback and probably why it's looking extra weird.

@PhMajerus
Copy link
Author

@miniksa This is amazing compared to the previous rendering, great work!
The shading blocks are a very difficult case, and if you look again on my first screenshot of that test pattern using mlterm, you'll see they don't line up perfectly either.

The problem is the original EGA/VGA font (8x14 font in ROM on old graphic cards) had the pattern perfect for alignment, but even the CGA (8x8) was somewhat off for the dark shade block. If I remember correctly, the default font used in Win95 console needed an alternate of shaded blocks to fit properly side by side (as in, inverting the foreground and background colors and using the opposite shading block to make their joins merge correctly using an invisible checkerboard pattern).

So I wouldn't worry too much about it for v1, the result you got already seems on par with other mature terminal emulators.

I find the colors narrowing a bigger problem for ANSI-art and Text UI, especially the convertion of black background to transparent background when they were explicitly black instead of CSI 49m.

Can you include the following in your tests ?
echo -e "\e[30m\u2590\e[47;7m#\e[27;49m\u258C\e[m"
Should display silver # symbol over a black square even with acrylic blur background, but it is shown as ▐#▌ instead, the background of the # cell being converted to transparent instead of staying black like the two sides.
image

@zadjii-msft
Copy link
Member

Can you include the following in your tests ?
echo -e "\e[30m\u2590\e[47;7m#\e[27;49m\u258C\e[m"

That might actually be #2661

@PhMajerus
Copy link
Author

@zadjii-msft
Yeah, your ColorType IsDefault flag should take care of it, but since we're getting close to v1 I'm trying to avoid something slipping through that gets noticed too late... Like #399 is still bothering me every single day in 20H1.

I just want to make sure you have a test case where the background color of a cell happens to be the exact same color as the default background color as seen by legacy conhost concepts, but needs to stay solid opaque, while CSI 49m gives acrylic transparency.
Really hope these VT stuff will all work for v1.

Thanks for all the work on fixing VT edge cases, It is looking great so far.

@miniksa
Copy link
Member

miniksa commented Apr 30, 2020

OK I spent all day fiddling around.

I have the math that matches the results from the states above.

Unfortunately I put the "with gap correction" to the left of "w/o gap correction"... but it goes as follows.

If there are 3 green boxes next to each other for that font... it's going to look fine in the Terminal.
If there are not, it will gap somewhere. (2nd and 3rd columns describe where it's gapping.)

image

The left most one is green if the designed height of the █ glyph is tall enough to fit the cell space we've given it after we've ceiling'd the baseline so it lands on a solid pixel and ceiling'd out the cell as a whole to an integer height.

(We integer-ify the baseline to ensure that the bottom of characters sits nice and crisply on a pixel. We integer-ify the cell height out to a perfect integer to enable scrolling offload.)

The middle column is green if the top of the █ glyph will touch or stretch outside the top of the cell after it's been drawn at the modified baseline.
The right most column is green if the bottom of the █ glyph will touch or stretch outside the bottom of the cell after it's been drawn at the modified baseline.

There were 4 triple-greens in the old algorithm (which aligned with the specific fonts in the test data). And there are 3 triple-greens in the linegap-adjust algorithm (also aligning with the specific fonts in the test data.)

I believe there's two steps to success here:

  1. Given that boxes/lines don't specifically care about the rounded baseline for the solid pixel, translate the glyphs back upward to where they should have been. Match the centerpoint of the glyph to the centerpoint of the cell.
  2. For glyphs that still wouldn't be big enough after translating them into the centerpoint of the cell, use a scale transform to fill the cell (or slightly overscan it by a few DU to guarantee it looks good).

Beyond that, I've written some code today that:

  1. Adds an analysis step to CustomTextLayout that can identify runs of box/line drawing characters and segment them out into their own runs.
  2. Tag all of these identified runs with a payload object that can direct custom rendering.
  3. Added the custom rendering object to the runs so it is carried through.
  4. Piped custom rendering objects the whole way down through CustomTextRenderer
  5. Repurposed the unused "draw glyphs manually" method to become our box drawing method. Corrected it to draw the outlines and the fills. Set sample translate/scale transformations between geometry source and sinks. Tested this out manually and showed that it visually looks correct... as long as we have the right metrics.

My goal for Friday will be:

  1. Take the math out of the Excel spreadsheet and code it into the analyzer so it can determine the translation and/or scale factors needed to adjust box glyphs so they look proper.
  2. Expand the custom object to hold this data and carry it down to the renderer.
  3. Update and verify this works against the custom text renderer by filling the geometry transform matrix.
    Then refine by:
    A. Don't do all this if they would already fit and look just fine. What a performance nightmare!
    B. See if I can generalize the box/line drawing data into character classes that can be loaded/cached on font change instead of loaded in real time.
    C. Ensure this still works for font fallback scenarios as some of these fonts implement SOME of the line/box glyphs and not others and we'd want the fallback glyphs to look decent while we're doing all this complex work anyway.

And the backup plan is:

  • Instead of actually determining scale/translate transform metrics for the geometries, instead implement a U+2500-U+259F-part switch case that just draws them with FillRectangle instead and ignores the glyphs in the fonts. I don't want to do this because I like the style/flavor that fonts can provide here. But it's a backup plan to all this math.

Either the main or backup plan solution should eliminate all gapping in both the X and Y dimensions for all of these glyphs no matter what font is used and no matter if that font implements all of these glyphs or not.

@PhMajerus
Copy link
Author

PhMajerus commented Apr 30, 2020

That’s a lot of work to work around font rendering issues.

It seems that having a lot of complicated mitigation code might make it more difficult for font designers to test their fonts, as they might be tinkering with their design to try to make something that looks good in a few test apps, and the different heuristics used by the code might appear random from the font designer point of view. I believe we need a switch to explicitly turn off all mitigation code paths so they can test their fonts against the more predictable optimized renderer and try to build a font that doesn’t rely on fallback paths. That would be both in settings.json and at the renderer XAML control level for other apps hosting the Terminal.

About using the font glyphs versus a fixed set of block elements and line drawing glyphs, I agree using the font to allow fonts to have their own identities.

For that idea, I have a suggestion that I believe would provide added value to the Windows Terminal in general and provide a fallback mechanism for our current issue when needed.
Most fonts won’t provide all the glyphs that the Terminal can display, we’ll have Latin font, CJK font, Arabic font, Korean font,… at this time we can select a single font, and the Terminal uses some other fallback font for missing characters. But how about letting the user specify a set of fonts by Unicode blocks/ranges? Just like MS Word allows the user to select a Latin font and another Asian text font, the Terminal could allow the user to select a main font as well as explicit alternate fonts for the CJK block, the Arabic block, the Math symbols block, and of course, the Block elements and Box drawing blocks. This would provide better control over the look of mixed scripts in the Terminal, and allow an optimized font that doesn’t need the performance hit of the fallbacks to be used for blocks and lines if the user’s favorite font doesn’t provide good ones.
Instead of a custom-drawing FillRectangle-based solution, Windows Terminal could ship with a font that provides just the block elements and line drawing glyphs to be used only for those Unicode blocks that works with the optimal rendering code path.
This also has the added benefit of making it possible to separate things like the Powerline and Nerd glyphs into separate fonts. Instead of merging them into special versions of Cascadia Code, and missing them when using a font that didn’t merge these glyphs, the Terminal could simply use Powerline-only and Nerd-only fonts for those Unicode blocks/ranges according to the same user-defined “fonts set”.
It makes even more sense with the extra Symbols for Legacy Computing added to Unicode 13.0 (https://www.unicode.org/charts/PDF/U1FB00.pdf), as we could have for example Commodore 64, Apple II, Atari 400/800, etc... fonts designed just for the block that provides the legacy computing symbols.

@miniksa
Copy link
Member

miniksa commented May 1, 2020

Thanks for your response, @PhMajerus. I still believe it's up to me to fix it in a complex way because I'm the one breaking it in the first place for many of these fonts. I'm the one adding additional vertical line padding in some cases and throwing off the prescribed dimensions of the fonts. I do it in order to use the scrolling mechanisms of Present1, maintain the integer-pixel cell dimensions that most of our code and client applications rely on, and to try to preserve the crispness of the glyphs through these adjustments. I know the "just don't mess with it" solution would probably resolve this on its own, but then I'd have to give back all the performance I got from differential drawing and Present1 and I'd have a new host of problems with hit testing and line wrapping and API-mapping.

I do believe in adding a fallback mechanism including being able to specify which ranges it applies to, but that's definitely going to come after 1.0 with the complexity of the settings configuration required. Such a thing will also still require me to adjust the analysis steps to further parse and identify the ranges to apply each font to, so that would have to be done for either path.

Finally... I think adding a setting to turn it off might be advisable and possible.

@miniksa
Copy link
Member

miniksa commented May 1, 2020

OK as of this (fe4e1c7), here's what we have:
EDIT: also as of this (f6e4542), the next column:

Test fonts:

Screenshots
Font Name fe4e1c7f f6e45423
Cascadia Code image image
Cascadia Patched image image
3270-Medium image image
Anonymous Pro image image
DejaVu Sans Mono image image
Envy Code R image image
Fira Code image image
Iosevka image image
monofur image image
Source Code Pro image image
Font Name fe4e1c7f f6e45423
Cascadia Code ✔️ ✔️
Cascadia Patched ✔️ ✔️
3270-Medium ✔️ ✔️
Anonymous Pro ❌ (fallback)
DejaVu Sans Mono ❌ (vertical adjust?) ✔️
Envy Code R ✔️ ✔️
Fira Code ✔️ ✔️
Iosevka ✔️ ✔️
monofur ✔️ ✔️
Source Code Pro ❌ (vertical adjust) ✔️
  • - Figure out what DejaVu Sans Mono's deal is - Scale + translation
  • - Figure out vertical adjust in Source Code Pro - Scale + translation
  • - Find the weird extra horizontal pixel here and there
  • - Make this work down font fallback route
  • - Use cached adjustment value if font fallback font matches the O.G. font

@miniksa
Copy link
Member

miniksa commented May 4, 2020

Got the font fallback part. Generating draft PR now. Here's an Anonymous Pro sample. Not perfect, but pretty darned good.

EDIT: Do note that most of the box glyphs are fallbacks from Segoe UI Symbol.

image

@ghost ghost added the In-PR This issue has a related PR label May 4, 2020
@miniksa
Copy link
Member

miniksa commented May 8, 2020

OK, I've solved most of it. There's still a few circumstances with a minor vertical banding between blocks in the horizontal direction, but I've filed #5816 as a follow on for that because the PR at #5743 adds so much goodness for a majority of cases.

miniksa added a commit that referenced this issue May 8, 2020
## Summary of the Pull Request
Identifies and scales glyphs in the box and line drawing ranges U+2500-U+259F to fit their cells.

## PR Checklist
* [x] Closes #455
* [x] I work here.
* [x] Manual tests. This is all graphical.
* [x] Metric ton of comments
* [x] Math spreadsheet included in PR.
* [x] Double check RTL glyphs.
* [x] Why is there the extra pixel?
* [x] Scrolling the mouse wheel check is done.
* [x] Not drawing outline?
* [x] Am core contributor. Roar.
* [x] Try suppressing negative scale factors and see if that gets rid of weird shading.

## Detailed Description of the Pull Request / Additional comments

### Background
- We want the Terminal to be fast at drawing. To be fast at drawing, we perform differential drawing, or only drawing what is different from the previous frame. We use DXGI's `Present1` method to help us with this as it helps us compose only the deltas onto the previous frame at drawing time and assists us in scrolling regions from the previous frame without intervention. However, it only works on strictly integer pixel row heights.
- Most of the hit testing and size-calculation logic in both the `conhost` and the Terminal products are based on the size of an individual cell. Historically, a cell was always dictated in a `COORD` structure, or two `SHORT` values... which are integers. As such, when we specify the space for any individual glyph to be displayed inside our terminal drawing region, we want it to fall perfectly inside of an integer box to ensure all these other algorithms work correctly and continue to do so.
- Finally, we want the Terminal to have font fallback and locate glyphs that aren't in the primary selected font from any other font it can find on the system that contains the glyph, per DirectWrite's font fallback mechanisms. These glyphs won't necessarily have the same font or glyph metrics as the base font, but we need them to fit inside the same cell dimensions as if they did because the hit testing and other algorithms aren't aware of which particular font is sourcing each glyph, just the dimensions of the bounding box per cell.

### How does Terminal deal with this?
- When we select a font, we perform some calculations using the design metrics of the font and glyphs to determine how we could fit them inside a cell with integer dimensions. Our process here is that we take the requested font size (which is generally a proxy for height), find the matching glyph width for that height then round it to an integer. We back convert from that now integer width to a height value which is almost certainly now a floating point number. But because we need an integer box value, we add line padding above and below the glyphs to ensure that the height is an integer as well as the width. Finally, we don't add the padding strictly equally. We attempt to align the English baseline of the glyph box directly onto an integer pixel multiple so most characters sit crisply on a line when displayed. 
- Note that fonts and their glyphs have a prescribed baseline, line gap, and advance values. We use those as guidelines to get us started, but then to meet our requirements, we pad out from those. This results in fonts that should be properly authored showing gaps. It also results in fonts that are improperly authored looking even worse than they normally would.

### Now how does block and line drawing come in?
- Block and Line drawing glyphs are generally authored so they will look fine when the font and glyph metrics are followed exactly as prescribed by the font. (For some fonts, this still isn't true and we want them to look fine anyway.)
- When we add additional padding or rounding to make glyphs fit inside of a cell, we can be adding more space than was prescribed around these glyphs. This can cause a gap to be visible.
- Additionally, when we move things like baselines to land on a perfect integer pixel, we may be drawing a glyph lower in the bounding box than was prescribed originally.

### And how do we solve it?
- We identify all glyphs in the line and block drawing ranges.
- We find the bounding boxes of both the cell and the glyph.
- We compare the height of the glyph to the height of the cell to see if we need to scale. We prescribe a scale transform if the glyph wouldn't be tall enough to fit the box. (We leave it alone otherwise as some glyphs intentionally overscan the box and scaling them can cause banding effects.)
- We inspect the overhang/underhang above and below the boxes and translate transform them (slide them) so they cover the entire cell area.
- We repeat the previous two steps but in the horizontal direction.

## Validation Steps Performed
- See these commments:
   - #455 (comment)
   - #455 (comment)
   - #455 (comment)

Also see the below one with more screenshots:
   - #5743 (comment)
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 8, 2020
DHowett-MSFT pushed a commit that referenced this issue May 8, 2020
## Summary of the Pull Request
Identifies and scales glyphs in the box and line drawing ranges U+2500-U+259F to fit their cells.

## PR Checklist
* [x] Closes #455
* [x] I work here.
* [x] Manual tests. This is all graphical.
* [x] Metric ton of comments
* [x] Math spreadsheet included in PR.
* [x] Double check RTL glyphs.
* [x] Why is there the extra pixel?
* [x] Scrolling the mouse wheel check is done.
* [x] Not drawing outline?
* [x] Am core contributor. Roar.
* [x] Try suppressing negative scale factors and see if that gets rid of weird shading.

## Detailed Description of the Pull Request / Additional comments

### Background
- We want the Terminal to be fast at drawing. To be fast at drawing, we perform differential drawing, or only drawing what is different from the previous frame. We use DXGI's `Present1` method to help us with this as it helps us compose only the deltas onto the previous frame at drawing time and assists us in scrolling regions from the previous frame without intervention. However, it only works on strictly integer pixel row heights.
- Most of the hit testing and size-calculation logic in both the `conhost` and the Terminal products are based on the size of an individual cell. Historically, a cell was always dictated in a `COORD` structure, or two `SHORT` values... which are integers. As such, when we specify the space for any individual glyph to be displayed inside our terminal drawing region, we want it to fall perfectly inside of an integer box to ensure all these other algorithms work correctly and continue to do so.
- Finally, we want the Terminal to have font fallback and locate glyphs that aren't in the primary selected font from any other font it can find on the system that contains the glyph, per DirectWrite's font fallback mechanisms. These glyphs won't necessarily have the same font or glyph metrics as the base font, but we need them to fit inside the same cell dimensions as if they did because the hit testing and other algorithms aren't aware of which particular font is sourcing each glyph, just the dimensions of the bounding box per cell.

### How does Terminal deal with this?
- When we select a font, we perform some calculations using the design metrics of the font and glyphs to determine how we could fit them inside a cell with integer dimensions. Our process here is that we take the requested font size (which is generally a proxy for height), find the matching glyph width for that height then round it to an integer. We back convert from that now integer width to a height value which is almost certainly now a floating point number. But because we need an integer box value, we add line padding above and below the glyphs to ensure that the height is an integer as well as the width. Finally, we don't add the padding strictly equally. We attempt to align the English baseline of the glyph box directly onto an integer pixel multiple so most characters sit crisply on a line when displayed.
- Note that fonts and their glyphs have a prescribed baseline, line gap, and advance values. We use those as guidelines to get us started, but then to meet our requirements, we pad out from those. This results in fonts that should be properly authored showing gaps. It also results in fonts that are improperly authored looking even worse than they normally would.

### Now how does block and line drawing come in?
- Block and Line drawing glyphs are generally authored so they will look fine when the font and glyph metrics are followed exactly as prescribed by the font. (For some fonts, this still isn't true and we want them to look fine anyway.)
- When we add additional padding or rounding to make glyphs fit inside of a cell, we can be adding more space than was prescribed around these glyphs. This can cause a gap to be visible.
- Additionally, when we move things like baselines to land on a perfect integer pixel, we may be drawing a glyph lower in the bounding box than was prescribed originally.

### And how do we solve it?
- We identify all glyphs in the line and block drawing ranges.
- We find the bounding boxes of both the cell and the glyph.
- We compare the height of the glyph to the height of the cell to see if we need to scale. We prescribe a scale transform if the glyph wouldn't be tall enough to fit the box. (We leave it alone otherwise as some glyphs intentionally overscan the box and scaling them can cause banding effects.)
- We inspect the overhang/underhang above and below the boxes and translate transform them (slide them) so they cover the entire cell area.
- We repeat the previous two steps but in the horizontal direction.

## Validation Steps Performed
- See these commments:
   - #455 (comment)
   - #455 (comment)
   - #455 (comment)

Also see the below one with more screenshots:
   - #5743 (comment)

(cherry picked from commit 70867df)
@ghost
Copy link

ghost commented May 13, 2020

🎉This issue was addressed in #5743, which has now been successfully released as Windows Terminal Release Candidate v0.11.1333.0 (1.0rc2).:tada:

Handy links:

jelster pushed a commit to jelster/terminal that referenced this issue May 28, 2020
## Summary of the Pull Request
Identifies and scales glyphs in the box and line drawing ranges U+2500-U+259F to fit their cells.

## PR Checklist
* [x] Closes microsoft#455
* [x] I work here.
* [x] Manual tests. This is all graphical.
* [x] Metric ton of comments
* [x] Math spreadsheet included in PR.
* [x] Double check RTL glyphs.
* [x] Why is there the extra pixel?
* [x] Scrolling the mouse wheel check is done.
* [x] Not drawing outline?
* [x] Am core contributor. Roar.
* [x] Try suppressing negative scale factors and see if that gets rid of weird shading.

## Detailed Description of the Pull Request / Additional comments

### Background
- We want the Terminal to be fast at drawing. To be fast at drawing, we perform differential drawing, or only drawing what is different from the previous frame. We use DXGI's `Present1` method to help us with this as it helps us compose only the deltas onto the previous frame at drawing time and assists us in scrolling regions from the previous frame without intervention. However, it only works on strictly integer pixel row heights.
- Most of the hit testing and size-calculation logic in both the `conhost` and the Terminal products are based on the size of an individual cell. Historically, a cell was always dictated in a `COORD` structure, or two `SHORT` values... which are integers. As such, when we specify the space for any individual glyph to be displayed inside our terminal drawing region, we want it to fall perfectly inside of an integer box to ensure all these other algorithms work correctly and continue to do so.
- Finally, we want the Terminal to have font fallback and locate glyphs that aren't in the primary selected font from any other font it can find on the system that contains the glyph, per DirectWrite's font fallback mechanisms. These glyphs won't necessarily have the same font or glyph metrics as the base font, but we need them to fit inside the same cell dimensions as if they did because the hit testing and other algorithms aren't aware of which particular font is sourcing each glyph, just the dimensions of the bounding box per cell.

### How does Terminal deal with this?
- When we select a font, we perform some calculations using the design metrics of the font and glyphs to determine how we could fit them inside a cell with integer dimensions. Our process here is that we take the requested font size (which is generally a proxy for height), find the matching glyph width for that height then round it to an integer. We back convert from that now integer width to a height value which is almost certainly now a floating point number. But because we need an integer box value, we add line padding above and below the glyphs to ensure that the height is an integer as well as the width. Finally, we don't add the padding strictly equally. We attempt to align the English baseline of the glyph box directly onto an integer pixel multiple so most characters sit crisply on a line when displayed. 
- Note that fonts and their glyphs have a prescribed baseline, line gap, and advance values. We use those as guidelines to get us started, but then to meet our requirements, we pad out from those. This results in fonts that should be properly authored showing gaps. It also results in fonts that are improperly authored looking even worse than they normally would.

### Now how does block and line drawing come in?
- Block and Line drawing glyphs are generally authored so they will look fine when the font and glyph metrics are followed exactly as prescribed by the font. (For some fonts, this still isn't true and we want them to look fine anyway.)
- When we add additional padding or rounding to make glyphs fit inside of a cell, we can be adding more space than was prescribed around these glyphs. This can cause a gap to be visible.
- Additionally, when we move things like baselines to land on a perfect integer pixel, we may be drawing a glyph lower in the bounding box than was prescribed originally.

### And how do we solve it?
- We identify all glyphs in the line and block drawing ranges.
- We find the bounding boxes of both the cell and the glyph.
- We compare the height of the glyph to the height of the cell to see if we need to scale. We prescribe a scale transform if the glyph wouldn't be tall enough to fit the box. (We leave it alone otherwise as some glyphs intentionally overscan the box and scaling them can cause banding effects.)
- We inspect the overhang/underhang above and below the boxes and translate transform them (slide them) so they cover the entire cell area.
- We repeat the previous two steps but in the horizontal direction.

## Validation Steps Performed
- See these commments:
   - microsoft#455 (comment)
   - microsoft#455 (comment)
   - microsoft#455 (comment)

Also see the below one with more screenshots:
   - microsoft#5743 (comment)
donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
## Summary of the Pull Request
Identifies and scales glyphs in the box and line drawing ranges U+2500-U+259F to fit their cells.

## PR Checklist
* [x] Closes #455
* [x] I work here.
* [x] Manual tests. This is all graphical.
* [x] Metric ton of comments
* [x] Math spreadsheet included in PR.
* [x] Double check RTL glyphs.
* [x] Why is there the extra pixel?
* [x] Scrolling the mouse wheel check is done.
* [x] Not drawing outline?
* [x] Am core contributor. Roar.
* [x] Try suppressing negative scale factors and see if that gets rid of weird shading.

## Detailed Description of the Pull Request / Additional comments

### Background
- We want the Terminal to be fast at drawing. To be fast at drawing, we perform differential drawing, or only drawing what is different from the previous frame. We use DXGI's `Present1` method to help us with this as it helps us compose only the deltas onto the previous frame at drawing time and assists us in scrolling regions from the previous frame without intervention. However, it only works on strictly integer pixel row heights.
- Most of the hit testing and size-calculation logic in both the `conhost` and the Terminal products are based on the size of an individual cell. Historically, a cell was always dictated in a `COORD` structure, or two `SHORT` values... which are integers. As such, when we specify the space for any individual glyph to be displayed inside our terminal drawing region, we want it to fall perfectly inside of an integer box to ensure all these other algorithms work correctly and continue to do so.
- Finally, we want the Terminal to have font fallback and locate glyphs that aren't in the primary selected font from any other font it can find on the system that contains the glyph, per DirectWrite's font fallback mechanisms. These glyphs won't necessarily have the same font or glyph metrics as the base font, but we need them to fit inside the same cell dimensions as if they did because the hit testing and other algorithms aren't aware of which particular font is sourcing each glyph, just the dimensions of the bounding box per cell.

### How does Terminal deal with this?
- When we select a font, we perform some calculations using the design metrics of the font and glyphs to determine how we could fit them inside a cell with integer dimensions. Our process here is that we take the requested font size (which is generally a proxy for height), find the matching glyph width for that height then round it to an integer. We back convert from that now integer width to a height value which is almost certainly now a floating point number. But because we need an integer box value, we add line padding above and below the glyphs to ensure that the height is an integer as well as the width. Finally, we don't add the padding strictly equally. We attempt to align the English baseline of the glyph box directly onto an integer pixel multiple so most characters sit crisply on a line when displayed. 
- Note that fonts and their glyphs have a prescribed baseline, line gap, and advance values. We use those as guidelines to get us started, but then to meet our requirements, we pad out from those. This results in fonts that should be properly authored showing gaps. It also results in fonts that are improperly authored looking even worse than they normally would.

### Now how does block and line drawing come in?
- Block and Line drawing glyphs are generally authored so they will look fine when the font and glyph metrics are followed exactly as prescribed by the font. (For some fonts, this still isn't true and we want them to look fine anyway.)
- When we add additional padding or rounding to make glyphs fit inside of a cell, we can be adding more space than was prescribed around these glyphs. This can cause a gap to be visible.
- Additionally, when we move things like baselines to land on a perfect integer pixel, we may be drawing a glyph lower in the bounding box than was prescribed originally.

### And how do we solve it?
- We identify all glyphs in the line and block drawing ranges.
- We find the bounding boxes of both the cell and the glyph.
- We compare the height of the glyph to the height of the cell to see if we need to scale. We prescribe a scale transform if the glyph wouldn't be tall enough to fit the box. (We leave it alone otherwise as some glyphs intentionally overscan the box and scaling them can cause banding effects.)
- We inspect the overhang/underhang above and below the boxes and translate transform them (slide them) so they cover the entire cell area.
- We repeat the previous two steps but in the horizontal direction.

## Validation Steps Performed
- See these commments:
   - microsoft/terminal#455 (comment)
   - microsoft/terminal#455 (comment)
   - microsoft/terminal#455 (comment)

Also see the below one with more screenshots:
   - microsoft/terminal#5743 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.