Inverse cursor color logic is (sometimes) broken

[alabuzhev]

Environment

Windows build number:
Microsoft Windows [Version 10.0.18362.449]

Windows Terminal version (if applicable):
No.

Any other software?
No.

Steps to reproduce

1. Open cmd.exe.
2. Go to window properties, "Colors" page.
3. Set "Screen Background" value to exactly 128 128 128.

Expected behavior

Cursor is visible no matter what background color is.

Actual behavior

Now you see it, now you don't.

Current "inverse" logic is too straightforward - it literally takes the inverse of the background, and for 128 128 128 the inverse is... 127 127 127, which is basically the same thing. Oops.

Note: Legacy console handles this corner case properly, as well as all Windows versions prior to 10.

Also, legacy console inversion algorithm is not exactly "inverse" - the colors are slightly different (and subjectively more pleasant). Is it possible to get this classic inversion in the modern console please?

This issue affects a text editor app that uses console color 8 (128 128 128 in classic color scheme) to highlight various parts of text (e.g. current line). Sudden cursor disappearing is confusing and annoying.

Example:

[alabuzhev]

I've experimented a bit and managed to get exactly the same cursor colors as in the Classic Console:

GDI:

diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 2f067fc5a..a9f9c77d1 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -733,7 +733,11 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
 
         for (auto r : cursorInvertRects)
         {
-            RETURN_HR_IF(E_FAIL, !(InvertRect(_hdcMemoryContext, &r)));
+            // Make the inverted cursor slightly darker to keep it distinct in all cases (see gh-3647).
+            const auto PrevObject = SelectObject(_hdcMemoryContext, GetStockObject(LTGRAY_BRUSH));
+            const auto Result = PatBlt(_hdcMemoryContext, r.left, r.top, r.right - r.left, r.bottom - r.top, PATINVERT);
+            SelectObject(_hdcMemoryContext, PrevObject);
+            RETURN_HR_IF(E_FAIL, !Result);
         }
     }
 

DX:

diff --git a/src/renderer/dx/CustomTextRenderer.cpp b/src/renderer/dx/CustomTextRenderer.cpp
index f3a894a43..7ad00c923 100644
--- a/src/renderer/dx/CustomTextRenderer.cpp
+++ b/src/renderer/dx/CustomTextRenderer.cpp
@@ -395,8 +395,9 @@ try
         if (firstPass)
         {
             // Draw a backplate behind the cursor in the *background* color so that we can invert it later.
-            // We're going to draw the exact same color as the background behind the cursor
-            const til::color color{ drawingContext.backgroundBrush->GetColor() };
+            // We're going to draw a slightly lighter color than the background behind the cursor,
+            // so that after the inversion it will be slightly darker and distinct in all cases (see gh-3647).
+            const til::color color{ til::color{ drawingContext.backgroundBrush->GetColor() } ^ RGB(63, 63, 63) };
             RETURN_IF_FAILED(d2dContext->CreateSolidColorBrush(color.with_alpha(255),
                                                                &brush));
         }

Atlas:

diff --git a/src/renderer/atlas/shader_ps.hlsl b/src/renderer/atlas/shader_ps.hlsl
index 789cd98b0..fce0b9517 100644
--- a/src/renderer/atlas/shader_ps.hlsl
+++ b/src/renderer/atlas/shader_ps.hlsl
@@ -168,6 +168,8 @@ float4 main(float4 pos: SV_Position): SV_Target
         [flatten] if (cursorColor == INVALID_COLOR && glyphs[cellPos].a != 0)
         {
             color = float4(1 - color.rgb, 1);
+            // Make the inverted cursor slightly darker to keep it distinct in all cases (see gh-3647).
+            color = abs(color - 0.25);
         }
     }

Before:

^^^ - this!

After:

The logic is pretty simple and equivalent to making the inverted color 25% darker:

uint32_t Color = current_cell_color();
uint32_t InvertedColor = ~Color & 0x00FFFFFF;
uint32_t CursorColor = InvertedColor ^ 0x003F3F3F;

[alabuzhev]

I've added DX and Atlas renderer changes to the previous comment.
Could you have a look please?
Would a PR be welcomed?

[zadjii-msft]

Lemme /cc @lhecker cause I think he's got some thoughts on the matter.

[lhecker]

Correct me if I'm wrong but the XOR results in this:

But the solution for AtlasEngine is unlike that since it always subtracts 0.25 brightness so you get an intersection between input and output for RGB(96, 96, 96).
So in case of AtlasEngine we could get a bit more creative... For instance this:

bool odd = (cellPos.x ^ cellPos.y) & 1;
float inv = odd ? 0.8 : 1.0;
color = float4(saturate(inv - color.rgb), 1);

This, with the help of odd, creates a checkerboard image (background 127 gray):

Or default "black" (RGB = 12) background:

We can also use the alpha channel of the cursor texture for inversion:

color = float4(saturate(glyphs[cellPos].a - color.rgb), 1);

...and then draw a only a semi-transparent content in the glyph texture in AtlasEngine::_drawCursor which results in the same darkening effect.

In general I'm in favor of such a change, but I'm unsure what the best approach for the visuals is.

[alabuzhev]

@lhecker thanks, the XOR plot is correct, or, alternatively:

the solution for AtlasEngine is unlike that

Mea culpa 🤦
It was supposed to be the same, but for some reason I decided to "simplify" it...
Something like this should work (and can probably be improved further):

diff --git a/src/renderer/atlas/shader_ps.hlsl b/src/renderer/atlas/shader_ps.hlsl
index 789cd98b0..320f2895e 100644
--- a/src/renderer/atlas/shader_ps.hlsl
+++ b/src/renderer/atlas/shader_ps.hlsl
@@ -168,6 +168,8 @@ float4 main(float4 pos: SV_Position): SV_Target
         [flatten] if (cursorColor == INVALID_COLOR && glyphs[cellPos].a != 0)
         {
             color = float4(1 - color.rgb, 1);
+            float3 shifted = ((uint3(color.rgb * 255) ^ 0x3f) & 0xff) / 255.0;
+            color = float4(shifted.rgb, 1);
         }
     }

I'm unsure what the best approach for the visuals is.

I'm all for creativity and fancy visuals, but it would be nice to at least have a uniform, always readable and already tested solution everywhere first, even if not the most creative one.

[zadjii-msft]

If you'd like to submit a PR for this - go for it!

[ghost]

🎉This issue was addressed in #13748, which has now been successfully released as Windows Terminal Preview v1.16.252.:tada:

Handy links:

• Release Notes
• Store Download

[lhecker]

BTW I'm intending to regress this issue a little bit in an upcoming version of AtlasEngine. The new rendering approach makes implementing inverted cursors, in particular the double-underline and empty-box variants, a little bit more difficult and I'd like to skip that at least in the beginning. This is because the new approach draws text in a layered approach with multiple passes and so it doesn't have the color information of each pixel readily available in order to XOR it.

So the new approach I've chosen only inspects the background color of a cell. If all components of the background color are within ±32 of #808080 gray, it chooses to consistently darken the range by -32, including the text content. In other words, it looks something like this:

I could also extend the range to ±64 and darken by -64, but I felt like this already resulted in a sufficient contrast. I hope that this will be fine, if at least until I get to implement proper inversion the way we have it now.

[alabuzhev]

within ±32 of #808080 gray

Isn't it ±16 in the picture?

[lhecker]

Ah woops. Yes it's ±16. It results in a -32 darkening, because when you invert a +16 color you get a -16 and so the delta is 32. I've considered using ±32 with a -64 darkening as well, but wasn't sure what to best pick. I was thinking that a smaller ±N could potentially result in a better visual "consistency".

Edit: I found that printf '\x1b[48;2;127;127;111m' produces a color that makes the cursor invisible with this approach. I suppose this happens since blue is the color humans have the hardest time differentiating. I'll try ±32 tomorrow.

[alabuzhev]

printf '\x1b[48;2;127;127;111m' produces a color that makes the cursor invisible with this approach

Isn't that just a plain inversion on your screenshot? ~(127, 127, 111) = (128, 128, 144)
I'd expect a color with -32 darkening applied to be, well, darker by (-32, -32, -32), not lighter by (+1, +1, +33).

[alabuzhev]

Ah, "If all components of the background color are within...", and this one doesn't match.
±32 might help indeed.

[lhecker]

Yeah it's a plain inversion, because one of the components isn’t within the "±16" range anymore - at least according to the way I built it (which uses 0x80/128 as the gray value to make some bit twiddling simpler). If it was 112 instead of 111 the darkening effect would apply and the cursor would be visible. Sorry, I should’ve been more specific in my edit.
But this has shown me that my approach clearly isn’t perfect yet. I‘m still hopeful that it works if I increase it to "±32", because an inversion of 127;127;95 to 128;128;169 should hopefully be visible at least. 😅

[alabuzhev]

Alternatively, instead of "components are within the ±16 range" it could be "the distance from the center of the RGB cube is within the N range".
A sphere might capture gray-ish pixels better than a cube (with more prominent vertices and edges) and allow to specify a larger range if needed.

[lhecker]

I just realized that I can also just draw the background XOR'd with 63 (0x3f3f3f) where the cursor is and then draw text on top in its regular color. If I then invert the area where the cursor is (via D3D11_BLEND_OP_SUBTRACT), it'll result in a background color as if it was XOR'd with 192 (0xc0c0c0). I'm not sure if it'll be a practical problem that the text color isn't XOR'd with 63/192, but it does at least solve the issue where the cursor is invisible on top of a gray background:

Here's the current AtlasEngine for reference:

The benefit of the current approach is that colors that are close together, stay close together. However I'm not 100% certain whether that's a good thing or not...