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

fix bug: correct the blend function in DX12 #3447

Closed
wants to merge 1 commit into from
Closed

Conversation

Ubpa
Copy link

@Ubpa Ubpa commented Sep 1, 2020

formula

  1. Result_alpha = Src_alpha + Dst_alpha * (1 - Src_alpha)
  2. Result_rgb = Result_color * Result_alpha = Src_color * Src_alpha + Dst_color * Dst_alpha * (1 - Src_alpha)

assume

  1. Dst_rgb = Dst_color * Dst_alpha
  2. texture (like fonts) is (Src_color, Src_alpha), not premultiplied alpha, so Src_rgb = Src_color

then

  1. Result_alpha = Src_alpha + Dst_alpha * (1 - Src_alpha)
  2. Result_rgb = Result_color * Result_alpha = Src_rgb * Src_alpha + Dst_rgb * (1 - Src_alpha)

so

  1. SrcBlendAlpha = D3D12_BLEND_ONE
  2. DestBlendAlpha = D3D12_BLEND_INV_SRC_ALPHA

Why nobody found this bug? Because Dst_alpha is useless In the general case (only use Src_alpha, see SrcBlend and DestBlend)
I found this bug when I use two context, one for game, one for editor, and the error alpha in game context exposed in editor context.

Here I show a demo (there are two contexts, one in game window)

error result (text disappeared)
CWM4(W2 SV9YR{`%0(CG L3

correct result
{BRG~JRTKZFV_PMK5LYM}I9

if fonts texture is premultiplied alpha, the blend mode should set to
SrcBlend = D3D12_BLEND_ONE

formula
1. Result_alpha = Src_alpha + Dst_alpha * (1 - Src_alpha)
2. Result_rgb = Result_color * Result_alpha = Src_color * Src_alpha + Dst_color * Dst_alpha * (1 - Src_alpha)

assume
1. Dst_rgb = Dst_color * Dst_alpha
2. texture (like fonts) is (Src_color, Src_alpha), not premultiplied alpha, so Src_rgb = Src_color

then
1. Result_alpha = Src_alpha + Dst_alpha * (1 - Src_alpha)
2. Result_rgb = Result_color * Result_alpha = Src_rgb * Src_alpha + Dst_rgb * (1 - Src_alpha)

so
1. SrcBlendAlpha = D3D12_BLEND_ONE
2. DestBlendAlpha = D3D12_BLEND_INV_SRC_ALPHA
@ocornut
Copy link
Owner

ocornut commented Oct 20, 2020

Thank you for this PR.
Also linking to #2873, #2764, #2693

@@ -552,8 +553,8 @@ bool ImGui_ImplDX12_CreateDeviceObjects()
desc.RenderTarget[0].SrcBlend = D3D12_BLEND_SRC_ALPHA;
desc.RenderTarget[0].DestBlend = D3D12_BLEND_INV_SRC_ALPHA;
desc.RenderTarget[0].BlendOp = D3D12_BLEND_OP_ADD;
desc.RenderTarget[0].SrcBlendAlpha = D3D12_BLEND_INV_SRC_ALPHA;
desc.RenderTarget[0].DestBlendAlpha = D3D12_BLEND_ZERO;
desc.RenderTarget[0].SrcBlendAlpha = D3D12_BLEND_ONE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I intended to write that D3D12_BLEND_SRC_ALPHA would be consistent with other backends, but...

It appears backends has different idea how to blend alpha channel into final image:

Backend Alpha Channel Blend Equation
D3D9 src * SRC_ALPHA + dest * INV_SRC_ALPHA
D3D10 src * INV_SRC_ALPHA + dest * 0
D3D11 src * INV_SRC_ALPHA + dest * 0
D3D12 src * 1 + dest * INV_SRC_ALPHA
OpenGL src * SRC_ALPHA + dest * INV_SRC_ALPHA
Metal src * SRC_ALPHA + dest * INV_SRC_ALPHA
Vulkan src * INV_SRC_ALPHA + dest * 0
Allegro src * SRC_ALPHA + dest * INV_SRC_ALPHA

I guess it does not matter until ImGui is rendered into the texture.
What is desired equation?

Copy link
Author

Choose a reason for hiding this comment

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

result alpha = 1 - result transparency
result transparency = src transparency * dest transparency
src transparency = 1 - src alpha
dest transparency = 1 - dst alpha

so
result alpha = src alpha * 1 + dest alpha * (1 - src alpha)

D3D12 is correct

ocornut added a commit that referenced this pull request Feb 18, 2021
… accross all backends. (#2693, #2764, #2766, #2873, #3447, #3813, #3816)

Some of the viewport ideas from #2766 are not in there yet (e.g. Vulkan: setting compositeAlpha based on cap.supportedCompositeAlpha)
@ocornut
Copy link
Owner

ocornut commented Feb 18, 2021

Thank you @Ubpa and sorry for late answer.
We've now merged this and applied similar changes to multiple backends and examples, see bda12e5 and 6a161b8.
Some remaining related work will be discussed in #2766

@ocornut ocornut closed this Feb 18, 2021
ocornut added a commit that referenced this pull request Feb 18, 2021
…r. (#2766, #3447)

Pragma linking with dwmapi.lib (Vista-era, ~9 kb). MinGW users will need to link with -ldwmapi.
erosnick added a commit to erosnick/DirectX12FromScratch that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants