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.

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

Refactor the renderer color calculations #6853

Merged
7 commits merged into from
Jul 10, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jul 9, 2020

This is a refactoring of the renderer color calculations to simplify the
implementation, and to make it easier to support additional
color-altering rendition attributes in the future (e.g. faint and
conceal).

References

Detailed Description of the Pull Request / Additional comments

When we added support for DECSCNM, that required the foreground and
background color lookup methods to be able to return the opposite of
what was requested when the reversed mode was set. That made those
methods unnecessarily complicated, and I thought we could simplify them
considerably just by combining the calculations into a single method
that derived both colors at the same time.

And since both conhost and Windows Terminal needed to perform the same
calculations, it also made sense to move that functionality into the
TextAttribute class, where it could easily be shared.

In general this way of doing things is a bit more efficient. However, it
does result in some unnecessary work when only one of the colors is
required, as is the case for the gridline painter. So to make that less
of an issue, I've reordered the gridline code a bit so it at least
avoids looking up the colors when no gridlines are needed.

Validation Steps Performed

Because of the API changes, quite a lot of the unit tests had to be
updated. For example instead of verifying colors with two separate calls
to LookupForegroundColor and LookupBackgroundColor, that's now
achieved with a single LookupAttributeColors call, comparing against a
pair of values. The specifics of the tests haven't changed though, and
they're all still working as expected.

I've also manually confirmed that the various color sequences and
rendition attributes are rendering correctly with the new refactoring.

@j4james j4james force-pushed the refactor-color-calculation branch from cc97bfe to 8a7830c Compare July 9, 2020 14:50
@j4james j4james marked this pull request as ready for review July 9, 2020 15:11
@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

This will also make it easier to do the perceptual color nudging suggested in #2638. Thank you for separating this out of DECSCNM 😄

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
@DHowett DHowett requested a review from zadjii-msft July 9, 2020 18:35
@j4james
Copy link
Collaborator Author

j4james commented Jul 9, 2020

This will also make it easier to do the perceptual color nudging suggested in #2638.

Actually I was thinking it might be better to handle the perceptual color adjustments at the time that palette was set, so you're not constantly doing those calculations on every paint. Color scheme changes should be comparatively rare.

@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

I was wondering about that. I'm worried that we'd need to do it for every foreground * every background (from the first 16 colors) and store that somewhere. I'm not too bothered by doing that, but I do want to make sure we understand the risks.

Also, what if an application requests a 16-color foreground on a nearly identical 256-color or RGB background? Is the point of perceptual nudging to solve bad color palettes or to solve bad runtime accessibility?

I'm not certain!

@j4james
Copy link
Collaborator Author

j4james commented Jul 9, 2020

Also, what if an application requests a 16-color foreground on a nearly identical 256-color or RGB background? Is the point of perceptual nudging to solve bad color palettes or to solve bad runtime accessibility?

I'm not certain!

Me neither. But my impression from playing with the ConEmu implementation was that they were just fixing bad palettes. Interestingly, though, whatever they're doing didn't seem to help with Solarized, which is what I thought it would be most useful for, so that kind of put me off the idea. Although there's no reason why we need to mimic their behaviour exactly - it needs a bit of thought.

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jul 10, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT July 10, 2020 19:37
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I didn't know how much I wanted this until I read it. Thank you very much, @j4james.

@miniksa
Copy link
Member

miniksa commented Jul 10, 2020

The only thought that floated through my head was "I wonder if this is worse for perf in a circumstance where someone only wants the fg or only wants the bg and we're working on both. But then I thought "huh I bet that's never really a thing, now is it." Or it's not a common enough case for it to matter.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 10, 2020
@ghost
Copy link

ghost commented Jul 10, 2020

Hello @DHowett!

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 ghost merged commit 3388a48 into microsoft:master Jul 10, 2020
@j4james j4james deleted the refactor-color-calculation branch July 11, 2020 12:16
@DHowett
Copy link
Member

DHowett commented Aug 21, 2020

This PR, and the other followups around color handling, finally shipped in the traditional console with insider build 20197. 😄 Thanks again

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants