Repository: microsoft/terminal (language: cpp)

Issue:
Some fonts' italic text has wavy baselines
### Windows Terminal version

1.16.2523.0

### Windows build number

10.0.19043.0

### Other Software

neovim 0.7.2 (WSL2)
FiraCode NF, 12pt as Terminal Font
colorscheme github_dark

### Steps to reproduce

Open a java source code file and comments are wavy, non-comment code lines are as expected
![WavyCommentLines](https://user-images.githubusercontent.com/56093026/190098169-7a6dd970-e306-47a0-a6fd-73ec37e72f8e.jpg)


### Expected Behavior

Open a java source code file and comments look the same way as other code lines

### Actual Behavior

comment lines are wavy


Discussion / hints:
Looks like the text is just formatted as italic by neovim. I'm sure whatever neovim config you're using has options for configuring how different elements are rendered - you can always disable it there. 

I'm sure there's a way to use [font features](https://docs.microsoft.com/en-us/windows/terminal/customize-settings/profile-appearance#font-features) to disable italics, though, I don't know it off the top of my head. 
Hi @zadjii-msft ,

that's right, that the comments are formatted in italics, that's defined in the colorscheme. Good point with the configuration, I will try that.
But this only appears in Windows Terminal. With wezterm, the same code is rendered as it should be. Therefore, I think, this is still a bug.
![NoWavyCommentLines](https://user-images.githubusercontent.com/56093026/190145207-ef83c8a7-0881-466e-8d84-0f8911f3b53c.jpg)

@indika-dev Does wezterm support italics as emitted by client apps? @wez for some help. Maybe there's something in the default config that disables them for wezterm. (sorry for not being more familiar with wezterm. I probably should be.)
Sorry, but I don't know, what you mean with "...italics as emitted by client apps". What I found in the documentation is [this.](https://wezfurlong.org/wezterm/config/lua/config/font_rules.html) But I didn't configure this.
neovim itself does not configure fonts. The terminal renders the "output" of neovim.
I mean along the lines of #5461. Apps like vim can tell the Terminal "I want this text to be italicized" in the same way they tell the Terminal "I want this text to be red".
Yeah, wezterm supports italics (`CSI 3 m` to enable, `CSI 23 m` to disable). 

My read of this issue is that the problem is that the italics have a wavy baseline in the image in the OP, which suggests to me an issue with how the glyphs are rasterized/hinted, rather than an issue with understanding whether italics should be enabled.

If someone reported this to me in wezterm, I'd be curious about whether it was font specific: wezterm uses a feature of freetype to synthesize italics (by skewing the glyph) when no italic variant of the font is found, and that can behave differently from "normal" font rendering, and I wonder if there are parallels to that in the MS font shaper/rasterizer stack used in MS terminal?

> My read of this issue is that the problem is that the italics have a wavy baseline in the image in the 

goodness gracious, thanks for that. I clearly didn't have enough coffee this morning and just figured OP meant "italic" when they said "wavy". Yea, the baselines **do** look a little wavy, don't they. Huh. 

@indika-dev can you give us your whole [settings.json file](https://github.com/microsoft/terminal/wiki/Frequently-Asked-Questions-(FAQ)#where-can-i-find-the-settings-file)/? The exact set of settings should help with a repro here. 
> I clearly didn't have enough coffee this morning

It's still pretty early!
I took a look at #5461 and Nerd Fonts were mentioned. So, I tested a bit and it looks like, that the wavy lines only appear under certain Nerd Fonts:
- FiraCode NF, CascadiaCove PL and JetBrainsMono NF have wavy lines
- SauceCodePro NF and BlexMono Nerd Font have **no** wavy lines
> @indika-dev can you give us your whole [settings.json file](https://github.com/microsoft/terminal/wiki/Frequently-Asked-Questions-(FAQ)#where-can-i-find-the-settings-file)/? 

Sure, here it is.
[settings.zip](https://github.com/microsoft/terminal/files/9567119/settings.zip)

Hey @lhecker, is this related to your glyph scaler?
It sure is. FiraCode without adjusting glyph position:
![image](https://user-images.githubusercontent.com/2256941/190239573-f67d661c-f715-4067-ad04-926d9c13abf5.png)

With adjustments:
![image](https://user-images.githubusercontent.com/2256941/190239718-c96d53c6-3213-4993-9c3f-ad553528c713.png)

Thoughts:
* Scaling should be relative to the baseline and not to the center of the cell and would probably fix this issue. (But is this correct?)
* Currently talking internally with DirectWrite wizards why we're getting weird glyph metrics for italic Fira Code (**edit:** turns out that `IDWriteTextLayout::GetOverhangMetrics` returns incorrect/overzealous metrics for simulated italics)
  Fun fact: Fira Code has no italic variant. It's automatically generated by DirectWrite and that's the primary source of these issues you're seeing.
I see the same with Caskaydia Cove Nerd Font from https://www.nerdfonts.com/font-downloads (also using Neovim)

![image](https://user-images.githubusercontent.com/1232595/190325514-1699cba4-c38c-49ba-b9e7-90173843922c.png)

It appeared fine before Terminal was updated to 1.16 this morning.
@birtles That's because that font, just like FiraCode, doesn't ship with a proper italic variant. When DirectWrite "simulates" those italic glyphs, it doesn't correctly compute the so called "overhangs" (= how much a glyph is outside of the bounding box / cell) and this confuses the new text renderer.

The original Cascadia Code ships with an actual italic variant which looks quite different (easily visible on the "f" character) and works correctly (because it's not simulated):
![image](https://user-images.githubusercontent.com/2256941/190401980-e2b2dcc7-1f1a-4ddc-b1b2-7234dc2b2574.png)

I'm working on a fix, but I'm not 100% sure how to best approach it yet.

Fix patch:
diff --git a/src/renderer/atlas/AtlasEngine.h b/src/renderer/atlas/AtlasEngine.h
index 0793433b2f9..fe3b9712bb0 100644
--- a/src/renderer/atlas/AtlasEngine.h
+++ b/src/renderer/atlas/AtlasEngine.h
@@ -523,9 +523,9 @@ namespace Microsoft::Console::Render
         struct CachedGlyphLayout
         {
             wil::com_ptr<IDWriteTextLayout> textLayout;
-            f32x2 halfSize;
             f32x2 offset;
             f32x2 scale;
+            f32x2 scaleCenter;
             D2D1_DRAW_TEXT_OPTIONS options = D2D1_DRAW_TEXT_OPTIONS_NONE;
             bool scalingRequired = false;
 
diff --git a/src/renderer/atlas/AtlasEngine.r.cpp b/src/renderer/atlas/AtlasEngine.r.cpp
index e727570a4a4..381f43de4d6 100644
--- a/src/renderer/atlas/AtlasEngine.r.cpp
+++ b/src/renderer/atlas/AtlasEngine.r.cpp
@@ -472,10 +472,10 @@ void AtlasEngine::_drawGlyph(const TileHashMap::iterator& it) const
 AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t* chars, u16 charsLength, u16 cellCount, IDWriteTextFormat* textFormat, bool coloredGlyph) const
 {
     const f32x2 layoutBox{ cellCount * _r.cellSizeDIP.x, _r.cellSizeDIP.y };
-    const f32x2 halfSize{ layoutBox.x * 0.5f, layoutBox.y * 0.5f };
     bool scalingRequired = false;
     f32x2 offset{ 0, 0 };
     f32x2 scale{ 1, 1 };
+    f32x2 scaleCenter;
 
     // See D2DFactory::DrawText
     wil::com_ptr<IDWriteTextLayout> textLayout;
@@ -550,12 +550,18 @@ AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t*
                 static_cast<f32>(glyphMetrics.advanceHeight) / static_cast<f32>(metrics.designUnitsPerEm) * _r.fontMetrics.fontSizeInDIP,
             };
 
+            scalingRequired = true;
+            // We always want box drawing glyphs to be centered in their cell.
+            offset.x = (layoutBox.x - boxSize.x) * 0.5f;
+            offset.y = (layoutBox.y - boxSize.y) * 0.5f;
             // We always want box drawing glyphs to exactly match the size of a terminal cell.
-            // So for safe measure we'll always scale them to the exact size.
             // But add 1px to the destination size, so that we don't end up with fractional pixels.
-            scalingRequired = true;
-            scale.x = layoutBox.x / boxSize.x;
-            scale.y = layoutBox.y / boxSize.y;
+            scale.x = (layoutBox.x + _r.dipPerPixel) / boxSize.x;
+            scale.y = (layoutBox.y + _r.dipPerPixel) / boxSize.y;
+            // Now that the glyph is in the center of the cell thanks
+            // to the offset, the scaleCenter is center of the cell.
+            scaleCenter.x = layoutBox.x * 0.5f;
+            scaleCenter.y = layoutBox.y * 0.5f;
         }
     }
     else
@@ -563,16 +569,7 @@ AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t*
         DWRITE_OVERHANG_METRICS overhang;
         THROW_IF_FAILED(textLayout->GetOverhangMetrics(&overhang));
 
-        const DWRITE_OVERHANG_METRICS clampedOverhang{
-            std::max(0.0f, overhang.left),
-            std::max(0.0f, overhang.top),
-            std::max(0.0f, overhang.right),
-            std::max(0.0f, overhang.bottom),
-        };
-        f32x2 actualSize{
-            layoutBox.x + overhang.left + overhang.right,
-            layoutBox.y + overhang.top + overhang.bottom,
-        };
+        auto actualSizeX = layoutBox.x + overhang.left + overhang.right;
 
         // Long glyphs should be drawn with their proper design size, even if that makes them a bit blurry,
         // because otherwise we fail to support "pseudo" block characters like the "===" ligature in Cascadia Code.
@@ -585,8 +582,7 @@ AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t*
             const auto advanceScale = _r.fontMetrics.advanceScale;
             scalingRequired = true;
             scale = { advanceScale, advanceScale };
-            actualSize.x *= advanceScale;
-            actualSize.y *= advanceScale;
+            actualSizeX *= advanceScale;
         }
 
         // We need to offset glyphs that are simply outside of our layout box (layoutBox.x/.y)
@@ -627,34 +623,27 @@ AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t*
         //   --> offsetY = 0
         //   --> scale   = layoutBox.y / (layoutBox.y + left + right)
         //               = 0.69f
-        offset.x = clampedOverhang.left - clampedOverhang.right;
-        offset.y = clampedOverhang.top - clampedOverhang.bottom;
+        offset.x = std::max(0.0f, overhang.left) - std::max(0.0f, overhang.right);
+        scaleCenter.x = offset.x;
+        scaleCenter.y = _r.fontMetrics.baselineInDIP;
 
-        if ((actualSize.x - layoutBox.x) > _r.dipPerPixel)
+        if ((actualSizeX - layoutBox.x) > _r.dipPerPixel)
         {
             scalingRequired = true;
             offset.x = (overhang.left - overhang.right) * 0.5f;
-            scale.x = layoutBox.x / actualSize.x;
+            scale.x = layoutBox.x / actualSizeX;
             scale.y = scale.x;
+            scaleCenter.x = layoutBox.x * 0.5f;
         }
-        if ((actualSize.y - layoutBox.y) > _r.dipPerPixel)
+        if (overhang.top > _r.dipPerPixel || overhang.bottom > _r.dipPerPixel)
         {
+            const auto descend = _r.cellSizeDIP.y - _r.fontMetrics.baselineInDIP;
+            const auto scaleTop = _r.fontMetrics.baselineInDIP / (_r.fontMetrics.baselineInDIP + overhang.top);
+            const auto scaleBottom = descend / (descend + overhang.bottom);
             scalingRequired = true;
-            offset.y = (overhang.top - overhang.bottom) * 0.5f;
-            scale.x = std::min(scale.x, layoutBox.y / actualSize.y);
+            scale.x = std::min(scale.x, std::min(scaleTop, scaleBottom));
             scale.y = scale.x;
         }
-
-        // As explained below, we use D2D1_DRAW_TEXT_OPTIONS_NO_SNAP to prevent a weird issue with baseline snapping.
-        // But we do want it technically, so this re-implements baseline snapping... I think?
-        // It calculates the new `baseline` height after transformation by `scale.y` relative to the center point `halfSize.y`.
-        //
-        // This works even if `scale.y == 1`, because then `baseline == baselineInDIP + offset.y` and `baselineInDIP`
-        // is always measured in full pixels. So rounding it will be equivalent to just rounding `offset.y` itself.
-        const auto baseline = halfSize.y + (_r.fontMetrics.baselineInDIP + offset.y - halfSize.y) * scale.y;
-        // This rounds to the nearest multiple of _r.dipPerPixel.
-        const auto baselineFixed = roundf(baseline * _r.pixelPerDIP) * _r.dipPerPixel;
-        offset.y += (baselineFixed - baseline) / scale.y;
     }
 
     auto options = D2D1_DRAW_TEXT_OPTIONS_NONE;
@@ -672,11 +661,19 @@ AtlasEngine::CachedGlyphLayout AtlasEngine::_getCachedGlyphLayout(const wchar_t*
     // where every single "=" might be blatantly misaligned vertically (same for any box drawings).
     WI_SetFlagIf(options, D2D1_DRAW_TEXT_OPTIONS_NO_SNAP, scalingRequired);
 
+    // ClearType basically has a 3x higher horizontal resolution. To make our glyphs render the same everywhere,
+    // it's probably for the best to ensure we initially rasterize them on a whole pixel boundary.
+    // (https://en.wikipedia.org/wiki/ClearType#How_ClearType_works)
+    offset.x = roundf(offset.x * _r.pixelPerDIP) * _r.dipPerPixel;
+    // As explained below, we use D2D1_DRAW_TEXT_OPTIONS_NO_SNAP to prevent a weird issue with baseline snapping.
+    // But we do want it technically, so this re-implements baseline snapping... I think?
+    offset.y = roundf(offset.y * _r.pixelPerDIP) * _r.dipPerPixel;
+
     return CachedGlyphLayout{
         .textLayout = textLayout,
-        .halfSize = halfSize,
         .offset = offset,
         .scale = scale,
+        .scaleCenter = scaleCenter,
         .options = options,
         .scalingRequired = scalingRequired,
     };
@@ -790,8 +787,8 @@ void AtlasEngine::CachedGlyphLayout::applyScaling(ID2D1RenderTarget* d2dRenderTa
             0,
             0,
             scale.y,
-            (origin.x + halfSize.x) * (1.0f - scale.x),
-            (origin.y + halfSize.y) * (1.0f - scale.y),
+            (origin.x + scaleCenter.x) * (1.0f - scale.x),
+            (origin.y + scaleCenter.y) * (1.0f - scale.y),
         };
         d2dRenderTarget->SetTransform(&transform);
     }


Test patch: