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.

Sign up for GitHub

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

Jump to bottom

Fix curlyline rendering in AtlasEngine and GDIRenderer #16444

Merged
merged 7 commits into from
Dec 14, 2023
Merged

Fix curlyline rendering in AtlasEngine and GDIRenderer #16444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a501fee
fix curlyline rendering in conhost
tusharsnx Dec 8, 2023
ef8a907
remove minimum threshold for curlyline peak in atlas engine
tusharsnx Dec 8, 2023
67c87c9
properly cast to the right type
tusharsnx Dec 8, 2023
e2ca95f
address feedbacks
tusharsnx Dec 9, 2023
9fa65f1
add explicit types to DrawStrokedLine() args
tusharsnx Dec 10, 2023
7a077ae
fix underline-offset and half-stroke-width calc
tusharsnx Dec 10, 2023
0b1ecf3
force curlyline at all font sizes
tusharsnx Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 27 additions & 29 deletions src/renderer/atlas/BackendD3D.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

(FYI: This PR is tiny if you suppress whitespace changes.)

// 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;
Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/atlas/BackendD3D.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 15 additions & 14 deletions src/renderer/gdi/paint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
lhecker marked this conversation as resolved.
Show resolved Hide resolved

// 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));
lhecker marked this conversation as resolved.
Show resolved Hide resolved
return S_OK;
Expand Down Expand Up @@ -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;
Expand Down
51 changes: 22 additions & 29 deletions src/renderer/gdi/state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Comment on lines +421 to 424
Copy link
Member

Choose a reason for hiding this comment

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

I found that this behavior can look somewhat unpleasant when Ctrl+scrolling up and down when using Consolas, because it'll jump in between very squiggly lines and barely squiggly lines all the time. Additionally, and unrelated to your PR, I'm having trouble ever getting the double-underline to separate into two distinct lines.

So, I'm currently doing a couple modifications to this function to bring it closer to the corresponding AtlasEngine code: Using floats throughout the function, unless rounding is needed (= for instance, right before calculating the thin line width) and I'm porting Word's double underline algorithm over. My hope is that all this will allow me to better understand how your current code works and to come up with an idea for making the transition between curly and less-curly lines "smoother". (I'll submit the above changes in a different PR after this one merges.)

Personally speaking I don't agree with this:

I know we can't move it upwards because that would mean it is discontinuous with the normal underline styles, so this seems totally sufficient to me!

I think discontinuous lines are better, if it avoids clipping. But I also get that discontinuous lines are kind of weird too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm currently doing a couple modifications to this function to bring it closer to the corresponding AtlasEngine code.....(I'll submit the above changes in a different PR after this one merges.)

That sounds good to me.

I have a few ideas that I'll just share here:

  1. We could give users an option to let WT/Conhost adjust underline-offfset. We'll make sure curlyline is always drawn perfectly. This is what Wezterm seems to do (can't be turned off).
  2. (Not sure if it works but) Have a separate render pass for drawing underlines after glyphs/background have been drawn. This ensures we can draw underlines at the right offset while avoiding any clipping. (Too much work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having trouble ever getting the double underline to separate into two distinct lines.

j4james briefly mentioned it in his comment in the original PR that brought doubly underlined rendering support:

Note how some fonts only have enough space for a thicker line, and not two distinct lines, but I think that's OK (XTerm always renders the attribute this way). Also note that this is more common in the GDI renderer because it uses a smaller line height.

Copy link
Member

Choose a reason for hiding this comment

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

I've just had a chat with Dustin, and we compared different approaches. Word's double-underline algorithm does look fairly well at small and big font sizes and works better with Consolas. Using the thinLineWidth (as in AtlasEngine) for the curly underline also appears to be slightly better visually than the full underlineWidth.

I'll approve this PR because it puts this code in a fantastic state to do further "fine tuning" on it. Thank you so much for working on this! I also didn't want to ask you to make any more changes, since I don't want to put any more unnecessary burden on you. I'll link the follow-up PR here and post screenshots there to compare the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Done: #16475

_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.
Expand Down
Loading