-
Notifications
You must be signed in to change notification settings - Fork 422
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
Use premultiplied colours in rendering #6456
base: master
Are you sure you want to change the base?
Conversation
Also adds `UniformColourInfo` for border colour uniform, instead of the gigantic definition that was in `Renderer`.
afd49bc
to
6f10e23
Compare
@@ -34,7 +35,7 @@ protected override Image<TPixel> ImageFromStream<TPixel>(Stream stream) | |||
} | |||
|
|||
bitmap.Recycle(); | |||
return Image.LoadPixelData<TPixel>(result, bitmap.Width, bitmap.Height); | |||
return PremultipliedImage.FromPremultiplied(Image.LoadPixelData<Rgba32>(result, bitmap.Width, bitmap.Height)); |
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.
According to https://developer.android.com/reference/android/graphics/BitmapFactory.Options#inPremultiplied which states that it is enabled by default, this line should be correct (cc @Susko3 if you can confirm this).
Fixes the volume meters issue in ppy/osu#31129 (comment)
My primary concern reading through these changes is that the alpha channel is still present in some parts of the code. For example... TestSceneDrawablePath.cs In these two, the alpha channel is still present in the image itself, even though When constructing the premult colour, we're still taking on the source alpha channel which is then being passed to the shader. i.e. a colour Am I misunderstanding something here? |
/// <summary> | ||
/// The <see cref="Color4"/> after alpha multiplication. | ||
/// </summary> | ||
public readonly Color4 Premultiplied; |
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.
Referring to my prior comment, #6456 (comment), it might be more intuitive to decompose this into RGB and a private float A=1
. Unless it becomes annoying to handle raw components in usages as opposed to a Color4
bundle.
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.
Sounds good, I was under the thought that any colour value that is represented by red-green-blue-alpha channels should be bundled in a Color4
, but I can agree with the argument that premult colours do not fall within that category particularly.
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.
I'm not entirely sure about making the alpha component private, it's still an effective value of the colour and needs to be exposed for UniformColour
to turn it to a Vector4
for shaders. The alpha component in premultiplied colours is referred to as the level of "occlusion" in the "Alpha compositing" wikipedia page:
I've went with that name for the alpha component field just to disassociate it from the name "Alpha" which we commonly recognize as the opacity of a pixel rather than "occlusion". Up for discussion.
Reading through more, I am guessing this is just my misunderstanding because the blending is Appears to make sense as it is? |
@@ -72,7 +72,7 @@ public struct BlendingParameters : IEquatable<BlendingParameters> | |||
|
|||
public static BlendingParameters Mixture => new BlendingParameters | |||
{ | |||
Source = BlendingType.SrcAlpha, | |||
Source = BlendingType.One, | |||
Destination = BlendingType.OneMinusSrcAlpha, | |||
SourceAlpha = BlendingType.One, | |||
DestinationAlpha = BlendingType.One, |
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.
As a side-node, I'm pretty sure this is incorrect and the alpha equation should be (1, 1-sA)
. That said, this is also incorrect on master but it may be what's causing your colour issues in ppy/osu#31129 (comment) ?
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.
It does not seem to have any effect on the issue (I've ensured the inherited property is also updated to OneMinusSrcAlpha
). That being said, I don't see any differences in game so the change can happen here.
I did not notice that the mixing algorithm included alpha so that's why I was wondering why you mentioned DestinationAlpha
being wrong in #3429. But now that I search again, from the very definition of the mixing algorithm (Porter–Duff's over operator) it does seem to be correct.
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.
Applied to Mixture
blending in dc00efe. Also added a test scene that can be ran to show blending differences between master and this PR with the fix applied.
After some self ranting and figuring out whether this change should be applied to Additive
blending as well, I've settled with a weak no because I cannot find any reference source to compare against, and the case where this matters is very specific (see 1ff2b65#diff-d2aa04480a7566b4c82977db2d05575222822b78e79807a9b3c607fe374fc254R288-R293).
In the usages that directly modify Alternatively I can provide an indexer from - raw.Premultiplied[i, 0] = new Rgba32(r * a, g * a, b * a, a);
+ raw[i, 0] = PremultipliedColour.FromStraight(new Color4(r, g, b, a)); And technically you can still pass down premultiplied colours if you wish: - raw.Premultiplied[i, 0] = new Rgba32(r * a, g * a, b * a, a);
+ raw[i, 0] = PremultipliedColour.FromPremultiplied(new Color4(r * a, g * a, b * a, a)); I think that might be a better approach as we don't want to expose the framework codebase to the idea that colours can be premultiplied, and leave it as an internal detail that is done right before the shader reads said colours.
Worthy of note that
If you use |
@smoogipoo To solidify the API, I've removed access to the underlying image of |
This reverts commit 8a4abb3.
59493e3 is pushed as colours with alpha>1 can be used to give strong glow effects. See https://discord.com/channels/188630481301012481/589331078574112768/1320398450457841754. This is also the premise of how |
RFC, cc @ppy/team-client.
This will be an extremely breaking change to all framework consumers, I will write down breaking notes for it after we all agree with the direction.
The efforts of this started when I noticed that iOS always outputs premultiplied image pixels when loading them via
UIImage
. Searching on the internet for methods to avoid this behaviour has shown nothing, and after opening a PR manually "fixing" this, #6454 (comment) came up, which shown that we can take a different route and switch permanently to premultiplied colours for textures and shaders, as discussed.Consider this an initial approach meant to stir discussions on the introduced API and intended direction.
Multiple classes have been introduced to emphasize on the concept of "premultiplied colours" in rendering, most of which are one-way conversion, meaning that a regular (aka straight-alpha) colour transformed into a premultiplied-alpha colour cannot be transformed back, as the conversion is technically lossy when given zero alpha, and I generally want to avoid implicit conversions from/to
Color4
as much as possible.There is not much to note about the changes, but, as a breakdown of introduced classes:
PremultipliedImage
: Exposes an image with alpha multiplication applied. The image may originally be in straight-alpha form converted usingFromStraight
, or already premultiplied (aka iOS) and wrapped in the structure usingFromPremultiplied
.PremultipliedColour
: Exposes aColor4
with alpha multiplication applied. Works similar toPremultipliedImage
.UniformColour
: Special version ofUniformVector4
for colours specifically, stores colours as premultiplied. Implicitly converts from/toPremultipliedColour
.UniformColourInfo
: This is introduced specifically for border colour specifications, as it was extremely too long and adding premultiplied API warranted this.TestSceneBlendingCorrectness
Inspired by #3429, this PR adds a test scene that shows blending comparisons between buffered and non-buffered rendering. Final result is achieved by operating using premultiplied colours and fixing alpha blending.