Merge pull request #9651 from Pokechu22/oob-texcoord

Fix out of bounds tex coord behavior; always apply fb_addprev and tex coord wrapping

Source/Core/VideoBackends/Software/Tev.cpp

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <cstring>
 
 #include "Common/ChunkFile.h"
 #include "Common/CommonTypes.h"
@@ -485,46 +486,46 @@ void Tev::Indirect(unsigned int stageNum, s32 s, s32 t)
 
   // matrix multiply - results might overflow, but we don't care since we only use the lower 24 bits
   // of the result.
-  const int indmtxid = indirect.mid & 3;
-  if (indmtxid)
+  if (indirect.matrix_index != IndMtxIndex::Off)
   {
-    const IND_MTX& indmtx = bpmem.indmtx[indmtxid - 1];
-    const int scale =
-        ((u32)indmtx.col0.s0 << 0) | ((u32)indmtx.col1.s1 << 2) | ((u32)indmtx.col2.s2 << 4);
+    const IND_MTX& indmtx = bpmem.indmtx[static_cast<u32>(indirect.matrix_index.Value()) - 1];
 
-    int shift;
+    const int shift = 17 - indmtx.GetScale();
 
-    switch (indirect.mid & 12)
+    switch (indirect.matrix_id)
     {
-    case 0:
+    case IndMtxId::Indirect:
       // matrix values are S0.10, output format is S17.7, so divide by 8
-      shift = (17 - scale);
       indtevtrans[0] = (indmtx.col0.ma * indcoord[0] + indmtx.col1.mc * indcoord[1] +
                         indmtx.col2.me * indcoord[2]) >>
                        3;
       indtevtrans[1] = (indmtx.col0.mb * indcoord[0] + indmtx.col1.md * indcoord[1] +
                         indmtx.col2.mf * indcoord[2]) >>
                        3;
       break;
-    case 4:  // s matrix
+    case IndMtxId::S:
       // s is S17.7, matrix elements are divided by 256, output is S17.7, so divide by 256. - TODO:
       // Maybe, since s is actually stored as S24, we should divide by 256*64?
-      shift = (17 - scale);
       indtevtrans[0] = s * indcoord[0] / 256;
       indtevtrans[1] = t * indcoord[0] / 256;
       break;
-    case 8:  // t matrix
-      shift = (17 - scale);
+    case IndMtxId::T:
       indtevtrans[0] = s * indcoord[1] / 256;
       indtevtrans[1] = t * indcoord[1] / 256;
       break;
     default:
+      PanicAlertFmt("Invalid indirect matrix ID {}", indirect.matrix_id);
       return;
     }
 
     indtevtrans[0] = shift >= 0 ? indtevtrans[0] >> shift : indtevtrans[0] << -shift;
     indtevtrans[1] = shift >= 0 ? indtevtrans[1] >> shift : indtevtrans[1] << -shift;
   }
+  else
+  {
+    // If matrix_index is Off (0), matrix_id should be Indirect (0)
+    ASSERT(indirect.matrix_id == IndMtxId::Indirect);
+  }
 
   if (indirect.fb_addprev)
   {
@@ -559,9 +560,16 @@ void Tev::Draw()
     const int stageNum2 = stageNum >> 1;
     const int stageOdd = stageNum & 1;
 
-    const u32 texcoordSel = bpmem.tevindref.getTexCoord(stageNum);
+    u32 texcoordSel = bpmem.tevindref.getTexCoord(stageNum);
     const u32 texmap = bpmem.tevindref.getTexMap(stageNum);
 
+    // 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 (texcoordSel >= bpmem.genMode.numtexgens)
+      texcoordSel = 0;
+
     const TEXSCALE& texscale = bpmem.texscale[stageNum2];
     const s32 scaleS = stageOdd ? texscale.ss1 : texscale.ss0;
     const s32 scaleT = stageOdd ? texscale.ts1 : texscale.ts0;
@@ -592,8 +600,13 @@ void Tev::Draw()
     const TevStageCombiner::ColorCombiner& cc = bpmem.combiners[stageNum].colorC;
     const TevStageCombiner::AlphaCombiner& ac = bpmem.combiners[stageNum].alphaC;
 
-    const int texcoordSel = order.getTexCoord(stageOdd);
-    const int texmap = order.getTexMap(stageOdd);
+    u32 texcoordSel = order.getTexCoord(stageOdd);
+    const u32 texmap = order.getTexMap(stageOdd);
+
+    // 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).
+    if (texcoordSel >= bpmem.genMode.numtexgens)
+      texcoordSel = 0;
 
     Indirect(stageNum, Uv[texcoordSel].s, Uv[texcoordSel].t);
 
@@ -603,8 +616,17 @@ void Tev::Draw()
       // RGBA
       u8 texel[4];
 
-      TextureSampler::Sample(TexCoord.s, TexCoord.t, TextureLod[stageNum], TextureLinear[stageNum],
-                             texmap, texel);
+      if (bpmem.genMode.numtexgens > 0)
+      {
+        TextureSampler::Sample(TexCoord.s, TexCoord.t, TextureLod[stageNum],
+                               TextureLinear[stageNum], texmap, texel);
+      }
+      else
+      {
+        // It seems like the result is always black when no tex coords are enabled, but further
+        // hardware testing is needed.
+        std::memset(texel, 0, 4);
+      }
 
 #if ALLOW_TEV_DUMPS
       if (g_ActiveConfig.bDumpTevTextureFetches)

Source/Core/VideoCommon/BPMemory.h

@@ -300,6 +300,31 @@ struct fmt::formatter<IndTexBias> : EnumFormatter<IndTexBias::STU>
   formatter() : EnumFormatter({"None", "S", "T", "ST", "U", "SU", "TU", "STU"}) {}
 };
 
