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

gpui: Add linear gradient support to fill background #20812

Merged
merged 40 commits into from
Dec 11, 2024

Conversation

huacnlee
Copy link
Contributor

@huacnlee huacnlee commented Nov 18, 2024

Release Notes:

  • gpui: Add linear gradient support to fill background

Run example:

cargo run -p gpui --example gradient
cargo run -p gpui --example gradient --features macos-blade

Demo

In GPUI (sRGB):

image

In GPUI (Oklab):

image

In CSS (sRGB):

https://codepen.io/huacnlee/pen/rNXgxBY

image

In CSS (Oklab):

https://codepen.io/huacnlee/pen/wBwBKOp

image

Currently only support 2 color stops with linear-gradient. I think this is we first introduce the gradient feature in GPUI, and the linear-gradient is most popular for use. So we can just add this first and then to add more other supports.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Nov 18, 2024
@huacnlee huacnlee changed the title gpui: Add linear gradient support to fill background. gpui: Add linear gradient support to fill background Nov 18, 2024
@widersky
Copy link

It would be great to have the ability to add a gradient to any interface element!

@iamnbutler
Copy link
Member

So excited for this if we can make it work! Thanks for working on it.

@huacnlee huacnlee marked this pull request as ready for review November 19, 2024 10:06
@huacnlee
Copy link
Contributor Author

huacnlee commented Nov 19, 2024

Linux and Windows support have done

@jansol
Copy link
Contributor

jansol commented Nov 20, 2024

I'm not 100% sure but it looks like the mix() is working with sRGB values? The colors should be converted to linear RGB before and back after, else there will be awkward inconsistencies like you can see when comparing the yellow-green gradient between CSS and Linux/Windows.

@huacnlee
Copy link
Contributor Author

I'm not 100% sure but it looks like the mix() is working with sRGB values? The colors should be converted to linear RGB before and back after, else there will be awkward inconsistencies like you can see when comparing the yellow-green gradient between CSS and Linux/Windows.

There have a hsla_to_rgba method for this, it was converted before in vs_quad.

@huacnlee
Copy link
Contributor Author

Screenshot from 2024-11-21 00-28-12

@jansol Looks like you are right, I have to do a test, now is better.

@huacnlee
Copy link
Contributor Author

huacnlee commented Nov 21, 2024

Added to support cx.paint_path with gradient, we need this to draw chart.

image

This is apply gradient in our application:

image

And the performance is looks no difference, I have test in our application and Zed, the FPS is like before.

@huacnlee huacnlee force-pushed the fill-gradient branch 3 times, most recently from efa94f2 to 7011666 Compare November 21, 2024 05:53
@huacnlee huacnlee marked this pull request as draft November 21, 2024 08:21
@huacnlee huacnlee marked this pull request as ready for review November 21, 2024 09:18
@jansol
Copy link
Contributor

jansol commented Nov 21, 2024

@jansol Looks like you are right, I have to do a test, now is better.

Thanks. Now it indeed matches your CSS reference.

However now that I've had a moment to check out the code and the reference in more detail I'd like to argue that the CSS reference itself is... not great: it does the gradient interpolation in gamma-corrected sRGB color space. This the CSS default -- likely for historical reasons, since it is the simplest most naïve option so everyone used to go for it first -- but is generally considered a pretty bad option. With "red" and "blue" endpoints it doesn't look too terrible, but if the endpoints are e.g. "red" and "green" you get a very noticeable and very ugly darkening in the middle:

image

The technically more "correct" solution would be to tell CSS to do the interpolation with linearized sRGB values: gradient(45deg in srgb-linear, red, green):

image

Unfortunately this makes brightness gradients look awkward:

image

At this point the best option for gradients specifically appears to be doing interpolation in the Oklab colorspace (gradient(45 in in oklab, red, green)). This avoids the ugly inconsistencies of nonlinear sRGB as well as the awkward handling of brightness gradients (say, "white" to "black"):

image
image

Raph Levien has done a nice comparison of various colorspaces for the purpose of gradients in his blog.

Conversions between linear sRGB and oklab are quite straightforward and the exact matrices are given in the blog post that introduced the colorspace.

I guess ideally we'd have the option to choose the colorspace for interpolation, but as a default I think Oklab is the closest to what one would want & intuitively expect. It does have some shortcomings (chance of out of gamut colors -- need to clamp the resulting sRGB values to the 0..1 range after interpolation & conversion back to linear sRGB) but I don't think that's a huge issue in this case.

@huacnlee
Copy link
Contributor Author

WIP to fix blade render.

@huacnlee
Copy link
Contributor Author

Ready to review

@huacnlee
Copy link
Contributor Author

sRGB:

image

Oklab:

image

@huacnlee
Copy link
Contributor Author

huacnlee commented Nov 28, 2024

Em... oklab not same like CSS.

image

@huacnlee
Copy link
Contributor Author

image

@huacnlee
Copy link
Contributor Author

image

In Blade

@huacnlee
Copy link
Contributor Author

Ready to review

Copy link
Contributor

@jansol jansol left a comment

Choose a reason for hiding this comment

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

The colorspaces are a bit mixed up. If you create a nonlinear sRGB gradient and an oklab gradient, a well as a solid box side by side and set the exact same color for all 4 stops and the solid background, they should look perfectly identical.

Comment on lines 365 to 370
if (quad.background.tag == 0u) {
out.background_solid = hsla_to_rgba(quad.background.solid);
} else if (quad.background.tag == 1u) {
out.background_color0 = linear_to_srgba(hsla_to_rgba(quad.background.colors[0].color));
out.background_color1 = linear_to_srgba(hsla_to_rgba(quad.background.colors[1].color));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So of I'm reading this right the background is set to linear sRGB if it's solid but converted to non-linear sRGB for gradients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is a bit of difference like the Metal, the hsla_to_rgba it was returns linear sRGB.

I kept the behavior of this function unchanged to avoid modifying other parts of the code.

And to convert the color in vertex stage to reduce convert color again in every fragment for performance.


switch (background.color_space) {
default: {
background_color = srgba_to_linear(mix(color0, color1, t));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a gradient, colors are non-linear sRGB and converted to linear sRGB after mixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it several times here, it seems that the color value obtained by the previous algorithm should be sRGB, which needs to be converted to linear to get the correct color.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, okay, yeah I found the "previous algorithm", now it makes sense.

Comment on lines 321 to 324
let oklab_color0 = linear_srgb_to_oklab(color0);
let oklab_color1 = linear_srgb_to_oklab(color1);
let oklab_color = mix(oklab_color0, oklab_color1, t);
background_color = oklab_to_linear_srgb(oklab_color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a gradient, colors are non-linear sRGB but they are converted to oklab as if they were linear sRGB?

@huacnlee huacnlee requested a review from jansol November 29, 2024 03:10
Copy link
Contributor

@jansol jansol left a comment

Choose a reason for hiding this comment

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

Okay, read through the code again & verified the final output with a color picker and both the code and the resulting colors match what I would expect now.

Left a few more comments, but those are purely about cosmetics & future ease of maintaining - feel free to address them or ignore them. As far as I'm concerned this is good to go as it is.

crates/gpui/src/platform/blade/shaders.wgsl Outdated Show resolved Hide resolved
crates/gpui/src/platform/blade/shaders.wgsl Outdated Show resolved Hide resolved
crates/gpui/src/platform/mac/shaders.metal Outdated Show resolved Hide resolved
crates/gpui/src/platform/mac/shaders.metal Outdated Show resolved Hide resolved
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Great, thank you for the review and fixes.

Just in case, let's merge it after the next release this Wednesday, to have more runaway for testing this.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Dec 10, 2024
@SomeoneToIgnore SomeoneToIgnore merged commit de89f8c into zed-industries:main Dec 11, 2024
14 checks passed
@huacnlee huacnlee deleted the fill-gradient branch December 12, 2024 02:31
@iamnbutler
Copy link
Member

iamnbutler commented Dec 12, 2024

Nice! I missed that this landed. Excited to try this out. We actually will use this in the upcoming git ui, so it is just in time! Thanks for adding it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants