Re-enable the setting to adjust the colors of indistinguishable text

[PankajBhojwani]

Summary of the Pull Request

Re-enable the setting to adjust indistinguishable text, with some changes:

• We now pre-calculate the nudged values for 'default bright foreground' as well
• When a color table entry is manually set (i.e. via VT sequences) we re-calculate the nudged values

References

#11095 #12160

PR Checklist

• Closes SGR 1 and SGR 8 don't work when adjusting lightness of indistinguishable text #11917
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated.

Validation Steps Performed

Indistinguishable text gets adjusted again

[PankajBhojwani]

In a follow-up, we will change the setting from a boolean value to an enum with 3 possible values: 'Never' to never adjust the text, 'Indexed' to only adjust the text if both the foreground/background are part of the current color scheme's color table (this is today's behaviour when the setting is set to "true"), and "Always" to always adjust the text if it is indistinguishable (this will be somewhat performance intensive since we will be calculating this at render time, but that is why it will be opt-in only)

[lhecker]

I don't quite understand why we shouldn't nudge brightened colors. 🤔

[lhecker]

this will be somewhat performance intensive since we will be calculating this at render time, but that is why it will be opt-in only

If performance turns out to be a problem we can use "redmean" still as I suggested offhand a while ago. I suspect it would work sufficiently well to determine a JND, since always nudging colors by 5 lightness is in a way a lot less "scientific" than the DeltaE algorithm itself, defeating its purpose a bit. This is the entire algorithm btw (output range 0-765):

double redmean(COLORREF c1, COLORREF c2) {
    int r1 = GetRValue(c1), g1 = GetGValue(c1), b1 = GetBValue(c1);
    int r2 = GetRValue(c2), g2 = GetGValue(c2), b2 = GetBValue(c2);
    int dr = r1 - r2;
    int dg = g1 - g2;
    int db = b1 - b2;
    int rmean = (r1 + r2) / 2;
    return sqrt((512 + rmean) * dr * dr / 256 + 4 * dg * dg + (767 - rmean) * db * db / 256);
}

[PankajBhojwani]

I don't quite understand why we shouldn't nudge brightened colors. 🤔

The issue arises when the colors are reversed and the foreground is bright - we will need to calculate what 'bright background' means but we don't have an easy way to do that

PR has changed a little (sorry!!)

[DHowett]

(Noting @j4james here, as he said "Finally, I think having bold in the color table is an essential prerequisite for getting the bold color to work when the option to Automaticaly adjust lightness of indistinguishable text is enabled (see #11917).". We did not end up choosing to fix 11939 beforehand, but may take the fix introduced here as a TODO on landing #11939. I would rather totally get rid of GetColor, given that 90% of its purpose is this weird brightness hack, which would necessitate breaking the workaround in this code. We could localize its implementation in RenderSettings.)

[j4james]

I think having bold in the color table is an essential prerequisite for getting the bold color to work

LOL. Clearly I was wrong, since the bold seems to working fine in this PR as far as I can tell.

That said, I still think this functionality should probably not be enabled by default, because there are going to be edges cases that don't work, and that's kind of inevitable when we're deliberately choosing to alter the colors that were requested.

As one example, if you're using a "standard" color scheme, like Campbell, where the default background is identical to black, and the default foreground is identical to white, then both these sequences should be invisible, but they aren't:

printf "\e[0;30mHIDDEN\e[m\n"
printf "\e[0;47mHIDDEN\e[m\n"

That particular case may be fixable, but there'll always be something. Having the default behavior be technically incorrect feels like we're asking for bug reports.

[j4james]

I don't know how well this would work in practice, but one idea I had for improving the performance was to move the color remapping from the renderer to the SetGraphicsRendition method in AdaptDispatch. It wouldn't work in quite the same way, but I think it should still produce a similar effect. Not suggesting that for this PR, though - just a thought for a potential future enhancement.

[lhecker]

I've had a think about this over the weekend. 🧠🤔
Not because I want to rudely intrude on other people's work more than necessary, but rather because I personally find this topic (rendering and thus the color topic) fascinating.

If we picture RGB colors as a 3D space and simplify things a bit, then the area below the JND (just-noticeable-difference of color) forms a simple sphere around our reference color. Within this coordinate system grayscale is the unit vector (1, 1, 1). Thus the question how much we need to brighten or darken the color of a point (= add or subtract such gray) so that it leaves the sphere of JND, becomes the calculation of a line-sphere intersection. This not just yields us immediately the 2 possible corrected colors, but also whether the line is within the sphere or not (= whether any adjustment needs to be done).

Of course the problem is that the MacAdam ellipse (the range of colors within the JND) is an ellipse/ellipsoid and not a sphere, but as mentioned before, redmean is a simple algorithm that apparently works sufficiently well and would allow us to scale the RGB axes to get an ellipsoid. The 3 scales would be, where R_0 is the red coordinate of the reference point (sphere center):

r = (2 + R_0 / 256) / 9
g = 4 / 9
b = (3 - R_0 / 256) / 9

Scaling the coordinates down by these factors ("squishing" the coordinate space), doing the line-sphere intersection and then scaling it back up should yield us appropriate results I believe.

In either case, in my testing this calculation can be done without any caching at all in real time (it wouldn't even show up in a perf trace) and (in a more limited testing) it seems to produce equally well adjusted colors. (Only the sqrt is a bit "problematic" but even that can be replaced with FRSQRTE/FRSQRTS on ARM64 and RSQRTSS on x86.)

[PankajBhojwani]

Had a chat with @lhecker, turns out the red-mean algorithm is not quite robust enough and just doesn't work as well as the delta e one. So we will stick to this algorithm for now and in the future come back to optimizing the delta e algorithm (Leonard has several ideas for how we can do this).

I've also switched the setting back to default off as per James' suggestion

[zadjii-msft]

I'm cool with this, but I'm gonna leave automerge off to give Dustin a last chance at it.

[ghost]

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[mailinglists35]

Hi, could you show an example of the relevant section of settings file where to enable this feature?
thanks

[mailinglists35]

Hi, could you show an example of the relevant section of settings file where to enable this feature? thanks

aha, figured out.

scroll to "profiles" to add it

    "profiles": 
    {
        "defaults": 
        {
            "adjustIndistinguishableColors": "always",
        },