From c3668e179c28dbe769f8a128e780a2269044f962 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Sat, 10 Apr 2021 17:41:06 -0700 Subject: Split TevStageIndirect::mid into matrix_index and matrix_id --- Source/Core/VideoCommon/PixelShaderGen.cpp | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) (limited to 'Source/Core/VideoCommon/PixelShaderGen.cpp') diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index ab59bfd52a..866b7bacc8 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -991,7 +991,7 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i // TODO: Should we reset alphabump to 0 here? } - if (tevind.mid != 0) + if (tevind.matrix_index != IndMtxIndex::Off) { // format static constexpr std::array tev_ind_fmt_mask{ @@ -1038,11 +1038,14 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i tev_ind_bias_add[u32(tevind.fmt.Value())]); } + // Multiplied by 2 because each matrix has two rows. + // Note also that the 4th column of the matrix contains the scale factor. + const u32 mtxidx = 2 * (static_cast(tevind.matrix_index.Value()) - 1); + // multiply by offset matrix and scale - calculations are likely to overflow badly, // yet it works out since we only care about the lower 23 bits (+1 sign bit) of the result - if (tevind.mid <= 3) + if (tevind.matrix_id == IndMtxId::Indirect) { - const u32 mtxidx = 2 * (tevind.mid - 1); out.SetConstantsUsed(C_INDTEXMTX + mtxidx, C_INDTEXMTX + mtxidx); out.Write("\tint2 indtevtrans{} = int2(idot(" I_INDTEXMTX @@ -1064,10 +1067,9 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i out.Write("\telse indtevtrans{} <<= (-" I_INDTEXMTX "[{}].w);\n", n, mtxidx); } } - else if (tevind.mid <= 7 && has_tex_coord) - { // s matrix - ASSERT(tevind.mid >= 5); - const u32 mtxidx = 2 * (tevind.mid - 5); + else if (tevind.matrix_id == IndMtxId::S) + { + ASSERT(has_tex_coord); out.SetConstantsUsed(C_INDTEXMTX + mtxidx, C_INDTEXMTX + mtxidx); out.Write("\tint2 indtevtrans{} = int2(fixpoint_uv{} * iindtevcrd{}.xx) >> 8;\n", n, @@ -1086,10 +1088,9 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i out.Write("\telse indtevtrans{} <<= (-" I_INDTEXMTX "[{}].w);\n", n, mtxidx); } } - else if (tevind.mid <= 11 && has_tex_coord) - { // t matrix - ASSERT(tevind.mid >= 9); - const u32 mtxidx = 2 * (tevind.mid - 9); + else if (tevind.matrix_id == IndMtxId::T) + { + ASSERT(has_tex_coord); out.SetConstantsUsed(C_INDTEXMTX + mtxidx, C_INDTEXMTX + mtxidx); out.Write("\tint2 indtevtrans{} = int2(fixpoint_uv{} * iindtevcrd{}.yy) >> 8;\n", n, @@ -1112,11 +1113,14 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i else { out.Write("\tint2 indtevtrans{} = int2(0, 0);\n", n); + ASSERT(false); // Unknown value for matrix_id } } else { out.Write("\tint2 indtevtrans{} = int2(0, 0);\n", n); + // If matrix_index is Off (0), matrix_id should be Indirect (0) + ASSERT(tevind.matrix_id == IndMtxId::Indirect); } // --------- -- cgit v1.2.3 From 002ff4e4dd594c75898df9ba5ee4a14bc8fb7f77 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Sat, 17 Apr 2021 09:57:16 -0700 Subject: PixelShaderGen: Remove unused num_texgens argument It became unused in f039149198657c1891e1c6462ed30c31ed4b8486. --- Source/Core/VideoCommon/PixelShaderGen.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'Source/Core/VideoCommon/PixelShaderGen.cpp') diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index 866b7bacc8..6cda21b41b 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -361,7 +361,7 @@ void ClearUnusedPixelShaderUidBits(APIType api_type, const ShaderHostConfig& hos uid_data->bounding_box &= host_config.bounding_box & host_config.backend_bbox; } -void WritePixelShaderCommonHeader(ShaderCode& out, APIType api_type, u32 num_texgens, +void WritePixelShaderCommonHeader(ShaderCode& out, APIType api_type, const ShaderHostConfig& host_config, bool bounding_box) { // dot product for integer vectors @@ -546,8 +546,7 @@ ShaderCode GeneratePixelShaderCode(APIType api_type, const ShaderHostConfig& hos uid_data->genMode_numtexgens, uid_data->genMode_numindstages); // Stuff that is shared between ubershaders and pixelgen. - WritePixelShaderCommonHeader(out, api_type, uid_data->genMode_numtexgens, host_config, - uid_data->bounding_box); + WritePixelShaderCommonHeader(out, api_type, host_config, uid_data->bounding_box); if (uid_data->forced_early_z && g_ActiveConfig.backend_info.bSupportsEarlyZ) { -- cgit v1.2.3 From f6cf85a8bca13d3e8a075f2f823a142b7b9d115d Mon Sep 17 00:00:00 2001 From: Tillmann Karras Date: Mon, 5 Aug 2019 02:18:37 +0100 Subject: PixelShaderGen: Fix OOB tex coord indices Previously we set the texture coordinate to zero, now we set the texture coordinate *index* to zero. This fixes the ripple effect of the Mario painting in Luigi's Mansion. Co-authored-by: Pokechu22 --- Source/Core/VideoCommon/PixelShaderGen.cpp | 56 +++++++++++++++--------------- 1 file changed, 28 insertions(+), 28 deletions(-) (limited to 'Source/Core/VideoCommon/PixelShaderGen.cpp') diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index 6cda21b41b..b59cf04ebb 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -238,14 +238,9 @@ PixelShaderUid GetPixelShaderUid() for (unsigned int n = 0; n < numStages; n++) { - int texcoord = bpmem.tevorders[n / 2].getTexCoord(n & 1); - bool bHasTexCoord = (u32)texcoord < bpmem.genMode.numtexgens; - // HACK to handle cases where the tex gen is not enabled - if (!bHasTexCoord) - texcoord = bpmem.genMode.numtexgens; + uid_data->stagehash[n].tevorders_texcoord = bpmem.tevorders[n / 2].getTexCoord(n & 1); uid_data->stagehash[n].hasindstage = bpmem.tevind[n].bt < bpmem.genMode.numindstages; - uid_data->stagehash[n].tevorders_texcoord = texcoord; if (uid_data->stagehash[n].hasindstage) uid_data->stagehash[n].tevind = bpmem.tevind[n].hex; @@ -774,9 +769,11 @@ ShaderCode GeneratePixelShaderCode(APIType api_type, const ShaderHostConfig& hos out.Write("col1 = float4(0.0, 0.0, 0.0, 0.0);\n"); } - // HACK to handle cases where the tex gen is not enabled if (uid_data->genMode_numtexgens == 0) { + // TODO: This is a hack to ensure that shaders still compile when setting out of bounds tex + // coord indices to 0. Ideally, it shouldn't exist at all, but the exact behavior hasn't been + // tested. out.Write("\tint2 fixpoint_uv0 = int2(0, 0);\n\n"); } else @@ -795,19 +792,19 @@ ShaderCode GeneratePixelShaderCode(APIType api_type, const ShaderHostConfig& hos { if ((uid_data->nIndirectStagesUsed & (1U << i)) != 0) { - const u32 texcoord = uid_data->GetTevindirefCoord(i); + u32 texcoord = uid_data->GetTevindirefCoord(i); const u32 texmap = uid_data->GetTevindirefMap(i); - if (texcoord < uid_data->genMode_numtexgens) - { - out.SetConstantsUsed(C_INDTEXSCALE + i / 2, C_INDTEXSCALE + i / 2); - out.Write("\ttempcoord = fixpoint_uv{} >> " I_INDTEXSCALE "[{}].{};\n", texcoord, i / 2, - (i & 1) != 0 ? "zw" : "xy"); - } - else - { - out.Write("\ttempcoord = int2(0, 0);\n"); - } + // Quirk: when the tex coord is not less than the number of tex gens (i.e. the tex coord does + // not exist), then tex coord 0 is used (though sometimes glitchy effects happen on console). + // This affects the Mario portrait in Luigi's Mansion, where the developers forgot to set + // the number of tex gens to 2 (bug 11462). + if (texcoord >= uid_data->genMode_numtexgens) + texcoord = 0; + + out.SetConstantsUsed(C_INDTEXSCALE + i / 2, C_INDTEXSCALE + i / 2); + out.Write("\ttempcoord = fixpoint_uv{} >> " I_INDTEXSCALE "[{}].{};\n", texcoord, i / 2, + (i & 1) ? "zw" : "xy"); out.Write("\tint3 iindtex{} = ", i); SampleTexture(out, "float2(tempcoord)", "abg", texmap, stereo, api_type); @@ -949,7 +946,8 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i const auto& stage = uid_data->stagehash[n]; out.Write("\n\t// TEV stage {}\n", n); - // HACK to handle cases where the tex gen is not enabled + // Quirk: when the tex coord is not less than the number of tex gens (i.e. the tex coord does not + // exist), then tex coord 0 is used (though sometimes glitchy effects happen on console). u32 texcoord = stage.tevorders_texcoord; const bool has_tex_coord = texcoord < uid_data->genMode_numtexgens; if (!has_tex_coord) @@ -1169,6 +1167,10 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i // Emulate s24 overflows out.Write("\ttevcoord.xy = (tevcoord.xy << 8) >> 8;\n"); } + else + { + out.Write("\ttevcoord.xy = fixpoint_uv{};\n", texcoord); + } TevStageCombiner::ColorCombiner cc; TevStageCombiner::AlphaCombiner ac; @@ -1194,7 +1196,7 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i out.Write("\trastemp = {}.{};\n", tev_ras_table[u32(stage.tevorders_colorchan)], rasswap); } - if (stage.tevorders_enable) + if (stage.tevorders_enable && uid_data->genMode_numtexgens > 0) { // Generate swizzle string to represent the texture color channel swapping const char texswap[5] = { @@ -1205,17 +1207,15 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i '\0', }; - if (!stage.hasindstage) - { - // calc tevcord - if (has_tex_coord) - out.Write("\ttevcoord.xy = fixpoint_uv{};\n", texcoord); - else - out.Write("\ttevcoord.xy = int2(0, 0);\n"); - } out.Write("\ttextemp = "); SampleTexture(out, "float2(tevcoord.xy)", texswap, stage.tevorders_texmap, stereo, api_type); } + else if (uid_data->genMode_numtexgens == 0) + { + // It seems like the result is always black when no tex coords are enabled, but further testing + // is needed. + out.Write("\ttextemp = int4(0, 0, 0, 0);\n"); + } else { out.Write("\ttextemp = int4(255, 255, 255, 255);\n"); -- cgit v1.2.3 From b5844ab195a38303e5fb82a7f6804a726fa8fb7a Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Sun, 18 Apr 2021 16:12:21 -0700 Subject: PixelShaderGen: always run indirect stage logic Hardware testing has confirmed that fb_addprev and wrapping both run even when the indirect stage is disabled. --- Source/Core/VideoCommon/PixelShaderGen.cpp | 48 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 23 deletions(-) (limited to 'Source/Core/VideoCommon/PixelShaderGen.cpp') diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index b59cf04ebb..636bf2be7f 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -220,13 +220,10 @@ PixelShaderUid GetPixelShaderUid() // indirect texture map lookup int nIndirectStagesUsed = 0; - if (uid_data->genMode_numindstages > 0) + for (unsigned int i = 0; i < numStages; ++i) { - for (unsigned int i = 0; i < numStages; ++i) - { - if (bpmem.tevind[i].IsActive() && bpmem.tevind[i].bt < uid_data->genMode_numindstages) - nIndirectStagesUsed |= 1 << bpmem.tevind[i].bt; - } + if (bpmem.tevind[i].IsActive()) + nIndirectStagesUsed |= 1 << bpmem.tevind[i].bt; } uid_data->nIndirectStagesUsed = nIndirectStagesUsed; @@ -240,9 +237,12 @@ PixelShaderUid GetPixelShaderUid() { uid_data->stagehash[n].tevorders_texcoord = bpmem.tevorders[n / 2].getTexCoord(n & 1); + // hasindstage previously was used as a criterion to set tevind to 0, but there are variables in + // tevind that are used even if the indirect stage is disabled, so now it is only left in to + // avoid breaking existing UIDs (in most cases, games will have 0 in tevind anyways) + // TODO: Remove hasindstage on the next UID version bump uid_data->stagehash[n].hasindstage = bpmem.tevind[n].bt < bpmem.genMode.numindstages; - if (uid_data->stagehash[n].hasindstage) - uid_data->stagehash[n].tevind = bpmem.tevind[n].hex; + uid_data->stagehash[n].tevind = bpmem.tevind[n].hex; TevStageCombiner::ColorCombiner& cc = bpmem.combiners[n].colorC; TevStageCombiner::AlphaCombiner& ac = bpmem.combiners[n].alphaC; @@ -810,6 +810,16 @@ ShaderCode GeneratePixelShaderCode(APIType api_type, const ShaderHostConfig& hos SampleTexture(out, "float2(tempcoord)", "abg", texmap, stereo, api_type); } } + for (u32 i = uid_data->genMode_numindstages; i < 4; i++) + { + // Referencing a stage above the number of ind stages is undefined behavior, + // and on console produces a noise pattern (details unknown). + // TODO: This behavior is nowhere near that, but it ensures the shader still compiles. + if ((uid_data->nIndirectStagesUsed & (1U << i)) != 0) + { + out.Write("\tint3 iindtex{} = int3(0, 0, 0); // Undefined behavior on console\n", i); + } + } for (u32 i = 0; i < numStages; i++) { @@ -953,11 +963,8 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i if (!has_tex_coord) texcoord = 0; - if (stage.hasindstage) { - TevStageIndirect tevind; - tevind.hex = stage.tevind; - + const TevStageIndirect tevind{.hex = stage.tevind}; out.Write("\t// indirect op\n"); // Perform the indirect op on the incoming regular coordinates @@ -1124,9 +1131,8 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i // Wrapping // --------- - // TODO: Should the last element be 1 or (1<<7)? - static constexpr std::array tev_ind_wrap_start{ - "0", "(256<<7)", "(128<<7)", "(64<<7)", "(32<<7)", "(16<<7)", "1", + static constexpr std::array tev_ind_wrap_start{ + "(256<<7)", "(128<<7)", "(64<<7)", "(32<<7)", "(16<<7)", }; // wrap S @@ -1134,14 +1140,14 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i { out.Write("\twrappedcoord.x = fixpoint_uv{}.x;\n", texcoord); } - else if (tevind.sw == IndTexWrap::ITW_0) + else if (tevind.sw >= IndTexWrap::ITW_0) // 7 (Invalid) appears to behave the same as 6 (ITW_0) { out.Write("\twrappedcoord.x = 0;\n"); } else { out.Write("\twrappedcoord.x = fixpoint_uv{}.x & ({} - 1);\n", texcoord, - tev_ind_wrap_start[u32(tevind.sw.Value())]); + tev_ind_wrap_start[u32(tevind.sw.Value()) - u32(IndTexWrap::ITW_256)]); } // wrap T @@ -1149,14 +1155,14 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i { out.Write("\twrappedcoord.y = fixpoint_uv{}.y;\n", texcoord); } - else if (tevind.tw == IndTexWrap::ITW_0) + else if (tevind.tw >= IndTexWrap::ITW_0) // 7 (Invalid) appears to behave the same as 6 (ITW_0) { out.Write("\twrappedcoord.y = 0;\n"); } else { out.Write("\twrappedcoord.y = fixpoint_uv{}.y & ({} - 1);\n", texcoord, - tev_ind_wrap_start[u32(tevind.tw.Value())]); + tev_ind_wrap_start[u32(tevind.tw.Value()) - u32(IndTexWrap::ITW_256)]); } if (tevind.fb_addprev) // add previous tevcoord @@ -1167,10 +1173,6 @@ static void WriteStage(ShaderCode& out, const pixel_shader_uid_data* uid_data, i // Emulate s24 overflows out.Write("\ttevcoord.xy = (tevcoord.xy << 8) >> 8;\n"); } - else - { - out.Write("\ttevcoord.xy = fixpoint_uv{};\n", texcoord); - } TevStageCombiner::ColorCombiner cc; TevStageCombiner::AlphaCombiner ac; -- cgit v1.2.3