Repository: microsoft/terminal (language: cpp)

Issue:
Curly underline is rendered as single
### Windows Terminal version

1.20.3131.0

### Windows build number

10.0.19045.3448

### Other Software

_No response_

### Steps to reproduce

_Originally posted in https://github.com/microsoft/terminal/pull/16097#discussion_r1389476524_

1. Set display scaling to 100%
2. Run OpenConsole or WT
3. Set font to Consolas
4. Set font size to:
   - 34 or smaller for OpenConsole, e.g. 20
   - 19 or smaller for WT, e.g. 12

5. Run something to showcase new underlines, e.g. this script
```
@echo off

echo 4:1 [4:1;58:2::255:000:000mtest[m
echo 4:2 [4:2;58:2::255:255:000mtest[m
echo 4:3 [4:3;58:2::000:255:000mtest[m
echo 4:4 [4:4;58:2::000:255:255mtest[m
echo 4:5 [4:5;58:2::255:000:255mtest[m

pause
```
\- where � is \x1b, or whatever else you might have.



### Expected Behavior

Clearly visible underline styles, curly in particular, e.g.:

![image](https://github.com/microsoft/terminal/assets/11453922/1ef33c14-04d0-4156-8f86-53750e0f3578)

### Actual Behavior

Curly is rendered as single in both OpenConsole and WT:

![image](https://github.com/microsoft/terminal/assets/11453922/4a03c331-1690-4ade-b352-8a7e9353ec43)

*OpenConsole also renders double underline as single, which may or may not be related.*

------

Other terminals, e.g. wezterm, seemingly have no issues with even smaller sizes:

![image](https://github.com/microsoft/terminal/assets/11453922/419b48b2-42a0-4a78-b4d2-03aa670b0d76)

Technically it's not *too small* if the wave height is at least 2 pixels:
![image](https://github.com/microsoft/terminal/assets/11453922/d6782c1a-2c47-44b7-93e3-02de8fa6fdf8)

Additionally, OpenConsole with even smaller sizes, e.g. Consolas 15, doesn't render anything but "double" at all:

![image](https://github.com/microsoft/terminal/assets/11453922/62f55908-737b-4281-9ef2-5b0a737fb9c5)

Technical limitations are understandable, but, assuming that the key selling point of this feature is spellchecking, any ugly/incorrect/clipping/overlapping rendering would be better than nothing at all.



Discussion / hints:
There are two issues in this Issue

### 1. Curly line drawn as Singly line

Admittedly, I've a high dpi screen which I use at 175% scaling. I guess the threshold we kept for curly underlines can be fine tuned for 100% display scaling. Something that works for 100% should work for 175% as well 🙂 

Can someone try a different value for `minCurlyLinePeakHeight`, and see which one works better at 100% display scaling? 

AtlasEngine (WT)

https://github.com/microsoft/terminal/blob/d14524cd4cc4970bb1b6456f9667e2dd661b9854/src/renderer/atlas/BackendD3D.cpp#L311-L313

GDI (Conhost)

https://github.com/microsoft/terminal/blob/d14524cd4cc4970bb1b6456f9667e2dd661b9854/src/renderer/gdi/state.cpp#L410-L432

In GDI, we have two conditions to check before drawing curly underline:
1. Underline width is atleast 1px.
2. Space between cell bottom and underline position is atleast `MinCurlyLinePeakHeight` (2px).

### 2. Underlines not visible at smaller font-sizes in Conhost 

>Additionally, OpenConsole with even smaller sizes, e.g. Consolas 15, doesn't render anything but "double" at all:

I guess the underline offset we're calculating is making underlines to be drawn below the cell bottom, effectively making them not visible at all. A fix could be to clamp the offset within cell boundary🤔 (For me, this happens at font-size 9px with Consolas.)


The line offsets should have already been set to their correct positions based on the provided font metrics in the `GdiEngine::UpdateFont` method. The problem is that we've now added an additional offset at render time here:

https://github.com/microsoft/terminal/blob/d14524cd4cc4970bb1b6456f9667e2dd661b9854/src/renderer/gdi/paint.cpp#L622-L626

That's almost certainly wrong. Even if if wasn't pushing the offsets out of range, it doesn't make sense to be altering the value here. What was the reason for this change?

Edit: I see now it's probably because the single and double underline are now rendered with `LineTo` calls rather than `PatBlt`. But there must be something wrong with that calculation, because the original line offset should definitely have been in range. Also, if we need that additional half-width offset, that calculation should really be done in advance in the `UpdateFont` method. There's surely no need to keep recalculating it every time an underline is rendered?
Also, assuming you're looking at this code again, note that the `underlineWidth` should never be less than 1. We've got a check for that here:

https://github.com/microsoft/terminal/blob/d14524cd4cc4970bb1b6456f9667e2dd661b9854/src/renderer/gdi/state.cpp#L379-L383

So I think the test below should never be necessary.

https://github.com/microsoft/terminal/blob/d14524cd4cc4970bb1b6456f9667e2dd661b9854/src/renderer/gdi/state.cpp#L409-L413
I'm gonna pull this one out of the triage queue and assign it to you, Tushar! Let me know if you have any objections. Thanks for working on underlines!

Fix patch:
diff --git a/src/renderer/atlas/BackendD3D.cpp b/src/renderer/atlas/BackendD3D.cpp
index 51494df7b23..e68fb2a7a57 100644
--- a/src/renderer/atlas/BackendD3D.cpp
+++ b/src/renderer/atlas/BackendD3D.cpp
@@ -306,37 +306,35 @@ void BackendD3D::_updateFontDependents(const RenderingPayload& p)
 {
     const auto& font = *p.s->font;
 
-    // The max height of Curly line peak in `em` units.
-    const auto maxCurlyLinePeakHeightEm = 0.075f;
-    // We aim for atleast 1px height, but since we draw 1px smaller curly line,
-    // we aim for 2px height as a result.
-    const auto minCurlyLinePeakHeight = 2.0f;
-
-    // Curlyline uses the gap between cell bottom and singly underline position
-    // as the height of the wave's peak. The baseline for curly-line is at the
-    // middle of singly underline. The gap could be too big, so we also apply
-    // a limit on the peak height.
-    const auto strokeHalfWidth = font.underline.height / 2.0f;
-    const auto underlineMidY = font.underline.position + strokeHalfWidth;
-    const auto cellBottomGap = font.cellSize.y - underlineMidY - strokeHalfWidth;
-    const auto maxCurlyLinePeakHeight = maxCurlyLinePeakHeightEm * font.fontSize;
-    auto curlyLinePeakHeight = std::min(cellBottomGap, maxCurlyLinePeakHeight);
+    // Curlyline is drawn with a desired height relative to the font size. The
+    // baseline of curlyline is at the middle of singly underline. When there's
+    // limited space to draw a curlyline, we apply a limit on the peak height.
+    {
+        // initialize curlyline peak height to a desired value. Clamp it to at
+        // least 1.
+        constexpr auto curlyLinePeakHeightEm = 0.075f;
+        _curlyLinePeakHeight = std::max(1.0f, std::roundf(curlyLinePeakHeightEm * font.fontSize));
+
+        // calc the limit we need to apply
+        const auto strokeHalfWidth = std::floor(font.underline.height / 2.0f);
+        const auto underlineMidY = font.underline.position + strokeHalfWidth;
+        const auto maxDrawableCurlyLinePeakHeight = font.cellSize.y - underlineMidY - font.underline.height;
+
+        // if the limit is <= 0 (no height at all), stick with the desired height.
+        // This is how we force a curlyline even when there's no space, though it
+        // might be clipped at the bottom.
+        if (maxDrawableCurlyLinePeakHeight > 0.0f)
+        {
+            _curlyLinePeakHeight = std::min(_curlyLinePeakHeight, maxDrawableCurlyLinePeakHeight);
+        }
 
-    // When it's too small to be curly, make it straight.
-    if (curlyLinePeakHeight < minCurlyLinePeakHeight)
-    {
-        curlyLinePeakHeight = 0;
+        const auto curlyUnderlinePos = underlineMidY - _curlyLinePeakHeight - font.underline.height;
+        const auto curlyUnderlineWidth = 2.0f * (_curlyLinePeakHeight + font.underline.height);
+        const auto curlyUnderlinePosU16 = gsl::narrow_cast<u16>(lrintf(curlyUnderlinePos));
+        const auto curlyUnderlineWidthU16 = gsl::narrow_cast<u16>(lrintf(curlyUnderlineWidth));
+        _curlyUnderline = { curlyUnderlinePosU16, curlyUnderlineWidthU16 };
     }
 
-    // We draw a smaller curly line (-1px) to avoid clipping due to the rounding.
-    _curlyLineDrawPeakHeight = std::max(0.0f, curlyLinePeakHeight - 1.0f);
-
-    const auto curlyUnderlinePos = font.underline.position - curlyLinePeakHeight;
-    const auto curlyUnderlineWidth = 2.0f * (curlyLinePeakHeight + strokeHalfWidth);
-    const auto curlyUnderlinePosU16 = gsl::narrow_cast<u16>(lrintf(curlyUnderlinePos));
-    const auto curlyUnderlineWidthU16 = gsl::narrow_cast<u16>(lrintf(curlyUnderlineWidth));
-    _curlyUnderline = { curlyUnderlinePosU16, curlyUnderlineWidthU16 };
-
     DWrite_GetRenderParams(p.dwriteFactory.get(), &_gamma, &_cleartypeEnhancedContrast, &_grayscaleEnhancedContrast, _textRenderingParams.put());
     // Clearing the atlas requires BeginDraw(), which is expensive. Defer this until we need Direct2D anyways.
     _fontChangedResetGlyphAtlas = true;
@@ -576,7 +574,7 @@ void BackendD3D::_recreateConstBuffer(const RenderingPayload& p) const
         data.enhancedContrast = p.s->font->antialiasingMode == AntialiasingMode::ClearType ? _cleartypeEnhancedContrast : _grayscaleEnhancedContrast;
         data.underlineWidth = p.s->font->underline.height;
         data.curlyLineWaveFreq = 2.0f * 3.14f / p.s->font->cellSize.x;
-        data.curlyLinePeakHeight = _curlyLineDrawPeakHeight;
+        data.curlyLinePeakHeight = _curlyLinePeakHeight;
         data.curlyLineCellOffset = p.s->font->underline.position + p.s->font->underline.height / 2.0f;
         p.deviceContext->UpdateSubresource(_psConstantBuffer.get(), 0, nullptr, &data, 0, 0);
     }
diff --git a/src/renderer/atlas/BackendD3D.h b/src/renderer/atlas/BackendD3D.h
index 0befe8fe342..4c248ed3ff3 100644
--- a/src/renderer/atlas/BackendD3D.h
+++ b/src/renderer/atlas/BackendD3D.h
@@ -291,7 +291,7 @@ namespace Microsoft::Console::Render::Atlas
         // The bounding rect of _cursorRects in pixels.
         til::rect _cursorPosition;
 
-        f32 _curlyLineDrawPeakHeight = 0;
+        f32 _curlyLinePeakHeight = 0.0f;
         FontDecorationPosition _curlyUnderline;
 
         bool _requiresContinuousRedraw = false;
diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 9722703d105..222576390e2 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -536,27 +536,30 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
     const auto DrawLine = [=](const auto x, const auto y, const auto w, const auto h) {
         return PatBlt(_hdcMemoryContext, x, y, w, h, PATCOPY);
     };
-    const auto DrawStrokedLine = [&](const auto x, const auto y, const auto w) {
+    const auto DrawStrokedLine = [&](const til::CoordType x, const til::CoordType y, const unsigned w) {
         RETURN_HR_IF(E_FAIL, !MoveToEx(_hdcMemoryContext, x, y, nullptr));
-        RETURN_HR_IF(E_FAIL, !LineTo(_hdcMemoryContext, x + w, y));
+        RETURN_HR_IF(E_FAIL, !LineTo(_hdcMemoryContext, gsl::narrow_cast<int>(x + w), y));
         return S_OK;
     };
-    const auto DrawCurlyLine = [&](const auto x, const auto y, const auto cCurlyLines) {
+    const auto DrawCurlyLine = [&](const til::CoordType x, const til::CoordType y, const size_t cCurlyLines) {
         const auto curlyLineWidth = fontWidth;
-        const auto curlyLineHalfWidth = lrintf(curlyLineWidth / 2.0f);
-        const auto controlPointHeight = gsl::narrow_cast<long>(std::floor(3.5f * _lineMetrics.curlylinePeakHeight));
+        const auto curlyLineHalfWidth = std::lround(curlyLineWidth / 2.0f);
+        const auto controlPointHeight = std::lround(3.5f * _lineMetrics.curlylinePeakHeight);
+
         // Each curlyLine requires 3 `POINT`s
         const auto cPoints = gsl::narrow<DWORD>(3 * cCurlyLines);
         std::vector<POINT> points;
         points.reserve(cPoints);
+
         auto start = x;
-        for (auto i = 0u; i < cCurlyLines; i++)
+        for (size_t i = 0; i < cCurlyLines; i++)
         {
             points.emplace_back(start + curlyLineHalfWidth, y - controlPointHeight);
             points.emplace_back(start + curlyLineHalfWidth, y + controlPointHeight);
             points.emplace_back(start + curlyLineWidth, y);
             start += curlyLineWidth;
         }
+
         RETURN_HR_IF(E_FAIL, !MoveToEx(_hdcMemoryContext, x, y, nullptr));
         RETURN_HR_IF(E_FAIL, !PolyBezierTo(_hdcMemoryContext, points.data(), cPoints));
         return S_OK;
@@ -619,28 +622,26 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
     const auto prevPen = wil::SelectObject(_hdcMemoryContext, hpen.get());
     RETURN_HR_IF_NULL(E_FAIL, prevPen.get());
 
-    const auto underlineMidY = std::lround(ptTarget.y + _lineMetrics.underlineOffset + _lineMetrics.underlineWidth / 2.0f);
     if (lines.test(GridLines::Underline))
     {
-        return DrawStrokedLine(ptTarget.x, underlineMidY, widthOfAllCells);
+        return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
     }
     else if (lines.test(GridLines::DoubleUnderline))
     {
-        const auto doubleUnderlineBottomLineMidY = std::lround(ptTarget.y + _lineMetrics.underlineOffset2 + _lineMetrics.underlineWidth / 2.0f);
-        RETURN_IF_FAILED(DrawStrokedLine(ptTarget.x, underlineMidY, widthOfAllCells));
-        return DrawStrokedLine(ptTarget.x, doubleUnderlineBottomLineMidY, widthOfAllCells);
+        RETURN_IF_FAILED(DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells));
+        return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset2, widthOfAllCells);
     }
     else if (lines.test(GridLines::CurlyUnderline))
     {
-        return DrawCurlyLine(ptTarget.x, underlineMidY, cchLine);
+        return DrawCurlyLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, cchLine);
     }
     else if (lines.test(GridLines::DottedUnderline))
     {
-        return DrawStrokedLine(ptTarget.x, underlineMidY, widthOfAllCells);
+        return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
     }
     else if (lines.test(GridLines::DashedUnderline))
     {
-        return DrawStrokedLine(ptTarget.x, underlineMidY, widthOfAllCells);
+        return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
     }
 
     return S_OK;
diff --git a/src/renderer/gdi/state.cpp b/src/renderer/gdi/state.cpp
index 099c2f43f78..13fd0ea59fc 100644
--- a/src/renderer/gdi/state.cpp
+++ b/src/renderer/gdi/state.cpp
@@ -11,15 +11,6 @@
 
 using namespace Microsoft::Console::Render;
 
-namespace
-{
-    // The max height of Curly line peak in `em` units.
-    constexpr auto MaxCurlyLinePeakHeightEm = 0.075f;
-
-    // The min height of Curly line peak.
-    constexpr auto MinCurlyLinePeakHeight = 2.0f;
-}
-
 // Routine Description:
 // - Creates a new GDI-based rendering engine
 // - NOTE: Will throw if initialization failure. Caller must catch.
@@ -406,29 +397,31 @@ GdiEngine::~GdiEngine()
         _lineMetrics.underlineOffset2 = _lineMetrics.underlineOffset - _lineMetrics.gridlineWidth;
     }
 
-    // Curly line doesn't render properly below 1px stroke width. Make it a straight line.
-    if (_lineMetrics.underlineWidth < 1)
-    {
-        _lineMetrics.curlylinePeakHeight = 0;
-    }
-    else
+    // Since we use GDI pen for drawing, the underline offset should point to
+    // the center of the underline.
+    const auto underlineHalfWidth = gsl::narrow_cast<int>(std::floor(_lineMetrics.underlineWidth / 2.0f));
+    _lineMetrics.underlineOffset += underlineHalfWidth;
+    _lineMetrics.underlineOffset2 += underlineHalfWidth;
+
+    // Curlyline is drawn with a desired height relative to the font size. The
+    // baseline of curlyline is at the middle of singly underline. When there's
+    // limited space to draw a curlyline, we apply a limit on the peak height.
     {
-        // Curlyline uses the gap between cell bottom and singly underline
-        // position as the height of the wave's peak. The baseline for curly
-        // line is at the middle of singly underline. The gap could be too big,
-        // so we also apply a limit on the peak height.
-        const auto strokeHalfWidth = _lineMetrics.underlineWidth / 2.0f;
-        const auto underlineMidY = _lineMetrics.underlineOffset + strokeHalfWidth;
-        const auto cellBottomGap = Font.GetSize().height - underlineMidY - strokeHalfWidth;
-        const auto maxCurlyLinePeakHeight = MaxCurlyLinePeakHeightEm * fontSize;
-        auto curlyLinePeakHeight = std::min(cellBottomGap, maxCurlyLinePeakHeight);
-
-        // When it's too small to be curly, make it a straight line.
-        if (curlyLinePeakHeight < MinCurlyLinePeakHeight)
+        // initialize curlyline peak height to a desired value. Clamp it to at
+        // least 1.
+        constexpr auto curlyLinePeakHeightEm = 0.075f;
+        _lineMetrics.curlylinePeakHeight = gsl::narrow_cast<int>(std::max(1L, std::lround(curlyLinePeakHeightEm * fontSize)));
+
+        // calc the limit we need to apply
+        const auto maxDrawableCurlyLinePeakHeight = Font.GetSize().height - _lineMetrics.underlineOffset - _lineMetrics.underlineWidth;
+
+        // if the limit is <= 0 (no height at all), stick with the desired height.
+        // This is how we force a curlyline even when there's no space, though it
+        // might be clipped at the bottom.
+        if (maxDrawableCurlyLinePeakHeight > 0.0f)
         {
-            curlyLinePeakHeight = 0.0f;
+            _lineMetrics.curlylinePeakHeight = std::min(_lineMetrics.curlylinePeakHeight, maxDrawableCurlyLinePeakHeight);
         }
-        _lineMetrics.curlylinePeakHeight = gsl::narrow_cast<int>(std::floor(curlyLinePeakHeight));
     }
 
     // Now find the size of a 0 in this current font and save it for conversions done later.


Test patch: