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.

* Compile RenderingTests and run it
* Using Consolas, MS Gothic and Cascadia Code while Ctrl+Scrolling
  up and down works as expected without clipping ✅

(cherry picked from commit 99193c9)
Service-Card-Id: 91356394
Service-Version: 1.19
  • Loading branch information
@lhecker @DHowett
lhecker authored and DHowett committed Dec 15, 2023
1 parent a62fb06 commit acb0630
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 159 deletions.
13 changes: 0 additions & 13 deletions .github/actions/spelling/expect/expect.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,6 @@ CPPCORECHECK
cppcorecheckrules
cpprestsdk
cppwinrt
CProc
cpx
CREATESCREENBUFFER
CREATESTRUCT
Expand Down Expand Up @@ -344,7 +343,6 @@ CTRLVOLUME
Ctxt
CUF
cupxy
curlyline
CURRENTFONT
currentmode
CURRENTPAGE
Expand Down Expand Up @@ -607,7 +605,6 @@ EXETYPE
exeuwp
exewin
exitwin
expectedinput
EXPUNGECOMMANDHISTORY
EXSTYLE
EXTENDEDEDITKEY
Expand Down Expand Up @@ -830,7 +827,6 @@ HPA
hpcon
HPCON
hpen
hpj
HPR
HProvider
HREDRAW
Expand Down Expand Up @@ -1022,7 +1018,6 @@ logissue
losslessly
loword
lparam
LPCCH
lpch
LPCPLINFO
LPCREATESTRUCT
Expand Down Expand Up @@ -1221,7 +1216,6 @@ NOMOVE
NONALERT
nonbreaking
nonclient
NONCONST
NONINFRINGEMENT
NONPREROTATED
nonspace
Expand Down Expand Up @@ -1582,7 +1576,6 @@ rgbs
rgci
rgfae
rgfte
rgi
rgn
rgp
rgpwsz
Expand Down Expand Up @@ -1751,7 +1744,6 @@ SOLIDBOX
Solutiondir
somefile
sourced
spammy
SRCCODEPAGE
SRCCOPY
SRCINVERT
Expand Down Expand Up @@ -1914,7 +1906,6 @@ tprivapi
tput
tracelog
tracelogging
traceloggingprovider
traceviewpp
trackbar
TRACKCOMPOSITION
Expand Down Expand Up @@ -2007,7 +1998,6 @@ USEFILLATTRIBUTE
USEGLYPHCHARS
USEHICON
USEPOSITION
userbase
USERDATA
userdpiapi
Userp
Expand Down Expand Up @@ -2050,8 +2040,6 @@ VKKEYSCAN
VMs
VPA
VPR
VProc
VRaw
VREDRAW
vsc
vsconfig
Expand Down Expand Up @@ -2093,7 +2081,6 @@ WCIA
WCIW
WCSHELPER
wcsicmp
wcsnicmp
wcsrev
wddm
wddmcon
Expand Down
10 changes: 7 additions & 3 deletions src/renderer/gdi/gdirenderer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,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
View file
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

1 comment on commit acb0630

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@check-spelling-bot Report

🔴 Please review

See the 📜action log for details.

Unrecognized words (11) 
CProc
hpj
LPCCH
NONCONST
rgi
spammy
traceloggingprovider
userbase
VProc
VRaw
wcsnicmp
To accept ✔️ these unrecognized words as correct, run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the release-1.19 branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/7228232799/attempts/1'
Pattern suggestions ✂️ (1)

You could add these patterns to .github/actions/spelling/patterns/acb06304c2002af43c321dd88fda3ae80c244c66.txt:

# Automatically suggested patterns
# hit-count: 1 file-count: 1
# tput arguments -- https://man7.org/linux/man-pages/man5/terminfo.5.html -- technically they can be more than 5 chars long...
\btput\s+(?:(?:-[SV]|-T\s*\w+)\s+)*\w{3,5}\b
Warnings (1)

See the 📜action log for details.

ℹ️ Warnings Count
ℹ️ candidate-pattern 1

See ℹ️ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

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

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.