The Wayback Machine - https://web.archive.org/web/20210123115909/https://github.com/microsoft/terminal/pull/5743
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

Scale box drawing glyphs to fit cells for visual bliss #5743

Merged
merged 16 commits into from May 8, 2020
Merged

Conversation

@miniksa
Copy link
Member

@miniksa miniksa commented May 4, 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

  • Closes #455
  • I work here.
  • Manual tests. This is all graphical.
  • Metric ton of comments
  • Math spreadsheet included in PR.
  • Double check RTL glyphs.
  • Why is there the extra pixel?
  • Scrolling the mouse wheel check is done.
  • Not drawing outline?
  • Am core contributor. Roar.
  • 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

Also see the below one with more screenshots:

miniksa added 3 commits Apr 27, 2020
…and descent to center glyphs vertically (as much as possible).
…ate CustomTextLayout to add a new analyzer pass to detect box drawing layouts and store a BoxDrawingEffect in the run. Add capability for Runs to hold a custom drawing effect. Pipe drawing effects all the way down. Make the glyph run drawing engine detect when a custom effect is applied and call a special method to perform the effect. Implement special effect for box drawing run based on the ol' manual glyph run drawing method, adjusting it to draw the outline as well as fill the geometries and also apply both a scale factor as well as the existing translation to the geometries.
@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on e0c4d28 Apr 29, 2020

New misspellings found, please review:

  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/e0c4d28e29030aa65960cf39ceb98cf3da4b8922.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on 3467dc0 Apr 29, 2020

New misspellings found, please review:

  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/3467dc0d99e2528df484085bb022fe733c58365a.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on 3d92b73 May 1, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/3d92b73222f25a90b77a2f79b60ff09a9e7c5bff.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on fe4e1c7 May 1, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/fe4e1c7fa02a38b158ca331f8785b7bc1c1217e1.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on f6e4542 May 2, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/f6e4542340f100762573f8ce8f95887d7724dd08.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

…nd performing the analysis after the fallback font has been adjusted to make its advances align with the primary font (by applying a font size scalar).
@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on 5d66def May 4, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/5d66defbad9b8a4f91a28fb242671cde65adbe84.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on 9310a5e May 4, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/9310a5e237349179ec5c3d21e16e304d36fb61d8.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented May 4, 2020

nit: imperative

@miniksa miniksa changed the title Scales box drawing glyphs to fit cells for visual bliss Scale box drawing glyphs to fit cells for visual bliss May 4, 2020
@github-actions

This comment has been hidden.

Copy link

@github-actions github-actions bot commented on 33fcdff May 5, 2020

New misspellings found, please review:

  • drwaing
  • IBox
  • oaidl
  • ocidl
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
drwaing
IBox
oaidl
ocidl
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/33fcdff9f6a3b77f6f994c9eab3b3d6f6f7e73ac.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

src/renderer/dx/IBoxDrawingEffect.idl Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxRenderer.cpp Show resolved Hide resolved
src/renderer/dx/DxRenderer.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 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 Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
miniksa added 2 commits May 6, 2020
…Use a move to save an addref/release on returning the Effect.
@miniksa miniksa marked this pull request as ready for review May 7, 2020
@miniksa
Copy link
Member Author

@miniksa miniksa commented May 7, 2020

@DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented May 7, 2020

idl didn't wanna build in CI. Fascinating!

…ully not generate useless proxy.
@miniksa
Copy link
Member Author

@miniksa miniksa commented May 8, 2020

I know this is all supposed to be vertical metrics, and I hate to be such a one-note Charlie here, but can you just run a quick check that RTL is still fine with this? It just touches a lot of the previous trouble spots

@schorrm, Lookin' good to me (from that old utf82.txt file):
image

@miniksa
Copy link
Member Author

@miniksa miniksa commented May 8, 2020

Screenshots (with don't scale to below 1.0f)
Font Name 9e42fc4
Cascadia Code image
Cascadia Patched image
Cascadia MetricsTest image
Consolas image
Lucida Console image
Anonymous Pro image
DejaVu Sans Mono image
Envy Code R image
Fira Code image
IBM 3270 image
Iosevka image
monofur image
Source Code Pro image
@miniksa
Copy link
Member Author

@miniksa miniksa commented May 8, 2020

OK, so, there's actually still one more thing:

  • Minor banding shows up at some font sizes (and in Anonymous Pro)

But I'm going to pull that into a new issue that may come post 1.0. This is so much better than it was for a majority of cases that I don't want to hold it up any longer.

@miniksa miniksa merged commit 70867df into master May 8, 2020
7 checks passed
7 checks passed
Spell checking
Details
Terminal CI Build #0.0.2005.0803 succeeded
Details
Terminal CI (Audit Mode Static Analysis Build x64) Audit Mode Static Analysis Build x64 succeeded
Details
Terminal CI (Build x64 Build x64 Release) Build x64 Build x64 Release succeeded
Details
Terminal CI (Code Health Scripts Proper Code Formatting Check) Code Health Scripts Proper Code Formatting Check succeeded
Details
auto-merge.config.enforce No dynamic merge policies are applicable.
license/cla All CLA requirements met.
Details
@miniksa miniksa deleted the dev/miniksa/box branch May 8, 2020
DHowett-MSFT pushed a commit that referenced this pull request 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)
DHowett-MSFT pushed a commit that referenced this pull request May 11, 2020
#5743 introduced a dependency from _any consumer of the DX header_ to a header generated from an IDL file.

Fixes #5819.
DHowett-MSFT pushed a commit that referenced this pull request May 12, 2020
#5743 introduced a dependency from _any consumer of the DX header_ to a header generated from an IDL file.

Fixes #5819.

(cherry picked from commit e088f73)
@msftbot
Copy link
Contributor

@msftbot msftbot bot commented May 13, 2020

🎉Windows Terminal Release Candidate v0.11.1333.0 (1.0rc2) has been released which incorporates this pull request.🎉

Handy links:

jelster added a commit to jelster/terminal that referenced this pull request 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)
jelster added a commit to jelster/terminal that referenced this pull request May 28, 2020
microsoft#5743 introduced a dependency from _any consumer of the DX header_ to a header generated from an IDL file.

Fixes microsoft#5819.
donno2048 added a commit to donno2048/terminal that referenced this pull request 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