-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
ShaderGen: Always calculate lighting for both color channels #4601
Conversation
Fixes Cel-Damage. Is there anything else I should test? |
@@ -319,7 +319,7 @@ static void LightAlpha(const Vec3& pos, const Vec3& normal, u8 lightNum, const L | |||
|
|||
void TransformColor(const InputVertexData* src, OutputVertexData* dst) | |||
{ | |||
for (u32 chan = 0; chan < xfmem.numChan.numColorChans; chan++) | |||
for (u32 chan = 0; chan < 2; chan++) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
{ | ||
object.Write("{\n"); | ||
|
||
bool colormatsource = !!(uid_data.matsource & (1 << j)); | ||
if (colormatsource) // from vertex | ||
{ | ||
if (components & (VB_HAS_COL0 << j)) | ||
if (j < numColorChans && components & (VB_HAS_COL0 << j)) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Marking as WIP until I get a chance to figure out the regression in Mario Sunshine. |
Vegas Party Depth is the only real regression here. The Brawl characters are already incorrect. |
This should be regression free now. Also fixes the grey boxes in Super Mario Sunshine on the hardware backends, and Tony Hawk on the software backend. I'm still not sure about the default values, it would be nice to verify this with a hardware test, but I've spent way too much time on this PR already. Furthermore, I suspect we should be using the number of elements field in the vertex format (at least for the color channels) when processing the vertices, but this is a larger change, and as I mentioned in the comment the way I've done it here catches the known cases for now. |
LGTM. Should we merge this for this progress report or wait for the next? |
Can this be rebased? I'd like to test it on something. |
@@ -23,7 +23,6 @@ VertexShaderUid GetVertexShaderUid() | |||
memset(uid_data, 0, sizeof(*uid_data)); | |||
|
|||
_assert_(bpmem.genMode.numtexgens == xfmem.numTexGen.numTexGens); | |||
_assert_(bpmem.genMode.numcolchans == xfmem.numChan.numColorChans); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
ping? :> |
I ran this through fifo_comparer against the fifoci tests (using current master as a base) and the only issue was the smsGreyCubes2.dff (the cubes are missing) which occurs on all backends. If this can be rebased, I can try it again. |
@iwubcode Not rendering the gray cubes in Super Mario Sunshine is actually console-accurate. |
This only fixes things according to fifoci
…On Tue, Jun 20, 2017, 2:14 AM JosJuice ***@***.***> wrote:
@iwubcode <https://github.com/iwubcode> Not rendering the gray cubes in
Super Mario Sunshine is actually console-accurate.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4601 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGSuQUc6gsffPbfJqYN8QkcpHMq-LWkuks5sF2NWgaJpZM4LZmGT>
.
|
const PortableVertexDeclaration& vdec) | ||
{ | ||
bool use_color_1 = vdec.colors[0].enable && vdec.colors[1].enable; | ||
for (size_t i = 0; i < 2; i++) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Rebase? |
I've decided to split this PR into two, one to fix SMS and one to fix cel-damage. The SMS fix will come later, after additional hardware tests, but this PR (cel-damage fix) is good to go now. Confirmed on hardware that the lighting pipeline still occurs even when numColorChans is zero, and these colours are used for texture coordinate generation. If numColorChans is zero, these values are not sent from the vertex stage to the rasterizer. This is what the PR modifies, the behavior below is unchanged and left for a future PR. The next question: But do we need to emulate the exact hardware behavior here? Or can we get away with just setting the colour inputs to zero. |
out.Write("%sfor (uint chan = 0u; chan < xfmem_numColorChans; chan++) {\n", | ||
api_type == APIType::D3D ? "[loop] " : ""); | ||
out.Write("%sfor (uint chan = 0u; chan < %uu; chan++) {\n", | ||
api_type == APIType::D3D ? "[loop] " : "", static_cast<u32>(NUM_XF_COLOR_CHANNELS)); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Cel-damage uses the color from the lighting stage of the vertex pipeline as texture coordinates, but sets numColorChans to zero. We now calculate the colors in all cases, but override the color before writing it from the vertex shader if numColorChans is set to a lower value.
Fix Super Mario Sunshine debug cubes (originally PR #4601 by stenzek)
Fixes https://bugs.dolphin-emu.org/issues/12620 The changed code did not match the corresponding code in VertexShaderGen. Some parts of the sky have 2 color channels in each vertex, while others only have 1, despite only color channel 0 being used and XFMEM_SETNUMCHAN being set to 1 for both of them. The old code (from dolphin-emu#4601) caused channel 0 to be set to channel 1 if the vertex contained both color channels but the number of channels was set to 1, which is wrong.
Fixes https://bugs.dolphin-emu.org/issues/12620 The changed code did not match the corresponding code in VertexShaderGen. Some parts of the sky have 2 color channels in each vertex, while others only have 1, despite only color channel 0 being used and XFMEM_SETNUMCHAN being set to 1 for both of them. The old code (from dolphin-emu#4601) caused channel 0 to be set to channel 1 if the vertex contained both color channels but the number of channels was set to 1, which is wrong.
Fixes https://bugs.dolphin-emu.org/issues/12620 The changed code did not match the corresponding code in VertexShaderGen. Some parts of the sky have 2 color channels in each vertex, while others only have 1, despite only color channel 0 being used and XFMEM_SETNUMCHAN being set to 1 for both of them. The old code (from dolphin-emu#4601) caused channel 0 to be set to channel 1 if the vertex contained both color channels but the number of channels was set to 1, which is wrong.
Up until now, we assumed that the XFMEM_SETNUMCHAN register controlled how many channels the lighting pipeline processes. If this was set to zero, no lighting was calculated at all, and the colors were passed through as-is from the corresponding attribute.
Cel-Damage is an interesting case, as it doesn't module the vertex color with the texture result as you would expect. Instead, it uses two sets of texture coordinates. The first reference a diffuse texture, as usual. The second set of texture coordinates, however, are derived from the vertex color. This set indexes a LUT-like texture, which is blended with the first texture result in a second TEV stage, to produce the lit pixels.
As we were setting the vertex color to white before calculating texture coordinates, this resulted in the same texel (at normalized coordinates 1,1) being used from the LUT texture, and flat, brightly-colored polygons. The software rasterizer used black as a "default" color, resulting in flat, dark-colored polygons.
This PR changes the shader generation to always process the lighting pipeline, and only "clear" the colors based on XFMEM_SETNUMCHAN after texture coordinate generation, fixing the lighting in Cel-Damage.
I've tested this behavior on hardware by using the lighting pipeline's color as texture coordinates, as cel-damage does, and comparing results when XFMEM_SETNUMCHAN is set to zero and one. No change is visible, confirming that the lighting pipeline does indeed run regardless of this register's value.
This opens up the door to further questions though, see my comment below for more info. However, to keep things simple, this change is self-contained, fixes cel-damage, and has no other regressions in fifoci.