Implement the Delta E algorithm to improve color perception by PankajBhojwani · Pull Request #11095 · microsoft/terminal

PankajBhojwani · 2021-08-31T21:19:16Z

Implements the Delta E algorithm
Uses the Delta E algorithm to precalculate adjusted foreground values based on possible foreground/background color pairs in the color table
Adds a setting to use the adjusted foreground values when applicable

PR Checklist

Closes Implement Delta E 2000 to correct color perception #2638
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.
I work here

Validation Steps Performed

Before:

After (note dark blue):

zadjii-msft · 2021-08-31T21:39:36Z

Can we make this a (default on) setting?

PankajBhojwani · 2021-08-31T21:44:46Z

Can we make this a (default on) setting?

That's probably a good idea

j4james · 2021-09-02T23:03:33Z

This seems like it would have a significant hit on performance. I could have sworn ConEmu only applied the algorithm to the initial palette initialization, but this looks like it's adjusting the colors on every color lookup in the renderer.

Also, would this not break the "concealed" SGR attribute, which deliberately sets the foreground and background colors to the same values?

DHowett · 2021-09-02T23:59:35Z

This seems like it would have a significant hit on performance. I could have sworn ConEmu only applied the algorithm to the initial palette initialization, but this looks like it's adjusting the colors on every color lookup in the renderer.

That's a good question; I understood it to be at render time for them as well (otherwise, they need to compute each color's contrast on each background and determine whether to nudge that foreground only on that background or nudge it generally... potentially having to resolve a recursive conflict with other colors?), but I may indeed be wrong.

EDIT: Also, perhaps, to improve the contrast of applications that use true color? I'm not sure whether that is a noble goal.

EDIT 2: However, it might be a noble goal if we want to add a feature for High Contrast users to actually induce high contrast in otherwise intransigent applications. (EDIT 3: Which I do -- that is an accessibility feature that could help people quite a bit!)

Also, would this not break the "concealed" SGR attribute, which deliberately sets the foreground and background colors to the same values?

Oh boy, yeah, almost certainly.

DHowett · 2021-09-03T00:04:11Z

EDIT 4: Surprisingly, i think it does do it for True Color

Here's ConEmu

Terminal

j4james · 2021-09-03T00:37:18Z

Hmmm... maybe it's doing it at the time the SGR attributes are applied then. That's probably better than render time, but that still seems like it would be expensive to me, especially if you're planning to have this enabled by default.

j4james · 2021-09-03T22:32:01Z

I've had a chance to do a bit more testing on ConEmu now, and it looks to me like it doesn't really support true color - it just maps those sequences to values in the palette. And even with the color adjustment disabled, that mapping seems to be choosing distinguishable colors, which is why it appears to be "working" for you. Also, if I use the 8-color SGR sequences, it's quite happy to let me use black-on-black or white-on-white. It's only color combinations that are close, but not the same, that get adjusted.

That said, I don't think we necessarily need to match what ConEmu is doing exactly, but I do think we need be selective about the situations in which this algorithm is applied, because it definitely has the potential to breaks things.

RGB colors aren't typically a problem, because an app is unlikely to choose a bad RGB color combination unless they had a good reason for it (e.g. they;re creating a gradient effect), which we really shouldn't be "fixing". Palette colors are where the problems arise, because the app doesn't know what palette combinations are distinguishable, and color scheme designers have a well known history of poor choices.

So I had originally thought this feature would just be making sure that every color combination in the palette was distinguishable (i.e. a way to fix the broken color schemes). That would assumedly have zero render-time expense, but would probably cover 99% of the visibility issues that users are facing.

PankajBhojwani · 2021-09-07T18:46:16Z

So I had originally thought this feature would just be making sure that every color combination in the palette was distinguishable (i.e. a way to fix the broken color schemes). That would assumedly have zero render-time expense, but would probably cover 99% of the visibility issues that users are facing.

Yep this is probably the better way to do it, thank you!

carlos-zamora

Love it! Thanks!

DHowett

(I would like to review this! Blocking for a chance.)

DHowett

I'm being nitpicky, I know. I'm sorry. I just had the open question really for Leonard 😄

src/cascadia/TerminalCore/Terminal.cpp

DHowett · 2021-10-07T19:17:35Z

src/cascadia/TerminalCore/Terminal.hpp


 static constexpr std::wstring_view linkPattern{ LR"(\b(https?|ftp|file)://[-A-Za-z0-9+&@#/%?=~_|$!:,.;]*[A-Za-z0-9+&@#/%=~_|$])" };
 static constexpr size_t TaskbarMinProgress{ 10 };
+static constexpr size_t DefaultBgIndex{ 16 };


This is kind-of an implementation detail that you could keep in the c++ file

Just because... it pollutes the global namespace of any file that includes it with some generically-named DefaultBgIndex constant 😄

same below ofc

Makes sense! Moved _MakeAdjustedColorArray into TerminalRenderData.cpp and these constants are only defined in that c++ file now

DHowett · 2021-10-07T19:19:53Z

src/cascadia/TerminalCore/Terminal.hpp


    void _NotifyTerminalCursorPositionChanged() noexcept;

+    std::array<std::array<COLORREF, 18>, 18> _adjustedForegroundColors;


We're spending 1296 bytes per Terminal instance -- @lhecker, is that something you're okay with? Or should we find a better way to store this that can go away if the feature is disabled?

If the data isn't accessed it's not in the CPU. Optimally I'd place it somewhere at the start or end of the struct, so all other (smaller) members can live closely together and get cached together.
Since that's not the case at the moment I can only suggest @PankajBhojwani to move the member before this is merged.

1.2kB of RAM usage isn't much either nowadays. Personally I'd lazily allocate it on the heap just because it's so large, but purely from a performance perspective it's not important to do that. What would be much better though would be to compress data to fit more of it into our modern CPUs! Halving the data for instance is easy, since storing signed 5-bit RGB offsets would fit into 16 bit per color instead of 32.

Considering that this should go into 1.12 I'd do neither "optimization" and just move the member (but moving it is important).

Moved it to the bottom of the struct!

DHowett

just fix or kick that failing build, and we're good! thanks!

ghost · 2021-10-07T22:42:30Z

Hello @PankajBhojwani!

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.

hectormz · 2021-10-08T15:17:50Z

Thanks @PankajBhojwani @zadjii-msft @DHowett and others for this feature!

This commit enables /fp:fast. This doubles the performance of the Delta E computation in #11095 for instance. Additionally it re-enables two options for debug builds which are normally enabled by default by Visual Studio. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * No change in binary size * No obvious change in behavior

ghost · 2021-10-20T17:06:40Z

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

Handy links:

…11095)" This reverts commit dd5dbb2.

…le text (#12160) ## Summary of the Pull Request Disables the automatic adjustment of indistinguishable text (added in #11095) because of the concerns brought up in #11917. Also, the setting is hidden in the SUI for as long as this feature remains disabled. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here

…13343) ## 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 * [x] Closes #11917 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. ## Validation Steps Performed Indistinguishable text gets adjusted again

PankajBhojwani added 2 commits August 31, 2021 13:42

transfer

6373bae

works

da2cad8

This comment has been minimized.

Sign in to view

remove

d1c9748

This comment has been minimized.

Sign in to view

method descriptions

0c8168b

This comment has been minimized.

Sign in to view

PankajBhojwani added 5 commits September 1, 2021 13:17

spell, format

7b1eefe

order

f8003d3

default on setting

e4fced6

remove colorspace

041e5ce

neater

12e8d6c

This comment has been minimized.

Sign in to view

private

384bf49

This comment has been minimized.

Sign in to view

dedcode

9f9af2e

PankajBhojwani added 2 commits September 8, 2021 14:52

delete a bunch, static functions

326decb

update comments

5bcbde2

This comment has been minimized.

Sign in to view

spell

daf815f

zadjii-msft added the zBugBash-Consider label Oct 4, 2021

zadjii-msft added this to the Terminal v1.12 milestone Oct 5, 2021

rename setting, only ignore same index

a95fb42

carlos-zamora approved these changes Oct 5, 2021

View reviewed changes

DHowett requested changes Oct 5, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 5, 2021

DHowett requested changes Oct 7, 2021

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 7, 2021

PankajBhojwani added 2 commits October 7, 2021 13:46

Constants only in cpp file

7277f9d

move member

acb2fb4

DHowett approved these changes Oct 7, 2021

View reviewed changes

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 7, 2021

ghost merged commit dd5dbb2 into main Oct 7, 2021

ghost deleted the dev/pabhoj/delta_e branch October 7, 2021 22:43

lhecker mentioned this pull request Oct 8, 2021

Enable fast floating point model and fast debug linking #11466

Merged

2 tasks

ghost mentioned this pull request Oct 20, 2021

Implement Delta E 2000 to correct color perception #2638

Closed

zadjii-msft mentioned this pull request Dec 8, 2021

Deprecate and eventually replace Solarized #6997

Closed

PankajBhojwani mentioned this pull request Jan 13, 2022

Disable being able to set the automatic adjustment of indistinguishable text #12160

Merged

6 tasks

PankajBhojwani added a commit that referenced this pull request Jan 14, 2022

Revert "Implement the Delta E algorithm to improve color perception (#…

83b9a4c

…11095)" This reverts commit dd5dbb2.

carlos-zamora mentioned this pull request May 9, 2022

[Epic] Improved High Contrast support #12999

Open

10 tasks

PankajBhojwani mentioned this pull request Jun 20, 2022

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

Merged

5 tasks

This pull request was closed.


		void _NotifyTerminalCursorPositionChanged() noexcept;

		std::array<std::array<COLORREF, 18>, 18> _adjustedForegroundColors;

Conversation

PankajBhojwani commented Aug 31, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Validation Steps Performed

Uh oh!

This comment has been minimized.

zadjii-msft commented Aug 31, 2021

Uh oh!

PankajBhojwani commented Aug 31, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

j4james commented Sep 2, 2021

Uh oh!

This comment has been minimized.

DHowett commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Sep 3, 2021

Uh oh!

j4james commented Sep 3, 2021

Uh oh!

j4james commented Sep 3, 2021

Uh oh!

PankajBhojwani commented Sep 7, 2021

Uh oh!

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

lhecker Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 7, 2021

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.

Uh oh!

hectormz commented Oct 8, 2021

Uh oh!

ghost commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PankajBhojwani commented Aug 31, 2021 •

edited by DHowett

Loading

DHowett commented Sep 2, 2021 •

edited

Loading

lhecker Oct 7, 2021 •

edited

Loading

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.