Move gamma correction logic from renderer/shader to vertex input #5721

frenzibyte · 2023-03-30T00:47:48Z

Closes SetPanelBackground's gradient appears lighter on veldrid renderer osu#22990
Alternative to / closes Fix gamma blending in new renderers #5713

We've generally been passing colours to the shaders in linear space, and convert the colours within the shader into sRGB space.

In Veldrid, this broke colour blending on buffered containers, as can be seen here:

(on the right is a box split into 50% and 100% opacity of the colour #FF6633, and on the left is the same but wrapped in a buffered container)

This breakage is essentially happening due to rendering happening in two stages and both the GPU driver and shader are incorrectly converting colour between sRGB and linear space until it ended up rendering the buffered container with a broken blend.

Rather than going through this fiasco, I've stopped both the renderer and the shader from ever applying any colour conversion in the shader, and I've instead let the framework pass colours to the shader in sRGB format.

The only difference this PR resulted in is ColourInfo.Gradient{Horizontal,Vertical} interpolating in sRGB space (gamma-incorrect), making it stand different from the other samples in the test scene:

But this matches both browser, and figma as well:

So we're pretty much golden. I've updated the test scene to point as such. We can also consider replacing Interpolation.ValueAt(Colour4) with two different versions specifying in which space colours are being blended in, but I'll leave that for a follow-up PR since this one is quite stuffed.

This caused the triangles button in osu! to render with different dark colours.

peppy

Initial pass looks pretty sane.

peppy · 2023-03-30T11:43:14Z

osu.Framework/Graphics/Colour/SRGBColour.cs

@@ -59,8 +72,8 @@ public struct SRGBColour : IEquatable<SRGBColour>
                first.Linear.A + second.Linear.A),
        };

-        public readonly Vector4 ToVector() => new Vector4(Linear.R, Linear.G, Linear.B, Linear.A);
-        public static SRGBColour FromVector(Vector4 v) => new SRGBColour { Linear = new Color4(v.X, v.Y, v.Z, v.W) };
+        public readonly Vector4 ToVector() => new Vector4(SRGB.R, SRGB.G, SRGB.B, SRGB.A);


The code quality failures here are likely worth considering. At very least we'd want to reduce the copy count to one by moving the property access to a local field, or at best avoid it being a computed property in the first place.

Suggested change

public readonly Vector4 ToVector() => new Vector4(SRGB.R, SRGB.G, SRGB.B, SRGB.A);

public readonly Vector4 ToVector()

{

var linear = Linear;

return new Vector4(linear.R, linear.G, linear.B, linear.A);

}

Although I don't see this inspection locally so I'm not sure where it's coming from..

It appears to come from .NET's own code style analysis. I've flipped SRGBColour storage since we're using SRGB much more than we use Linear now.

since we're using SRGB much more than we use Linear now.

I'm not necessarily opposed to inverting, but this conclusion is completely non-obvious to me. ApplyChild() uses multiplication ops which now end up calling ToLinear(). That will be done by every drawable.

I guess it's more of which is prominent in the codebase now (in terms of direct usages). I didn't notice the operators were widely used though.

That said on second thought draw nodes are subject to state caching/invalidation, so the ApplyChild() calls should be 99% one-and-done init logic. So optimising for reads and avoiding .ToLinear() calls on the cpu (which inverting achieves) is probably what we want.

Also makes the struct less weird.

bdach · 2023-03-30T17:14:55Z

SampleGame/SampleGameGame.cs

I'm slightly weirded out that the display of SampleGame is being changed just for this pull. I'd have put the boxes in a test scene instead.

Uh, this was definitely not intended.

osu.Framework/Graphics/Colour/SRGBColour.cs

bdach · 2023-03-30T17:22:44Z

osu.Framework/Graphics/Colour/SRGBColour.cs

@@ -59,8 +72,8 @@ public struct SRGBColour : IEquatable<SRGBColour>
                first.Linear.A + second.Linear.A),
        };

-        public readonly Vector4 ToVector() => new Vector4(Linear.R, Linear.G, Linear.B, Linear.A);
-        public static SRGBColour FromVector(Vector4 v) => new SRGBColour { Linear = new Color4(v.X, v.Y, v.Z, v.W) };
+        public readonly Vector4 ToVector() => new Vector4(SRGB.R, SRGB.G, SRGB.B, SRGB.A);


since we're using SRGB much more than we use Linear now.

I'm not necessarily opposed to inverting, but this conclusion is completely non-obvious to me. ApplyChild() uses multiplication ops which now end up calling ToLinear(). That will be done by every drawable.

bdach · 2023-03-30T17:26:31Z

osu.Framework/Graphics/Rendering/GlobalUniformData.cs

        public UniformBool BackbufferDraw;
