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.

Sign up for GitHub

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

Jump to bottom

Fix til::color::layer_over #13946

Merged
3 commits merged into from
Sep 9, 2022
Merged

Fix til::color::layer_over #13946

3 commits merged into from
Sep 9, 2022

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 8, 2022 •
edited by lhecker
Loading

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
DHowett
DHowett reviewed Sep 8, 2022
View reviewed changes
src/inc/til/color.h Outdated Show resolved Hide resolved
@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.

DHowett
DHowett approved these changes Sep 8, 2022
View reviewed changes
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

lhecker
lhecker requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment •
edited
Loading

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 •
edited
Loading

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.

lhecker
lhecker reviewed Sep 9, 2022
View reviewed changes
src/inc/til/color.h
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
Reviewers

@lhecker lhecker lhecker approved these changes

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zadjii-msft @lhecker @DHowett