Merge pull request #8903 from AdmiralCurtiss/gcmemcard-file-identity-check

GCMemcard: Change behavior of TitlePresent() to more closely resemble how saves are actually identified.

Source/Core/Core/CMakeLists.txt

@@ -226,6 +226,8 @@ add_library(core
   HW/GCMemcard/GCMemcardDirectory.h
   HW/GCMemcard/GCMemcardRaw.cpp
   HW/GCMemcard/GCMemcardRaw.h
+  HW/GCMemcard/GCMemcardUtils.cpp
+  HW/GCMemcard/GCMemcardUtils.h
   HW/GCPad.cpp
   HW/GCPad.h
   HW/GCPadEmu.cpp

Source/Core/Core/Core.vcxproj

@@ -159,6 +159,7 @@
     <ClCompile Include="HW\GCMemcard\GCMemcard.cpp" />
     <ClCompile Include="HW\GCMemcard\GCMemcardDirectory.cpp" />
     <ClCompile Include="HW\GCMemcard\GCMemcardRaw.cpp" />
+    <ClCompile Include="HW\GCMemcard\GCMemcardUtils.cpp" />
     <ClCompile Include="HW\GCPad.cpp" />
     <ClCompile Include="HW\GCPadEmu.cpp" />
     <ClCompile Include="HW\GPFifo.cpp" />
@@ -518,6 +519,7 @@
     <ClInclude Include="HW\GCMemcard\GCMemcardBase.h" />
     <ClInclude Include="HW\GCMemcard\GCMemcardDirectory.h" />
     <ClInclude Include="HW\GCMemcard\GCMemcardRaw.h" />
+    <ClInclude Include="HW\GCMemcard\GCMemcardUtils.h" />
     <ClInclude Include="HW\GCPad.h" />
     <ClInclude Include="HW\GCPadEmu.h" />
     <ClInclude Include="HW\GPFifo.h" />

Source/Core/Core/Core.vcxproj.filters

@@ -481,6 +481,9 @@
     <ClCompile Include="HW\GCMemcard\GCMemcardRaw.cpp">
       <Filter>HW %28Flipper/Hollywood%29\GCMemcard</Filter>
     </ClCompile>
+    <ClCompile Include="HW\GCMemcard\GCMemcardUtils.cpp">
+      <Filter>HW %28Flipper/Hollywood%29\GCMemcard</Filter>
+    </ClCompile>
     <ClCompile Include="HW\GCPad.cpp">
       <Filter>HW %28Flipper/Hollywood%29\GCPad</Filter>
     </ClCompile>
@@ -1260,6 +1263,9 @@
     <ClInclude Include="HW\GCMemcard\GCMemcardRaw.h">
       <Filter>HW %28Flipper/Hollywood%29\GCMemcard</Filter>
     </ClInclude>
+    <ClInclude Include="HW\GCMemcard\GCMemcardUtils.h">
+      <Filter>HW %28Flipper/Hollywood%29\GCMemcard</Filter>
+    </ClInclude>
     <ClInclude Include="HW\GCPadEmu.h">
       <Filter>HW %28Flipper/Hollywood%29\GCPad</Filter>
     </ClInclude>

Source/Core/Core/HW/GCMemcard/GCMemcard.cpp

@@ -21,6 +21,8 @@
 #include "Common/StringUtil.h"
 #include "Common/Swap.h"
 
+#include "Core/HW/GCMemcard/GCMemcardUtils.h"
+
 static constexpr std::optional<u64> BytesToMegabits(u64 bytes)
 {
   const u64 factor = ((1024 * 1024) / 8);
@@ -405,22 +407,19 @@ u16 GCMemcard::GetFreeBlocks() const
   return GetActiveBat().m_free_blocks;
 }
 
-u8 GCMemcard::TitlePresent(const DEntry& d) const
+std::optional<u8> GCMemcard::TitlePresent(const DEntry& d) const
 {
   if (!m_valid)
-    return DIRLEN;
+    return std::nullopt;
 
-  u8 i = 0;
-  while (i < DIRLEN)
+  const Directory& dir = GetActiveDirectory();
+  for (u8 i = 0; i < DIRLEN; ++i)
   {
-    if (GetActiveDirectory().m_dir_entries[i].m_gamecode == d.m_gamecode &&
-        GetActiveDirectory().m_dir_entries[i].m_filename == d.m_filename)
-    {
-      break;
-    }
-    i++;
+    if (HasSameIdentity(dir.m_dir_entries[i], d))
+      return i;
   }
-  return i;
+
+  return std::nullopt;
 }
 
 bool GCMemcard::GCI_FileName(u8 index, std::string& filename) const
@@ -853,7 +852,7 @@ GCMemcardImportFileRetVal GCMemcard::ImportFile(const DEntry& direntry,
   {
     return GCMemcardImportFileRetVal::OUTOFBLOCKS;
   }
-  if (TitlePresent(direntry) != DIRLEN)
+  if (TitlePresent(direntry))
   {
     return GCMemcardImportFileRetVal::TITLEPRESENT;
   }

Source/Core/Core/HW/GCMemcard/GCMemcard.h

@@ -460,8 +460,9 @@ public:
   // get the free blocks from bat
   u16 GetFreeBlocks() const;
 
-  // If title already on memcard returns index, otherwise returns -1
-  u8 TitlePresent(const DEntry& d) const;
+  // Returns index of the save with the same identity as the given DEntry, or nullopt if no save
+  // with that identity exists in this card.
+  std::optional<u8> TitlePresent(const DEntry& d) const;
 
   bool GCI_FileName(u8 index, std::string& filename) const;
   // DEntry functions, all take u8 index < DIRLEN (127)

Source/Core/Core/HW/GCMemcard/GCMemcardUtils.cpp

@@ -0,0 +1,56 @@
+#include "Core/HW/GCMemcard/GCMemcardUtils.h"
+
+#include "Core/HW/GCMemcard/GCMemcard.h"
+
+namespace Memcard
+{
+bool HasSameIdentity(const DEntry& lhs, const DEntry& rhs)
+{
+  // The Gamecube BIOS identifies two files as being 'the same' (that is, disallows copying from one
+  // card to another when both contain a file like it) when the full array of all of m_gamecode,
+  // m_makercode, and m_filename match between them.
+
+  // However, despite that, it seems like the m_filename should be treated as a nullterminated
+  // string instead, because:
+  // - Games seem to identify their saves regardless of what bytes appear after the first null byte.
+  // - If you have two files that match except for bytes after the first null in m_filename, the
+  //   BIOS behaves oddly if you attempt to copy the files, as it seems to clear out those extra
+  //   non-null bytes. See below for details.
+
+  // Specifically, the following chain of actions fails with a rather vague 'The data may not have
+  // been copied.' error message:
+  // - Have two memory cards with one save file each.
+  // - The two save files should have identical gamecode and makercode, as well as an equivalent
+  //   filename up until and including the first null byte.
+  // - On Card A have all remaining bytes of the filename also be null.
+  // - On Card B have at least one of the remaining bytes be non-null.
+  // - Copy the file on Card B to Card A.
+  // The BIOS will abort halfway through the copy process and declare Card B as unusable until you
+  // eject and reinsert it, and leave a "Broken File000" file on Card A, though note that the file
+  // is not visible and will be cleaned up when reinserting the card while still within the BIOS.
+  // Additionally, either during or after the copy process, the bytes after the first null on Card B
+  // are changed to null, which is presumably why the copy process ends up failing as Card A would
+  // then have two identical files. For reference, the Wii System Menu behaves exactly the same.
+
+  // With all that in mind, even if it mismatches the comparison behavior of the BIOS, we treat
+  // m_filename as a nullterminated string for determining if two files identify as the same, as not
+  // doing so would cause more harm and confusion that good in practice.
+
+  if (lhs.m_gamecode != rhs.m_gamecode)
+    return false;
+  if (lhs.m_makercode != rhs.m_makercode)
+    return false;
+
+  for (size_t i = 0; i < lhs.m_filename.size(); ++i)
+  {
+    const u8 a = lhs.m_filename[i];
+    const u8 b = rhs.m_filename[i];
+    if (a == 0)
+      return b == 0;
+    if (a != b)
+      return false;
+  }
+
+  return true;
+}
+}  // namespace Memcard

Source/Core/Core/HW/GCMemcard/GCMemcardUtils.h

@@ -0,0 +1,12 @@
+// Copyright 2020 Dolphin Emulator Project
+// Licensed under GPLv2+
+// Refer to the license.txt file included.
+
+#pragma once
+
+namespace Memcard
+{
+struct DEntry;
+
+bool HasSameIdentity(const DEntry& lhs, const DEntry& rhs);
+}  // namespace Memcard