-
Notifications
You must be signed in to change notification settings - Fork 622
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
Metal alpha blending fixes + color handling improvements #4913
Metal alpha blending fixes + color handling improvements #4913
Conversation
} else .{ | ||
.color = true, | ||
.depth = 4, | ||
.space = try macos.graphics.ColorSpace.createDeviceRGB(), | ||
.space = try macos.graphics.ColorSpace.createNamed(.displayP3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qwerasd205 suggestion: Should we make this ...ColorSpace.createFromName(.displayP3)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I could understand changing it to createWithName
if we wanted to match the name of the CoreGraphics function, but this really feels like splitting hairs either way. I'm not devoted to the name, but if I changed it I'd be more likely to change it to createWithName
than createFromName
unless there's particularly good reasoning...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qwerasd205 The way I see it, we are creating a ColorSpace
from a ColorSpace.Name
called .displayP3
.
From that perspective, it would logically flow into ColorSpace.createFromName(.displayP3)
.
I do think createWithName
is also a great option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically our macos
pkg is a thin layer over Carbon/AppKit APIs and we just mirror their names, so technically the correct name here is createWithName
but I'm not going to lose sleep over it and don't really care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitchellh @qwerasd205 Im also happy if we leave it the way it is.
Resolving comment.
@@ -520,6 +520,7 @@ pub fn clone( | |||
assert(node.data.capacity.rows >= chunk.end - chunk.start); | |||
defer node.data.assertIntegrity(); | |||
node.data.size.rows = chunk.end - chunk.start; | |||
node.data.size.cols = chunk.node.data.size.cols; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took me over an hour to find 😭
Otherwise pages may have the wrong width if they were resized down with a fast path that just chanes the size without adjusting capacity at all.
This commit is quite large because it's fairly interconnected and can't be split up in a logical way. The main part of this commit is that alpha blending is now always done in the Display P3 color space, and depending on the configured `window-colorspace` colors will be converted from sRGB or assumed to already be Display P3 colors. In addition, a config option `text-blending` has been added which allows the user to configure linear blending (AKA "gamma correction"). Linear alpha blending also applies to images and makes custom shaders receive linear colors rather than sRGB. In addition, an experimental option has been added which corrects linear blending's tendency to make dark text look too thin and bright text look too thick. Essentially it's a correction curve on the alpha channel that depends on the luminance of the glyph being drawn.
6c5b251
to
a8b9c5b
Compare
) This fixes a regression introduced by the rework of this area before during the color space changes (#4913). It seems like the original intent of this code was the behavior it regressed to, but it turns out to be better like this.
This PR addresses #2125 for the Metal renderer. Both options are available: "Apple-style" blending where colors are blended in a wide gamut color space, which reduces but does not eliminate artifacts; and linear blending where colors are blended in linear RGB.
Because this doesn't add support for linear blending on Linux, I don't know whether the issue should be closed or not.
List of changes in no particular order
window-colorspace
todisplay-p3
then terminal colors are automatically converted from sRGB to the corresponding Display P3 color in the shaderbackgroundColor
property on theCAMetalLayer
, because this should fix the flash of transparency during startup (macOS: background bleeds/flicker when resizing splits, creating tabs #4516) and the weirdness at the edge of the window when resizing. I didn't make that a part of this PR because it requires further changes and my changes are already pretty significant, but I can make it a follow-up.Main takeaways
Color blending now matches nearly identically to Apple apps like Terminal.app and TextEdit, not quite identical in worst case scenarios, off by the tiniest bit, because the default color space is slightly different than Display P3.
Linear alpha blending is now available for mac users who prefer more accurate color reproduction, and personally I think it looks very nice with the alpha correction turned on, I will be daily driving that configuration.
Future work
CALayer.backgroundColor
instead of in shader, to avoid issues around edges when resizing.