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

VideoCommon: Fix color channel logic when per-pixel lighting is in use #10164

Merged
merged 1 commit into from Oct 14, 2021

Conversation

Pokechu22
Copy link
Contributor

Fixes bug 12698.

This was broken in #10012 (specifically by 06579e4 and c3dec34) when I attempted to simplify the logic incorrectly.

For color channel 0, this is approximately the code that is generated prior to that PR:
  if ((uid_data->components & VB_HAS_COL0) != 0)
  {
    out.Write("vertex_color_0 = rawcolor0;\n");
  }
  else
  {
    out.Write("vertex_color_0 = missing_color_value;\n");
  }
  // roughly
  out.write("o.colors_0 = LightingShaderCode(vertex_color_0);");
  if (uid_data->numColorChans == 0)
  {
    if ((uid_data->components & VB_HAS_COL0) != 0)
      out.Write("o.colors_0 = rawcolor0;\n");
    else
      out.Write("o.colors_0 = float4(1.0, 1.0, 1.0, 1.0);\n");
  }

  if (per_pixel_lighting)
  {
    if ((uid_data->components & VB_HAS_COL0) != 0)
      out.Write("o.colors_0 = vertex_color_0;\n");
  }
  else
  {
    if (uid_data->numColorChans == 0)
      out.Write("o.colors_0 = float4(0.0, 0.0, 0.0, 0.0);\n");
  }

Here is a table of results:

chans==0 VB_HAS_COL0 per-pixel Result
Yes Yes Yes vertex_color_0 => rawcolor0
Yes Yes No float4(0, 0, 0, 0)
Yes No Yes float4(1, 1, 1, 1) - vertex_color_0 => missing_color_value would also work
Yes No No float4(0, 0, 0, 0)
No Yes Yes vertex_color_0 => rawcolor0
No Yes No LightingShaderCode(vertex_color_0) => LightingShaderCode(rawcolor0)
No No Yes LightingShaderCode(vertex_color_0) => LightingShaderCode(missing_color_value)
No No No LightingShaderCode(vertex_color_0) => LightingShaderCode(missing_color_value)
For color channel 0, this is approximately the code that is generated after that PR:
  if ((uid_data->components & VB_HAS_COL0) != 0)
  {
    out.Write("vertex_color_0 = rawcolor0;\n");
  }
  else
  {
    out.Write("vertex_color_0 = missing_color_value;\n");
  }
  // roughly
  out.write("o.colors_0 = LightingShaderCode(vertex_color_0);");
  if (uid_data->numColorChans == 0)
  {
    if ((uid_data->components & VB_HAS_COL0) != 0)
    {
      if (per_pixel_lighting)
        out.Write("o.colors_0 = vertex_color_0;\n");
      else
        out.Write("o.colors_0 = rawcolor0;\n");
    }
    else
    {
      out.Write("o.colors_0 = float4(0.0, 0.0, 0.0, 0.0);\n");
    }
  }

Here is a table of results:

chans==0 VB_HAS_COL0 per-pixel Result
Yes Yes Yes vertex_color_0 => rawcolor0
Yes Yes No rawcolor0
Yes No Yes float4(0, 0, 0, 0)
Yes No No float4(0, 0, 0, 0)
No Yes Yes LightingShaderCode(vertex_color_0) => LightingShaderCode(rawcolor0)
No Yes No LightingShaderCode(vertex_color_0) => LightingShaderCode(rawcolor0)
No No Yes LightingShaderCode(vertex_color_0) => LightingShaderCode(missing_color_value)
No No No LightingShaderCode(vertex_color_0) => LightingShaderCode(missing_color_value)

As you can see, the results disagree in several places.

For color channel 0, this is approximately the code that is generated with this PR:
  if ((uid_data->components & VB_HAS_COL0) != 0)
  {
    out.Write("vertex_color_0 = rawcolor0;\n");
  }
  else
  {
    out.Write("vertex_color_0 = missing_color_value;\n");
  }
  // roughly
  out.write("o.colors_0 = LightingShaderCode(vertex_color_0);");
  if (per_pixel_lighting) {
    out.Write("o.colors_0 = vertex_color_0;\n");
  } else if (uid_data->numColorChans == 0)
    out.Write("o.colors_0 = float4(0.0, 0.0, 0.0, 0.0);\n");
  }

Here is a table of results:

chans==0 VB_HAS_COL0 per-pixel Result
Yes Yes Yes vertex_color_0 => rawcolor0
Yes Yes No float4(0, 0, 0, 0)
Yes No Yes vertex_color_0 => missing_color_value
Yes No No float4(0, 0, 0, 0)
No Yes Yes vertex_color_0 => rawcolor0
No Yes No LightingShaderCode(vertex_color_0) => LightingShaderCode(rawcolor0)
No No Yes vertex_color_0 => missing_color_value
No No No LightingShaderCode(vertex_color_0) => LightingShaderCode(missing_color_value)

These also don't match the old behavior, but now the pixel shader is always getting the raw color, never a pre-lit value.

In addition to fixing the darkness in Xenoblade Chronicles, this also makes the debug cubes in Super Mario Sunshine have the same brightness with per-pixel lighting enabled as with it disabled. (This only shows up with the fifoplayer; missing_color_value is set to 0 by gameini when launching the game normally.) Here are images from both games, along with the CRC32 hashes of those images to more easily identify distinct results:

Test5.0-151055.0-15260This PR
Super Mario Sunshine, no per-pixel lighting 00000000_2021-10-13_15-45-46 s 5.0-15105 no ppl
2B87C475
00000000_2021-10-13_15-43-59 s 5.0-15260 no ppl
2B87C475
00000000_2021-10-13_15-48-24 s new no ppl
2B87C475
Super Mario Sunshine, with per-pixel lighting 00000000_2021-10-13_15-45-49 s 5.0-15105 with ppl
733708B3
00000000_2021-10-13_15-44-03 s 5.0-15260 with ppl
6DF359AE
00000000_2021-10-13_15-48-26 s new with ppl
49E8A160
Xenoblade, no per-pixel lighting 00000000_2021-10-13_15-46-24 x 5.0-15105 no ppl
378454C1
00000000_2021-10-13_15-46-57 x 5.0-15260 no ppl
378454C1
00000000_2021-10-13_15-49-07 x new no ppl
378454C1
Xenoblade, with per-pixel lighting 00000000_2021-10-13_15-46-28 x 5.0-15105 with ppl
6FC10472
00000000_2021-10-13_16-20-35 x 5.0-15260 with ppl
64A4CC02
00000000_2021-10-13_15-49-10 x new with ppl
6FC10472

Both specialized shaders and ubershaders give exactly identical results for all of these.

@JMC47 JMC47 merged commit 7d63933 into dolphin-emu:master Oct 14, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants