Skip to content

Fix til::color::layer_over#13946

Merged
3 commits merged intomainfrom
dev/migrie/b/float-colors-are-hard
Sep 9, 2022
Merged

Fix til::color::layer_over#13946
3 commits merged intomainfrom
dev/migrie/b/float-colors-are-hard

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 8, 2022

The color of inactive tab text is incorrect since #13689 due to the introduction
of til::color::layer_over which incorrectly calculated the RGB values.

Validation Steps Performed

  • Added unit tests ✅

Co-authored-by: Leonard Hecker lhecker@microsoft.com

@zadjii-msft zadjii-msft added Severity-Blocking We won't ship a release like this! No-siree. Area-Theming Anything related to the theming of elements of the window labels Sep 8, 2022
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 8, 2022
@ghost
Copy link

ghost commented Sep 8, 2022

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 7 hours 1 minute. No worries though, I will be back when the time is right! 😉

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.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

still my open Q but approving since it's EoD and it's small and you tested it

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

(I believe that this change has a bug. See my comment above.)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 8, 2022
@lhecker lhecker changed the title Fix a bug in determining the color of inactive tab text Fix til::color::layer_over Sep 9, 2022
@lhecker
Copy link
Member

lhecker commented Sep 9, 2022

I wrote a simple HTML page testing the alpha blending between blue and orange colors.
I then imported the resulting colors into a unit tests which allowed me to quickly find the underlying bug:

(r * sourceAlpha + destination.r * destinationAlpha * aInverse)

is in the range [0, 255] just like resultA, so after the / resultA you end up with [0, 1]. Multiplying by * 255.0f would fix this issue. But with the unit tests in place, I was able to simplify the code quite a bit. The new alpha blending function is much simpler.

const auto resultB = (b * sourceAlpha + destination.b * destinationAlpha * aInverse) / resultA;
const auto resultR = (r * a + destination.r * destination.a * aInverse) / resultA;
const auto resultG = (g * a + destination.g * destination.a * aInverse) / resultA;
const auto resultB = (b * a + destination.b * destination.a * aInverse) / resultA;
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to multiply by 255 if we simply make sure that the part in front of the division is in the range [0, 255^2]. After dividing it by resultA ([0, 255]) we get a result in the range [0, 255].

@ghost ghost merged commit c02bb29 into main Sep 9, 2022
@ghost ghost deleted the dev/migrie/b/float-colors-are-hard branch September 9, 2022 14:00
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-Theming Anything related to the theming of elements of the window AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Severity-Blocking We won't ship a release like this! No-siree.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants