Skip to content

Commit

Permalink
Put the final touches on GDI's underlines (#16475)
Browse files Browse the repository at this point in the history
While #16444 left wavy lines in an amazing state already, there were
a few more things that could be done to make GDI look more consistent
with other well known Windows applications.

But before that, a couple unrelated, but helpful changes were made:
* `GdiEngine::UpdateFont` was heavily modified to do all calculations
  in floats. All modern CPUs have fast FPUs and even the fairly slow
  `lroundf` function is so fast (relatively) nowadays that in a cold
  path like this, we can liberally call it to convert back to `int`s.
  This makes intermediate calculation more accurate and consistent.
* `GdiEngine::PaintBufferGridLines` was exception-unsafe due to its
  use of a `std::vector` with catch clause and this PR fixes that.
  Additionally, the vector was swapped out with a `til::small_vector`
  to reduce heap allocations. (Arena allocators!)
* RenderingTests was updated to cover styled underlines

With that in place, these improvements were done:
* Word's double-underline algorithm was ported over from `AtlasEngine`.
  It uses a half underline-width (aka `thinLineWidth`) which will now
  also be used for wavy lines to make them look a bit more filigrane.
* The Bézier curve for wavy/curly underlines was modified to use
  control points at (0.5,0.5) and (0.5,-0.5) respectively. This results
  in a maxima at y=0.1414 which is much closer to a sine curve with a
  maxima at 1/(2pi) = 0.1592. Previously, the maxima was a lot higher
  (roughly 4x) depending on the aspect ratio of the glyphs.
* Wavy underlines don't depend on the aspect ratio of glyphs anymore.
  This previously led to several problems depending on the exact font.
  The old renderer would draw exactly 3 periods of the wave into
  each cell which would also ensure continuity between cells.
  Unfortunately, this meant that waves could look inconsistent.
  The new approach always uses the aforementioned sine-like waves.
* The wavy underline offset was clamped so that it's never outside of
  bounds of a line. This avoids clipping.

## Validation Steps Performed
* Compile RenderingTests and run it
* Using Consolas, MS Gothic and Cascadia Code while Ctrl+Scrolling
  up and down works as expected without clipping ✅
  • Loading branch information
lhecker authored Dec 15, 2023
1 parent 28acc10 commit 99193c9
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 161 deletions.
15 changes: 0 additions & 15 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,6 @@ CPPCORECHECK
cppcorecheckrules
cpprestsdk
cppwinrt
CProc
cpx
CREATESCREENBUFFER
CREATESTRUCT
Expand Down Expand Up @@ -324,7 +323,6 @@ CTRLVOLUME
Ctxt
CUF
cupxy
curlyline
CURRENTFONT
currentmode
CURRENTPAGE
Expand Down Expand Up @@ -583,7 +581,6 @@ EXETYPE
exeuwp
exewin
exitwin
expectedinput
EXPUNGECOMMANDHISTORY
EXSTYLE
EXTENDEDEDITKEY
Expand Down Expand Up @@ -795,7 +792,6 @@ HPA
hpcon
HPCON
hpen
hpj
HPR
HProvider
HREDRAW
Expand Down Expand Up @@ -978,7 +974,6 @@ logissue
losslessly
loword
lparam
LPCCH
lpch
LPCPLINFO
LPCREATESTRUCT
Expand Down Expand Up @@ -1177,7 +1172,6 @@ NOMOVE
NONALERT
nonbreaking
nonclient
NONCONST
NONINFRINGEMENT
NONPREROTATED
nonspace
Expand Down Expand Up @@ -1525,17 +1519,14 @@ rgbs
rgci
rgfae
rgfte
rgi
rgn
rgp
rgpwsz
rgrc
rgui
rgw
RIGHTALIGN
RIGHTBUTTON
riid
Rike
RIS
roadmap
robomac
Expand Down Expand Up @@ -1685,7 +1676,6 @@ SOLIDBOX
Solutiondir
somefile
sourced
spammy
SRCCODEPAGE
SRCCOPY
SRCINVERT
Expand Down Expand Up @@ -1831,7 +1821,6 @@ Tpp
Tpqrst
tracelog
tracelogging
traceloggingprovider
traceviewpp
trackbar
TRACKCOMPOSITION
Expand Down Expand Up @@ -1915,7 +1904,6 @@ USEFILLATTRIBUTE
USEGLYPHCHARS
USEHICON
USEPOSITION
userbase
USERDATA
userdpiapi
Userp
Expand Down Expand Up @@ -1956,8 +1944,6 @@ vpack
vpackdirectory
VPACKMANIFESTDIRECTORY
VPR
VProc
VRaw
VREDRAW
vsc
vsconfig
Expand Down Expand Up @@ -2001,7 +1987,6 @@ WCIA
WCIW
WCSHELPER
wcsicmp
wcsnicmp
wcsrev
wddm
wddmcon
Expand Down
10 changes: 7 additions & 3 deletions src/renderer/gdi/gdirenderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,16 @@ namespace Microsoft::Console::Render
struct LineMetrics
{
int gridlineWidth;
int underlineOffset;
int underlineOffset2;
int thinLineWidth;
int underlineCenter;
int underlineWidth;
int doubleUnderlinePosTop;
int doubleUnderlinePosBottom;
int strikethroughOffset;
int strikethroughWidth;
int curlylinePeakHeight;
int curlyLineCenter;
int curlyLinePeriod;
int curlyLineControlPointOffset;
};

LineMetrics _lineMetrics;
Expand Down
85 changes: 53 additions & 32 deletions src/renderer/gdi/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
// Licensed under the MIT license.

#include "precomp.h"
#include <vector>
#include "gdirenderer.hpp"

#include <til/small_vector.h>

#include "../inc/unicode.hpp"

#pragma hdrstop
Expand Down Expand Up @@ -516,6 +517,7 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
// Return Value:
// - S_OK or suitable GDI HRESULT error or E_FAIL for GDI errors in functions that don't reliably return a specific error code.
[[nodiscard]] HRESULT GdiEngine::PaintBufferGridLines(const GridLineSet lines, const COLORREF gridlineColor, const COLORREF underlineColor, const size_t cchLine, const til::point coordTarget) noexcept
try
{
LOG_IF_FAILED(_FlushBufferLines());

Expand All @@ -531,38 +533,50 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
// Get the font size so we know the size of the rectangle lines we'll be inscribing.
const auto fontWidth = _GetFontSize().width;
const auto fontHeight = _GetFontSize().height;
const auto widthOfAllCells = fontWidth * gsl::narrow_cast<unsigned>(cchLine);
const auto widthOfAllCells = fontWidth * gsl::narrow_cast<til::CoordType>(cchLine);

const auto DrawLine = [=](const auto x, const auto y, const auto w, const auto h) {
const auto DrawLine = [=](const til::CoordType x, const til::CoordType y, const til::CoordType w, const til::CoordType h) {
return PatBlt(_hdcMemoryContext, x, y, w, h, PATCOPY);
};
const auto DrawStrokedLine = [&](const til::CoordType x, const til::CoordType y, const unsigned w) {
const auto DrawStrokedLine = [&](const til::CoordType x, const til::CoordType y, const til::CoordType w) {
RETURN_HR_IF(E_FAIL, !MoveToEx(_hdcMemoryContext, x, y, nullptr));
RETURN_HR_IF(E_FAIL, !LineTo(_hdcMemoryContext, gsl::narrow_cast<int>(x + w), y));
RETURN_HR_IF(E_FAIL, !LineTo(_hdcMemoryContext, x + w, y));
return S_OK;
};
const auto DrawCurlyLine = [&](const til::CoordType x, const til::CoordType y, const size_t cCurlyLines) {
const auto curlyLineWidth = fontWidth;
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 (size_t i = 0; i < cCurlyLines; i++)
const auto DrawCurlyLine = [&](const til::CoordType begX, const til::CoordType y, const til::CoordType width) {
const auto period = _lineMetrics.curlyLinePeriod;
const auto halfPeriod = period / 2;
const auto controlPointOffset = _lineMetrics.curlyLineControlPointOffset;

// To ensure proper continuity of the wavy line between cells of different line color
// this code starts/ends the line earlier/later than it should and then clips it.
// Clipping in GDI is expensive, but it was the easiest approach.
// I've noticed that subtracting -1px prevents missing pixels when GDI draws. They still
// occur at certain (small) font sizes, but I couldn't figure out how to prevent those.
const auto lineStart = ((begX - 1) / period) * period;
const auto lineEnd = begX + width;

IntersectClipRect(_hdcMemoryContext, begX, ptTarget.y, begX + width, ptTarget.y + fontHeight);
const auto restoreRegion = wil::scope_exit([&]() {
// Luckily no one else uses clip regions. They're weird to use.
SelectClipRgn(_hdcMemoryContext, nullptr);
});

// You can assume that each cell has roughly 5 POINTs on average. 128 POINTs is 1KiB.
til::small_vector<POINT, 128> points;

// This is the start point of the Bézier curve.
points.emplace_back(lineStart, y);

for (auto x = lineStart; x < lineEnd; x += period)
{
points.emplace_back(start + curlyLineHalfWidth, y - controlPointHeight);
points.emplace_back(start + curlyLineHalfWidth, y + controlPointHeight);
points.emplace_back(start + curlyLineWidth, y);
start += curlyLineWidth;
points.emplace_back(x + halfPeriod, y - controlPointOffset);
points.emplace_back(x + halfPeriod, y + controlPointOffset);
points.emplace_back(x + period, y);
}

RETURN_HR_IF(E_FAIL, !MoveToEx(_hdcMemoryContext, x, y, nullptr));
RETURN_HR_IF(E_FAIL, !PolyBezierTo(_hdcMemoryContext, points.data(), cPoints));
return S_OK;
const auto cpt = gsl::narrow_cast<DWORD>(points.size());
return PolyBezier(_hdcMemoryContext, points.data(), cpt);
};

if (lines.test(GridLines::Left))
Expand Down Expand Up @@ -605,7 +619,6 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
RETURN_HR_IF(E_FAIL, !DrawLine(ptTarget.x, y, widthOfAllCells, _lineMetrics.strikethroughWidth));
}

// Create a pen matching the underline style.
DWORD underlinePenType = PS_SOLID;
if (lines.test(GridLines::DottedUnderline))
{
Expand All @@ -615,37 +628,45 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
{
underlinePenType = PS_DASH;
}

DWORD underlineWidth = _lineMetrics.underlineWidth;
if (lines.any(GridLines::DoubleUnderline, GridLines::CurlyUnderline))
{
underlineWidth = _lineMetrics.thinLineWidth;
}

const LOGBRUSH brushProp{ .lbStyle = BS_SOLID, .lbColor = underlineColor };
wil::unique_hpen hpen(ExtCreatePen(underlinePenType | PS_GEOMETRIC | PS_ENDCAP_FLAT, _lineMetrics.underlineWidth, &brushProp, 0, nullptr));
wil::unique_hpen hpen(ExtCreatePen(underlinePenType | PS_GEOMETRIC | PS_ENDCAP_FLAT, underlineWidth, &brushProp, 0, nullptr));

// Apply the pen.
const auto prevPen = wil::SelectObject(_hdcMemoryContext, hpen.get());
RETURN_HR_IF_NULL(E_FAIL, prevPen.get());

if (lines.test(GridLines::Underline))
{
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineCenter, widthOfAllCells);
}
else if (lines.test(GridLines::DoubleUnderline))
{
RETURN_IF_FAILED(DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells));
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset2, widthOfAllCells);
RETURN_IF_FAILED(DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.doubleUnderlinePosTop, widthOfAllCells));
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.doubleUnderlinePosBottom, widthOfAllCells);
}
else if (lines.test(GridLines::CurlyUnderline))
{
return DrawCurlyLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, cchLine);
return DrawCurlyLine(ptTarget.x, ptTarget.y + _lineMetrics.curlyLineCenter, widthOfAllCells);
}
else if (lines.test(GridLines::DottedUnderline))
{
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineCenter, widthOfAllCells);
}
else if (lines.test(GridLines::DashedUnderline))
{
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineOffset, widthOfAllCells);
return DrawStrokedLine(ptTarget.x, ptTarget.y + _lineMetrics.underlineCenter, widthOfAllCells);
}

return S_OK;
}
CATCH_RETURN();

// Routine Description:
// - Draws the cursor on the screen
Expand Down
Loading

0 comments on commit 99193c9

Please sign in to comment.