-        private readonly UniformPadding8 pad1;
+        private readonly UniformPadding12 pad1;


We have padding here and padding at the end of the global uniform block that comes out to 16 bytes total. Could/should we rearrange members here to consolidate the padding like the other pull did?

Sure, sounds good.

Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>

bdach · 2023-03-30T18:19:21Z

Testing reference:

	ground truth (legacy opengl on master @ windows)	this PR (dx11 @ windows)	this PR (legacy opengl @ android)
`BorderColour` (masking, colour fill, border fill)
`BorderSmoothing`
`ColourGradient`
`Effects` (blur, shadow)
The boxes
`HSVColourPicker`
`Video`
`WrapModes` (texture sampling)

TL;DR: We were mixing and matching blending before. It appears that buffered containers did gamma-incorrect blending, while Colour/BorderColour/border effects/blur shader did gamma-correct. So the last ones will change in this pull, but they will change to gamma-incorrect, which is what we want anyway to match browsers and figma designs.

osu.Framework.Tests/Visual/Drawables/TestSceneGammaCorrection.cs

Co-authored-by: Susko3 <Susko3@protonmail.com>

frenzibyte added 7 commits March 28, 2023 22:02

Make sense of SRGBColours

63652b1

Disable GPU-level colour space conversion

cb7eb83

Disable shader-level colour space conversion

801f813

Convert shader input colours to sRGB space

9afb576

Fix colour info interpolation happening in linear space

fc8f029

This caused the triangles button in osu! to render with different dark colours.

Work around blurring broken on Metal

61b408c

Update colour interpolation test scene to match expectations

a636a3b

frenzibyte added the area:graphics label Mar 30, 2023

frenzibyte requested a review from a team March 30, 2023 00:47

pull-request-size bot added the size/L label Mar 30, 2023

frenzibyte mentioned this pull request Mar 30, 2023

Update logo animation shader inline with framework changes ppy/osu-resources#252

Merged

peppy self-requested a review March 30, 2023 06:57

peppy reviewed Mar 30, 2023

View reviewed changes

bdach assigned frenzibyte Mar 30, 2023

Flip SRGBColour storage to avoid implicit copies

afa5ebc

Also makes the struct less weird.

bdach reviewed Mar 30, 2023

View reviewed changes

Update SRGBColour documentation

d8c4a2b

Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>

frenzibyte added 2 commits March 30, 2023 21:34

Revert sample game changes and include colour blending samples

33d62a8

Simplify global uniform data padding

7d6dd85

pull-request-size bot added size/XL and removed size/L labels Mar 30, 2023

Enable NRT on test scene

c72eb3e

Susko3 suggested changes Mar 30, 2023

View reviewed changes

osu.Framework.Tests/Visual/Drawables/TestSceneGammaCorrection.cs Outdated Show resolved Hide resolved

osu.Framework.Tests/Visual/Drawables/TestSceneGammaCorrection.cs Outdated Show resolved Hide resolved

Fix wrong text labels from old test scene

4767b94

Co-authored-by: Susko3 <Susko3@protonmail.com>

bdach added the next-release Pull requests which are almost there label Mar 30, 2023

Merge branch 'master' into fix-srgb-conversion

fbcfbc6

peppy approved these changes Mar 31, 2023

View reviewed changes

smoogipoo approved these changes Mar 31, 2023

View reviewed changes

smoogipoo enabled auto-merge March 31, 2023 11:22

smoogipoo and others added 2 commits March 31, 2023 20:22

Merge branch 'master' into fix-srgb-conversion

2703f0a

Merge branch 'master' into fix-srgb-conversion

e503031

smoogipoo merged commit 22d7443 into ppy:master Mar 31, 2023
12 checks passed

frenzibyte deleted the fix-srgb-conversion branch March 31, 2023 15:47

bdach mentioned this pull request Apr 1, 2023

Buffered containers render with wrong colours on Android #5694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move gamma correction logic from renderer/shader to vertex input #5721

Move gamma correction logic from renderer/shader to vertex input #5721

frenzibyte commented Mar 30, 2023

peppy left a comment

peppy Mar 30, 2023

frenzibyte Mar 30, 2023

bdach Mar 30, 2023

frenzibyte Mar 30, 2023

bdach Mar 30, 2023 •

edited

bdach Mar 30, 2023

frenzibyte Mar 30, 2023

bdach Mar 30, 2023

bdach Mar 30, 2023

frenzibyte Mar 30, 2023

bdach commented Mar 30, 2023 •

edited

Move gamma correction logic from renderer/shader to vertex input #5721

Move gamma correction logic from renderer/shader to vertex input #5721

Conversation

frenzibyte commented Mar 30, 2023

peppy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Mar 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Mar 30, 2023 • edited

bdach Mar 30, 2023 •

edited

bdach commented Mar 30, 2023 •

edited