+enum class IndMtxIndex : u32
+{
+  Off = 0,
+  Matrix0 = 1,
+  Matrix1 = 2,
+  Matrix2 = 3,
+};
+template <>
+struct fmt::formatter<IndMtxIndex> : EnumFormatter<IndMtxIndex::Matrix2>
+{
+  formatter() : EnumFormatter({"Off", "Matrix 0", "Matrix 1", "Matrix 2"}) {}
+};
+
+enum class IndMtxId : u32
+{
+  Indirect = 0,
+  S = 1,
+  T = 2,
+};
+template <>
+struct fmt::formatter<IndMtxId> : EnumFormatter<IndMtxId::T>
+{
+  formatter() : EnumFormatter({"Indirect", "S", "T"}) {}
+};
+
 // Indirect texture bump alpha
 enum class IndTexBumpAlpha : u32
 {
@@ -335,23 +360,23 @@ union IND_MTXA
 {
   BitField<0, 11, s32> ma;
   BitField<11, 11, s32> mb;
-  BitField<22, 2, u32> s0;  // bits 0-1 of scale factor
+  BitField<22, 2, u8, u32> s0;  // bits 0-1 of scale factor
   u32 hex;
 };
 
 union IND_MTXB
 {
   BitField<0, 11, s32> mc;
   BitField<11, 11, s32> md;
-  BitField<22, 2, u32> s1;  // bits 2-3 of scale factor
+  BitField<22, 2, u8, u32> s1;  // bits 2-3 of scale factor
   u32 hex;
 };
 
 union IND_MTXC
 {
   BitField<0, 11, s32> me;
   BitField<11, 11, s32> mf;
-  BitField<22, 2, u32> s2;  // bits 4-5 of scale factor
+  BitField<22, 2, u8, u32> s2;  // bits 4-5 of scale factor
   u32 hex;
 };
 
@@ -360,6 +385,7 @@ struct IND_MTX
   IND_MTXA col0;
   IND_MTXB col1;
   IND_MTXC col2;
+  u8 GetScale() const { return (col0.s0 << 0) | (col1.s1 << 2) | (col2.s2 << 4); }
 };
 
 union IND_IMASK
@@ -475,8 +501,12 @@ union TevStageIndirect
   BitField<4, 1, bool, u32> bias_s;
   BitField<5, 1, bool, u32> bias_t;
   BitField<6, 1, bool, u32> bias_u;
-  BitField<7, 2, IndTexBumpAlpha> bs;     // Indicates which coordinate will become the 'bump alpha'
-  BitField<9, 4, u32> mid;                // Matrix ID to multiply offsets with
+  BitField<7, 2, IndTexBumpAlpha> bs;  // Indicates which coordinate will become the 'bump alpha'
+  // Indicates which indirect matrix is used when matrix_id is Indirect.
+  // Also always indicates which indirect matrix to use for the scale factor, even with S or T.
+  BitField<9, 2, IndMtxIndex> matrix_index;
+  // Should be set to Indirect (0) if matrix_index is Off (0)
+  BitField<11, 2, IndMtxId> matrix_id;
   BitField<13, 3, IndTexWrap> sw;         // Wrapping factor for S of regular coord
   BitField<16, 3, IndTexWrap> tw;         // Wrapping factor for T of regular coord
   BitField<19, 1, bool, u32> lb_utclod;   // Use modified or unmodified texture
@@ -492,9 +522,9 @@ union TevStageIndirect
 
   u32 fullhex;
 
-  // If bs and mid are zero, the result of the stage is independent of
+  // If bs and matrix are zero, the result of the stage is independent of
   // the texture sample data, so we can skip sampling the texture.
-  bool IsActive() const { return bs != IndTexBumpAlpha::Off || mid != 0; }
+  bool IsActive() const { return bs != IndTexBumpAlpha::Off || matrix_index != IndMtxIndex::Off; }
 };
 template <>
 struct fmt::formatter<TevStageIndirect>
@@ -508,13 +538,15 @@ struct fmt::formatter<TevStageIndirect>
                      "Format: {}\n"
                      "Bias: {}\n"
                      "Bump alpha: {}\n"
+                     "Offset matrix index: {}\n"
                      "Offset matrix ID: {}\n"
                      "Regular coord S wrapping factor: {}\n"
                      "Regular coord T wrapping factor: {}\n"
                      "Use modified texture coordinates for LOD computation: {}\n"
                      "Add texture coordinates from previous TEV stage: {}",
-                     tevind.bt, tevind.fmt, tevind.bias, tevind.bs, tevind.mid, tevind.sw,
-                     tevind.tw, tevind.lb_utclod ? "Yes" : "No", tevind.fb_addprev ? "Yes" : "No");
+                     tevind.bt, tevind.fmt, tevind.bias, tevind.bs, tevind.matrix_index,
+                     tevind.matrix_id, tevind.sw, tevind.tw, tevind.lb_utclod ? "Yes" : "No",
+                     tevind.fb_addprev ? "Yes" : "No");
   }
 };
 
@@ -600,13 +632,13 @@ struct fmt::formatter<TEXSCALE>
 union RAS1_IREF
 {
   BitField<0, 3, u32> bi0;  // Indirect tex stage 0 ntexmap
-  BitField<3, 3, u32> bc0;  // Indirect tex stage 0 ntexmap
+  BitField<3, 3, u32> bc0;  // Indirect tex stage 0 ntexcoord
   BitField<6, 3, u32> bi1;
   BitField<9, 3, u32> bc1;
   BitField<12, 3, u32> bi2;
-  BitField<15, 3, u32> bc3;  // Typo?
-  BitField<18, 3, u32> bi4;
-  BitField<21, 3, u32> bc4;
+  BitField<15, 3, u32> bc2;
+  BitField<18, 3, u32> bi3;
+  BitField<21, 3, u32> bc3;
   u32 hex;
 
   u32 getTexCoord(int i) const { return (hex >> (6 * i + 3)) & 7; }
@@ -625,8 +657,8 @@ struct fmt::formatter<RAS1_IREF>
                      "Stage 1 ntexmap: {}\nStage 1 ntexcoord: {}\n"
                      "Stage 2 ntexmap: {}\nStage 2 ntexcoord: {}\n"
                      "Stage 3 ntexmap: {}\nStage 3 ntexcoord: {}",
-                     indref.bi0, indref.bc0, indref.bi1, indref.bc1, indref.bi2, indref.bc3,
-                     indref.bi4, indref.bc4);
+                     indref.bi0, indref.bc0, indref.bi1, indref.bc1, indref.bi2, indref.bc2,
+                     indref.bi3, indref.bc3);
   }
 };
 
@@ -911,6 +943,8 @@ union GenMode
   BitField<7, 1, u32> unused;              // 1 bit unused?
   BitField<8, 1, bool, u32> flat_shading;  // unconfirmed
   BitField<9, 1, bool, u32> multisampling;
+  // This value is 1 less than the actual number (0-15 map to 1-16).
+  // In other words there is always at least 1 tev stage
   BitField<10, 4, u32> numtevstages;
   BitField<14, 2, CullMode> cullmode;
   BitField<16, 3, u32> numindstages;
@@ -937,7 +971,7 @@ struct fmt::formatter<GenMode>
                      "ZFreeze: {}",
                      mode.numtexgens, mode.numcolchans, mode.unused,
                      mode.flat_shading ? "Yes" : "No", mode.multisampling ? "Yes" : "No",
-                     mode.numtevstages, mode.cullmode, mode.numindstages,
+                     mode.numtevstages + 1, mode.cullmode, mode.numindstages,
                      mode.zfreeze ? "Yes" : "No");
   }
 };
@@ -1912,7 +1946,7 @@ struct BPMemory
   GenMode genMode;
   u32 display_copy_filter[4];  // 01-04
   u32 unknown;                 // 05
-  // indirect matrices (set by GXSetIndTexMtx, selected by TevStageIndirect::mid)
+  // indirect matrices (set by GXSetIndTexMtx, selected by TevStageIndirect::matrix_index)
   // abc form a 2x3 offset matrix, there's 3 such matrices
   // the 3 offset matrices can either be indirect type, S-type, or T-type
   // 6bit scale factor s is distributed across IND_MTXA/B/C.

Source/Core/VideoCommon/GXPipelineTypes.h

@@ -20,6 +20,7 @@ namespace VideoCommon
 // As pipelines encompass both shader UIDs and render states, changes to either of these should
 // also increment the pipeline UID version. Incrementing the UID version will cause all UID
 // caches to be invalidated.
+// TODO: Remove PixelShaderUid hasindstage on the next UID version bump
 constexpr u32 GX_PIPELINE_UID_VERSION = 2;  // Last changed in PR 9122
 
 struct GXPipelineUid