Curly underline is rendered as single

[alabuzhev]

Windows Terminal version

1.20.3131.0

Windows build number

10.0.19045.3448

Other Software

No response

Steps to reproduce

Originally posted in #16097 (comment)

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.:

Actual Behavior

Curly is rendered as single in both OpenConsole and WT:

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:

Technically it's not too small if the wave height is at least 2 pixels:

Additionally, OpenConsole with even smaller sizes, e.g. Consolas 15, doesn't render anything but "double" at all:

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.

[tusharsnx]

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)

terminal/src/renderer/atlas/BackendD3D.cpp

Lines 311 to 313 in d14524c

	// 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;

GDI (Conhost)

terminal/src/renderer/gdi/state.cpp

Lines 410 to 432 in d14524c

	if (_lineMetrics.underlineWidth < 1)
	{
	_lineMetrics.curlylinePeakHeight = 0;
	}
	else
	{
	// 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));
	}

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.)

[j4james]

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:

terminal/src/renderer/gdi/paint.cpp

Lines 622 to 626 in d14524c

	const auto underlineMidY = std::lround(ptTarget.y + _lineMetrics.underlineOffset + _lineMetrics.underlineWidth / 2.0f);
	if (lines.test(GridLines::Underline))
	{
	return DrawStrokedLine(ptTarget.x, underlineMidY, widthOfAllCells);
	}

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?

[j4james]

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:

terminal/src/renderer/gdi/state.cpp

Lines 379 to 383 in d14524c

	// We always want the lines to be visible, so if a stroke width ends
	// up being zero, we need to make it at least 1 pixel.
	_lineMetrics.gridlineWidth = std::max(_lineMetrics.gridlineWidth, 1);
	_lineMetrics.underlineWidth = std::max(_lineMetrics.underlineWidth, 1);
	_lineMetrics.strikethroughWidth = std::max(_lineMetrics.strikethroughWidth, 1);

So I think the test below should never be necessary.

terminal/src/renderer/gdi/state.cpp

Lines 409 to 413 in d14524c

	// Curly line doesn't render properly below 1px stroke width. Make it a straight line.
	if (_lineMetrics.underlineWidth < 1)
	{
	_lineMetrics.curlylinePeakHeight = 0;
	}

[DHowett]

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!

[DHowett]

Hey @tusharsnx, I've got a crazy plan... but it might need this bug fixed for me to pull it off 😁

(are you able to look into it?)

[tusharsnx]

@DHowett

(are you able to look into it?)

Almost ready! 🙂

[DHowett]

Okay I definitely thought that's what you meant on Twitter, I just had to make sure!