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

Ignore the top bit of the indirect texture matrix scale #9863

Merged
merged 2 commits into from Jul 11, 2021

Conversation

Pokechu22
Copy link
Contributor

This PR replaces #9861; from that:

This fixes the appearance of Zora eyes (and bodies) in Twilight Princess; see bug 10346. This originally regressed in #68, and I bisected the issue to cff952c (though shaders from that commit will not run unless line 707 is changed by replacing int2(round(dot(...), dot(...))) with int2(round(dot(...)), round(dot(...)))).

However, this version should avoid introducing regressions. I semi-hardware tested it and the behavior seems to be that the top bit of the scale factor is ignored, i.e. supplying a scale value of 47 is the same as supplying one of 15, resulting in a scale factor of 2^(15-17) = 2^(-2). Specifically, I tested it with the Super Mario Bros. fifolog using the hardware FIFO player, and manually edited the scale factor for Object 3 (the hardware FIFO player lets you modify the FIFO while it's doing playback, which is neat). I confirmed that there was no wrapping between 0 and 31, and tested some of the values for 32-63 which matched the earlier values (but did not test them all due to it being a bit of a pain). I also checked the Zora eyes fifolog and confirmed that 47 and 15 looked the same (while supplying 46/48 or 14/16 looked different).

This change does also produce a rendering difference for the goop bubbles in Super Mario Sunshine. Unfortunately the hardware FIFO player is buggy and doesn't like Sunshine; I was only able to see the goop bubbles by hiding geometry for objects 371-441 and 100-200 to avoid vertex explosions (these may be excessive ranges, but they seemed to work). Although that did hide the goop itself, the goop bubbles did still show up, and the leftmost one was green and white (showing part of the path). I also tested on the game itself, and can confirm that the goop bubbles aren't supposed to match the appearance of the goop they obstruct; they're supposed to look like a different part of the area.

@Pokechu22 Pokechu22 force-pushed the zora-eyes-v2 branch 2 times, most recently from 2fdb415 to b6171d2 Compare July 5, 2021 19:59
@Pokechu22
Copy link
Contributor Author

I've switched to having a 1-bit bitfield instead of doing masking in GetScale(). I also switched to using formatters for the matrix columns instead of doing manual bitshifting; this also fixed a typo where column 1's float representation was actually the one for column 0 (something I also had a fix for in #9718).

Here's a somewhat messy decompilation of GXSetIndTexMtx as seen in Super Mario Sunshine; note that it uses (uVar2 & 0x30) << 0x12 and not (uVar2 & 0x10) << 0x12.

For reference/reproducibility, here's the patch I used to the hardware fifo player when testing (it forces 1 frame and fifo version 1 since the hardware player has a different interpretation of version 2 than what Dolphin actually uses, and also hardcodes different framebuffer sizes):
diff --git a/dffclient/client.cpp b/dffclient/client.cpp
index 77f0dc4..32c6927 100644
--- a/dffclient/client.cpp
+++ b/dffclient/client.cpp
@@ -344,7 +344,7 @@ QVariant DffModel::data(const QModelIndex& index, int role) const

                        if (data[0] == GX_LOAD_BP_REG)
                        {
-//                             if (role == Qt::DisplayRole)
+                               if (role == Qt::DisplayRole)
                                {
                                        char reg_name[32] = { 0 };
                                        GetBPRegInfo(data+1, reg_name, sizeof(reg_name), NULL, 0);
diff --git a/source/FifoDataFile.cpp b/source/FifoDataFile.cpp
index 746d140..c5c8b63 100644
--- a/source/FifoDataFile.cpp
+++ b/source/FifoDataFile.cpp
@@ -12,6 +12,8 @@ void LoadDffData(const char* filename, FifoData& out)
        size_t numread = fread(&header, sizeof(DffFileHeader), 1, out.file);
        header.FixEndianness();

+       header.file_version = 1;
+       header.frameCount = 1;
        if (header.fileId != 0x0d01f1f0 || header.min_loader_version > 2)
        {
                printf ("file ID or version don't match!\n");
diff --git a/source/main.cpp b/source/main.cpp
index 1638b38..2885716 100644
--- a/source/main.cpp
+++ b/source/main.cpp
@@ -212,6 +212,18 @@ void Init()
        VIDEO_Init();

        rmode = VIDEO_GetPreferredMode(NULL);
+       // NESVCtest.dff - these values aren't right (produces half-height video) but it's close enough to test
+       rmode->fbWidth = 512;
+       rmode->viWidth = 512;
+       rmode->xfbHeight = 228;
+       rmode->efbHeight = 228;
+       rmode->viHeight = 228;
+       // FACEATTACK.dff
+       /*rmode->fbWidth = 608;
+       rmode->viWidth = 608;
+       rmode->xfbHeight = 448;
+       rmode->efbHeight = 448;
+       rmode->viHeight = 448;*/
        first_frame = 1;
        fb = 0;
 #if ENABLE_CONSOLE!=1

@Pokechu22
Copy link
Contributor Author

Here's a fifolog that goes through all 64 possible values of the scale: IndirectScaleTestSMB.zip

I made it by hacking up the fifo recording code to manually increment the scale value, and then recording a 64-frame fifolog on the SMB title screen (note that this does mean the coin palette changes).
diff --git a/Source/Core/Core/FifoPlayer/FifoRecorder.cpp b/Source/Core/Core/FifoPlayer/FifoRecorder.cpp
index 48338399cc..b712884d3f 100644
--- a/Source/Core/Core/FifoPlayer/FifoRecorder.cpp
+++ b/Source/Core/Core/FifoPlayer/FifoRecorder.cpp
@@ -90,6 +90,32 @@ void FifoRecorder::WriteGPCommand(const u8* data, u32 size)
     size_t currentSize = m_FifoData.size();
     m_FifoData.resize(currentSize + size);
     memcpy(&m_FifoData[currentSize], data, size);
+
+    if (data[0] == 0x61)
+    {
+      // Hack to set the indirect scale values
+      if (data[1] >= 0x6 && data[1] <= 0xe)  // BPMEM_IND_MTXA thru BPMEM_IND_MTXC + 6
+      {
+        u32 value = 64 - m_RecordFramesRemaining;
+        static u32 lastValue = 0xffffffff;
+        static u32 counter = 0;
+        if (value != lastValue) {
+          lastValue = value;
+          counter = 0;
+        }
+        counter++;
+        if (counter > 6 && counter <= 9)
+        {
+          // Target object 3 of each frame, hacky
+          u32 col = (data[1] - 0x6) % 3;
+          value >>= (2 * col);
+          value &= 3;
+          // replace data[2]
+          m_FifoData[currentSize + 2] &= ~0xc0;
+          m_FifoData[currentSize + 2] |= value << 6;
+        }
+      }
+    }
   }
 
   if (m_FrameEnded && !m_FifoData.empty())
I also modified the hardware fifo player to not loop playback so that I could more clearly determine when it starts and ends.
diff --git a/dffclient/client.cpp b/dffclient/client.cpp
index 77f0dc4..32c6927 100644
--- a/dffclient/client.cpp
+++ b/dffclient/client.cpp
@@ -344,7 +344,7 @@ QVariant DffModel::data(const QModelIndex& index, int role) const

                        if (data[0] == GX_LOAD_BP_REG)
                        {
-//                             if (role == Qt::DisplayRole)
+                               if (role == Qt::DisplayRole)
                                {
                                        char reg_name[32] = { 0 };
                                        GetBPRegInfo(data+1, reg_name, sizeof(reg_name), NULL, 0);
diff --git a/source/FifoDataFile.cpp b/source/FifoDataFile.cpp
index 746d140..5d048ff 100644
--- a/source/FifoDataFile.cpp
+++ b/source/FifoDataFile.cpp
@@ -12,6 +12,8 @@ void LoadDffData(const char* filename, FifoData& out)
        size_t numread = fread(&header, sizeof(DffFileHeader), 1, out.file);
        header.FixEndianness();

+       header.file_version = 1;
+       //header.frameCount = 1;
        if (header.fileId != 0x0d01f1f0 || header.min_loader_version > 2)
        {
                printf ("file ID or version don't match!\n");
diff --git a/source/main.cpp b/source/main.cpp
index 1638b38..f9871ab 100644
--- a/source/main.cpp
+++ b/source/main.cpp
@@ -212,6 +212,18 @@ void Init()
        VIDEO_Init();

        rmode = VIDEO_GetPreferredMode(NULL);
+       // NESVCtest.dff - these values aren't right (produces half-height video) but it's close enough to test
+       rmode->fbWidth = 512;
+       rmode->viWidth = 512;
+       rmode->xfbHeight = 228;
+       rmode->efbHeight = 228;
+       rmode->viHeight = 228;
+       // FACEATTACK.dff
+       /*rmode->fbWidth = 608;
+       rmode->viWidth = 608;
+       rmode->xfbHeight = 448;
+       rmode->efbHeight = 448;
+       rmode->viHeight = 448;*/
        first_frame = 1;
        fb = 0;
 #if ENABLE_CONSOLE!=1
@@ -561,6 +573,7 @@ int main()
                        wgPipe->U8 = GX_LOAD_BP_REG;
                        wgPipe->U32 = (BPMEM_TRIGGER_EFB_COPY << 24) | copy.Hex;

+                       fb ^= 1;
                        VIDEO_SetNextFramebuffer(frameBuffer[fb]);
                        if (first_frame)
                        {
@@ -570,8 +583,9 @@ int main()
                }
 #endif
                VIDEO_Flush();
-               VIDEO_WaitVSync();
-               fb ^= 1;
+               for (int i = 0; i < 60; i++) {
+                       VIDEO_WaitVSync();
+               }

                // TODO: Menu stuff
                // reset GX state
@@ -597,7 +611,10 @@ int main()
                }

                ++cur_frame;
-               cur_frame = first_frame + ((cur_frame-first_frame) % (last_frame-first_frame+1));
+               //cur_frame = first_frame + ((cur_frame-first_frame) % (last_frame-first_frame+1));
+               if (cur_frame >= last_frame) {
+                       break;
+               }
        }

        fclose(fifo_data.file);

The behavior in Dolphin with this PR mostly matches my results on hardware, other than something weird with the 0 case (which I think is a fifo player bug, as it didn't occur during manual testing), and that it should produce a specific pattern when the value is 31, but on Dolphin that pattern instead happens at 30 (and 31 is a blue sky, while both agree on 32). Manual testing also had the same effect happen at 63 (though it doesn't show up in the video because I probably broke that somehow), while Dolphin with this change has it happen at 62. This slight inaccuracy (with 30/31 and 62/63) also affected Dolphin before this change, so it's not a regression, just an oddity.

This fixes the Zora eye coloration in Twilight Princess.
This also fixes a bug where the float version of row 1 actually showed row 0 again.
@Pokechu22 Pokechu22 changed the title Ignore the last bit of the indirect texture matrix scale Ignore the top bit of the indirect texture matrix scale Jul 11, 2021
@Tilka Tilka merged commit 9362676 into dolphin-emu:master Jul 11, 2021
11 checks passed
@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • sms-bubbles on ogl-lin-mesa: diff
  • tp-skin on ogl-lin-mesa: diff
  • zww-armos on ogl-lin-mesa: diff
  • sms-bubbles on sw-lin-mesa: diff
  • tp-skin on sw-lin-mesa: diff
  • zww-armos on sw-lin-mesa: diff
  • sms-bubbles on ogl-lin-radeon: diff
  • tp-skin on ogl-lin-radeon: diff
  • zww-armos on ogl-lin-radeon: diff
  • sms-bubbles on uberogl-lin-radeon: diff
  • tp-skin on uberogl-lin-radeon: diff
  • zww-armos on uberogl-lin-radeon: diff

automated-fifoci-reporter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants