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

Use grayscale AA always on non-opaque backgrounds #5277

Merged
9 commits merged into from
Apr 24, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 7, 2020

Summary of the Pull Request

When we're on acrylic, we can't have cleartype text unfortunately. This PR changes the DX renderer to force cleartype runs of text that are on a non-opaque background to use grayscale AA instead.

References

Here are some of the URLS I was referencing as writing this:

Additionally:

PR Checklist

Detailed Description of the Pull Request / Additional comments

Basically, if you use cleartype on a light background, what you'll get today is the text foreground color added to the background. This will make the text look basically invisible.

So, what I did was use some trickery with PushLayer to basically create a layer where the text would be forced to render in grayscale AA. I only enable this layer pushing business when both:

  • The user has enabled cleartype text
  • The background opacity < 1.0

This plumbs some information through from the TermControl to the DX Renderer to make this smooth.

Validation Steps Performed

Opened both cleartype and grayscale panes SxS, and messed with the opacity liberally.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues labels Apr 8, 2020
…leartype-on-light

# Conflicts:
#	src/renderer/dx/DxRenderer.cpp
@zadjii-msft zadjii-msft marked this pull request as ready for review April 13, 2020 21:06
@zadjii-msft
Copy link
Member Author

BLOCK THIS

It doesn't work on first launch when you've got acrylic + cleartype. In that scenario it starts by using the solid BG by default. Need to investigate...

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Someone asked for this to be blocked. Sorry champ I don’t make the rules.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2020
@zadjii-msft
Copy link
Member Author

BLOCK THIS

It doesn't work on first launch when you've got acrylic + cleartype. In that scenario it starts by using the solid BG by default. Need to investigate...

Fixed in latest commit

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I'd feel a LOT more comfortable if @DHowett-MSFT or @miniksa signs off on this.


// Make sure we redraw all the cells, to update whether they're actually
// drawn with cleartype or not.
// We don't terribly care if this fails.
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why don't we care?

Copy link
Member

Choose a reason for hiding this comment

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

It's not worth crashing over if it does fail. It'll just make a graphical glitch... is my most likely candidate for a reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's definitely the reason

@zadjii-msft
Copy link
Member Author

@DHowett-MSFT you blocked this for me, unblock yo self plz

@DHowett-MSFT
Copy link
Contributor

Oop

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 24, 2020
@ghost
Copy link

ghost commented Apr 24, 2020

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 ghost merged commit c803893 into master Apr 24, 2020
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using cleartype anti aliasing makes dark foreground invisible on light background.
4 participants