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 3 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
40 changes: 17 additions & 23 deletions src/renderer/atlas/BackendD3D.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,36 +306,30 @@ 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);

// When it's too small to be curly, make it straight.
if (curlyLinePeakHeight < minCurlyLinePeakHeight)
{
curlyLinePeakHeight = 0;
}
const auto strokeHalfWidth = font.underline.height / 2.0f;
const auto underlineMidY = font.underline.position + strokeHalfWidth;
const auto cellBottomGap = font.cellSize.y - underlineMidY - strokeHalfWidth;

// We draw a smaller curly line (-1px) to avoid clipping due to the rounding.
_curlyLineDrawPeakHeight = std::max(0.0f, curlyLinePeakHeight - 1.0f);
// The max height of Curly line peak in `em` units.
constexpr auto maxCurlyLinePeakHeightEm = 0.075f;
const auto maxCurlyLinePeakHeight = maxCurlyLinePeakHeightEm * font.fontSize;

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 };
auto curlyLinePeakHeight = std::min(cellBottomGap, maxCurlyLinePeakHeight);

// 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.
Expand Down
24 changes: 13 additions & 11 deletions src/renderer/gdi/paint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,20 +543,24 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
};
const auto DrawCurlyLine = [&](const auto x, const auto y, const auto cCurlyLines) {
tusharsnx marked this conversation as resolved.
Show resolved Hide resolved
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);

const auto end = x + gsl::narrow_cast<til::CoordType>(cCurlyLines * curlyLineWidth);
auto start = x;
for (auto i = 0u; i < cCurlyLines; i++)
while (start < end)
tusharsnx marked this conversation as resolved.
Show resolved Hide resolved
{
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 +623,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
54 changes: 23 additions & 31 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,30 @@ 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
const int strokeHalfWidth = std::lround(_lineMetrics.underlineWidth / 2.0);
tusharsnx marked this conversation as resolved.
Show resolved Hide resolved

// Since we use GDI pen for drawing, the underline offset should point to
// the center of the underline.
_lineMetrics.underlineOffset += strokeHalfWidth;
_lineMetrics.underlineOffset2 += strokeHalfWidth;

// We want the underline to always be visible and remain within the cell
// bottom, so we clamp the offset to fit just inside.
_lineMetrics.underlineOffset = std::min(_lineMetrics.underlineOffset, maxUnderlineOffset);
_lineMetrics.underlineOffset2 = std::min(_lineMetrics.underlineOffset2, maxUnderlineOffset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still seems wrong to me. The offsets have already been clamped prior to this, so no additional clamping should have been necessary. The strokeHalfWidth adjustment should just be accounting for the difference between a PatBlt rendering and a LineTo rendering. If it was correct, then the LineTo rendering should exactly match where the PatBlt would have rendered without that adjustment. If it ends up out of bounds, then that's clearly not the case, so something must be wrong with the strokeHalfWidth calculation. Maybe a rounding issue? I don't know.

Worst case, if you can't get it to work, at least move the strokeHalfWidth adjustment higher up, prior to where we're doing the first clamping (and get rid of this second set of clamping). Because we've got special case handling for the double underline offset, which is potentially going to be broken if you clamp it a second time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think another potential problem is that the maxUnderlineOffset that we're using for clamping is intended to be used with the original PatBlt offsets. Once you add the strokeHalfWidth adjustments, that range is no longer applicable. The only way I can see this working is if we can get that strokeHalfWidth calculation correct.

I'd suggest writing a little test app that renders a bunch of lines of varying widths, using both LineTo and PatBlt. From that you should be able to figure out exactly what offset needs to be applied at each width to get the two renderings to match.

Copy link
Contributor Author

@tusharsnx tusharsnx Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, I'm not well-versed in Windows programming in general, so creating an app from scratch would be a different task for me 😅

However, I did experiment with the idea of comparing LineTo and PatBlt, and it made it much more clear what I needed to do.

With this code:

// a fixed offset
const auto cellMidY = std::lround(fontHeight / 2.0f);

// adjust for drawing with DrawStrokedLine()
const auto cellMidYWithHalfStrokeWidth = cellMidY + gsl::narrow_cast<int>(std::floor(_lineMetrics.underlineWidth / 2));

// Select a brush

// draw a line with PatBlt()
RETURN_HR_IF(E_FAIL, !DrawLine(ptTarget.x, ptTarget.y + cellMidY, widthOfAllCells, _lineMetrics.underlineWidth));

// Select the dotted pen

// draw a line with LineTo()
RETURN_HR_IF(E_FAIL, !DrawStrokedLine(ptTarget.x, ptTarget.y + cellMidYWithHalfStrokeWidth, widthOfAllCells));

Result:

"`e[4:4m Dotted `e[m"

Screenshot 2023-12-10 112020 Screenshot 2023-12-10 113355

I tried this with other font sizes, and they all show the same result. So, the DrawLine() and DrawStrokedLine() calls with those args are synonymous to each other.

This still seems wrong to me. The offsets have already been clamped prior to this, so no additional clamping should have been necessary.

Yeah, you're right! It wouldn't have been necessary if I had floored strokeHalfWidth before adding it to the two offsets. (I had lround'd it, so that was the issue.)


// 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.
{
// 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)
{
curlyLinePeakHeight = 0.0f;
}
_lineMetrics.curlylinePeakHeight = gsl::narrow_cast<int>(std::floor(curlyLinePeakHeight));
const auto cellBottomGap = std::lround(Font.GetSize().height - _lineMetrics.underlineOffset - strokeHalfWidth);

// get the max height for curly line peak. Max height is in `em` units.
constexpr auto maxCurlyLinePeakHeightEm = 0.075f;
const auto maxCurlyLinePeakHeight = std::lround(maxCurlyLinePeakHeightEm * fontSize);

_lineMetrics.curlylinePeakHeight = std::clamp(cellBottomGap, 0L, maxCurlyLinePeakHeight);
}

// Now find the size of a 0 in this current font and save it for conversions done later.
Expand Down
Loading