Add Support for Gecko Codes to Achievements Whitelist #12955

LillyJadeKatrin · 2024-07-25T12:42:43Z

This PR sets up the Achievements whitelist for patches to also support Gecko codes.

JosJuice · 2024-07-25T16:04:54Z

I recommend also supporting Action Replay codes.

LillyJadeKatrin · 2024-07-25T20:50:12Z

I can add that as well, but thought one step at a time, and the requests have overwhelmingly been for Gecko next.

JosJuice · 2024-07-26T14:09:10Z

Okay, I suppose we can do one at a time.

JosJuice · 2024-07-27T18:25:24Z

The message "Failed to verify patch {} from file {}." currently mentions patches specifically. It should either mention the specific type of code (i.e. Gecko code or patch), or it could say just "code" instead of "patch".

Source/Core/Core/GeckoCodeConfig.h

Tilka · 2024-08-11T13:48:27Z

Smoke-tested?

AdmiralCurtiss · 2024-08-13T18:01:24Z

In particular, please test if there is any way to cause this to discard 'unapproved' codes from the user's local game INI. It's unclear to me whether that is possible from the code.

JosJuice · 2024-08-14T19:24:47Z

I missed something in my review: The unit test (PatchAllowlistTest.cpp) is still only checking patches, not Gecko codes.

LillyJadeKatrin · 2024-08-16T00:16:50Z

Decided to add a couple things anyways so I'm drafting this until it's ready.

LillyJadeKatrin · 2024-10-04T02:50:11Z

In particular, please test if there is any way to cause this to discard 'unapproved' codes from the user's local game INI. It's unclear to me whether that is possible from the code.

I'm not entirely certain I follow what you mean here @AdmiralCurtiss , I'm not aware of anything that causes codes to be removed from the INI files but I don't know what I don't know. I'm definitely not calling it on purpose; the functionality should simply be filtering out active codes. I think.

AdmiralCurtiss · 2024-10-06T12:49:47Z

What I mean is, this seems like a perfectly plausible code path that I'm not sure how you're preventing:

GeckoCodeWidget constructor
  calls GeckoCodeWidget::LoadCodes()
   calls Gecko::LoadCodes(game_ini_default, game_ini_local)
     filters codes, removes any unapproved
   filtered codes are stored in m_gecko_codes

later:

GeckoCodeWidget::SaveCodes() is called for any reason
  calls Gecko::SaveCodes(game_ini_local, m_gecko_codes) with the filtered codes
    calls inifile.SetLines("Gecko", lines) with filtered codes
      overwrites any pre-existing codes in the ini
  saves modified ini without the unapproved codes to disk

LillyJadeKatrin · 2024-10-06T16:15:59Z

While I'm looking into other questions brought up by this investigation, I can tell you that it appears that no code path within Dolphin is modifying ini files from the Sys folder, aka codes from the repo. What to do with user codes is a different question I'm evaluating in the Discord server currently.

Now that patches and codes are enabled on a case by case basis, remove patcher code blocking codes entirely in hardcore mode, and reword the warning to be more accurate.

Due to a strange compatibility bug with creator name on Gecko codes, this also adds the SSL patch on all versions of the Nintendo Channel.

dolphin-ci · 2024-10-15T02:18:29Z

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

sw3-dt on ogl-lin-mesa: diff

_{^{automated-fifoci-reporter}}

Re-reviewing after the new changes

JosJuice · 2024-10-15T07:57:11Z

Data/Sys/ApprovedInis.json

@@ -312,4 +328,4 @@
 "title": "Gormiti: The Lords of Nature!",
 "6F8CD59D897338CA90939149E1A62588620C6D88": "Fix black screen"
 }
-}
+}


Please add trailing newlines to all files that lack them.

JosJuice · 2024-10-15T07:57:22Z

Source/Core/Core/AchievementManager.cpp

-void AchievementManager::FilterApprovedPatches(std::vector<PatchEngine::Patch>& patches,
- const std::string& game_ini_id) const
+template <typename T>
+void AchievementManager::FilterApprovedIni(std::vector<T>& patches,


Suggested change

void AchievementManager::FilterApprovedIni(std::vector<T>& patches,

void AchievementManager::FilterApprovedIni(std::vector<T>& codes,

JosJuice · 2024-10-15T07:57:30Z

Source/Core/Core/AchievementManager.cpp

@@ -387,46 +390,100 @@ void AchievementManager::FilterApprovedPatches(std::vector<PatchEngine::Patch>&
 if (!IsHardcoreModeActive())
 return;

+ // Approved patches list failed to hash


Suggested change

// Approved patches list failed to hash

// Approved codes list failed to hash

JosJuice · 2024-10-15T07:57:45Z

Source/Core/Core/AchievementManager.cpp

- INFO_LOG_FMT(ACHIEVEMENTS, "Verifying patch {}", patch_itr->name);
+ if (patch_itr->enabled)
+ {
+ INFO_LOG_FMT(ACHIEVEMENTS, "Verifying patch {}", patch_itr->name);


Suggested change

INFO_LOG_FMT(ACHIEVEMENTS, "Verifying patch {}", patch_itr->name);

INFO_LOG_FMT(ACHIEVEMENTS, "Verifying code {}", patch_itr->name);

JosJuice · 2024-10-15T07:58:21Z

Source/Core/Core/AchievementManager.h

+ Common::SHA1::Digest GetPatchHash(const PatchEngine::Patch& patch) const;
+ Common::SHA1::Digest GetPatchHash(const Gecko::GeckoCode& patch) const;
+ Common::SHA1::Digest GetPatchHash(const ActionReplay::ARCode& patch) const;


Suggested change

Common::SHA1::Digest GetPatchHash(const PatchEngine::Patch& patch) const;

Common::SHA1::Digest GetPatchHash(const Gecko::GeckoCode& patch) const;

Common::SHA1::Digest GetPatchHash(const ActionReplay::ARCode& patch) const;

Common::SHA1::Digest GetCodeHash(const PatchEngine::Patch& code) const;

Common::SHA1::Digest GetCodeHash(const Gecko::GeckoCode& code) const;

Common::SHA1::Digest GetCodeHash(const ActionReplay::ARCode& code) const;

JosJuice · 2024-10-15T08:05:56Z

Source/Core/Core/PatchEngine.cpp

+ auto ar_codes = ActionReplay::LoadCodes(globalIni, localIni);
+#ifdef USE_RETRO_ACHIEVEMENTS
+ {
+ std::lock_guard lg{AchievementManager::GetInstance().GetLock()};


Redundant locking.

JosJuice · 2024-10-15T08:08:16Z

Source/Core/DiscIO/RiivolutionPatcher.cpp

@@ -524,9 +524,6 @@ static bool MemoryMatchesAt(const Core::CPUThreadGuard& guard, u32 offset,
 static void ApplyMemoryPatch(const Core::CPUThreadGuard& guard, u32 offset,
 std::span<const u8> value, std::span<const u8> original)
 {
- if (AchievementManager::GetInstance().IsHardcoreModeActive())
- return;
-


Are you sure you want to remove this? You have no filtering for Riivolution memory patches.

JosJuice · 2024-10-15T08:09:22Z

Source/Core/DolphinQt/Config/HardcoreWarningWidget.cpp

@@ -35,7 +35,7 @@ void HardcoreWarningWidget::CreateWidgets()
 auto* icon = new QLabel;
 icon->setPixmap(warning_icon);

- m_text = new QLabel(tr("This feature is disabled in hardcore mode."));
+ m_text = new QLabel(tr("Unapproved codes will not be applied in hardcore mode."));


In my opinion, this would be clearer without the double negative:

Suggested change

m_text = new QLabel(tr("Unapproved codes will not be applied in hardcore mode."));

m_text = new QLabel(tr("In hardcore mode, only approved codes will be applied."));

But this is only a suggestion.

JosJuice · 2024-10-15T08:14:11Z

Source/Core/DolphinQt/Config/ARCodeWidget.cpp

+#ifdef USE_RETRO_ACHIEVEMENTS
+ {
+ std::lock_guard lg{AchievementManager::GetInstance().GetLock()};
+ AchievementManager::GetInstance().FilterApprovedARCodes(m_ar_codes, m_game_id);


Doing filtering in ARCodeWidget and GeckoCodeWidget looks a lot to me like it would cause the problem AdmiralCurtiss mentioned. Look just a few lines below and you'll see a call to SaveCodes.

JosJuice · 2024-10-15T08:18:16Z

Source/UnitTests/Core/PatchAllowlistTest.cpp

@@ -67,12 +70,22 @@ TEST(PatchAllowlist, VerifyHashes)
 Common::IniFile ini_file;
 ini_file.Load(file.physicalName, true);
 std::string game_id = file.virtualName.substr(0, file.virtualName.find_first_of('.'));
+ std::map<std::string /*hash*/, std::string /*name*/> hashes;


It would be a bit more efficient to, instead of having an std::map where you store all hashes, have a "check hash" function that you can call from the patches loop, the AR codes loop and the Gecko loop. That way, instead of having to store the hash from each loop iteration in the std::map, you can discard the hash at the end of each loop iteration, like the code was doing before this PR.

JosJuice · 2024-10-15T09:12:55Z

The description of the last commit mentions "a strange compatibility bug with creator name on Gecko codes". Could you explain why this means you should allowlist the SSL patch for the Nintendo Channel?

JosJuice · 2024-10-15T09:14:03Z

Also, have you checked what happens if you use netplay code syncing when the person hosting isn't using hardcore mode but the person joining is?

LillyJadeKatrin force-pushed the retroachievements-gecko branch 2 times, most recently from 774e6a8 to 2f1eaf5 Compare July 25, 2024 13:28

JosJuice reviewed Jul 27, 2024

View reviewed changes

Source/Core/Core/GeckoCodeConfig.h Outdated Show resolved Hide resolved

LillyJadeKatrin force-pushed the retroachievements-gecko branch 2 times, most recently from b79f789 to f10a618 Compare August 9, 2024 01:07

JosJuice previously approved these changes Aug 11, 2024

View reviewed changes

LillyJadeKatrin marked this pull request as draft August 16, 2024 00:16

LillyJadeKatrin force-pushed the retroachievements-gecko branch 5 times, most recently from 16f958b to afb9862 Compare September 12, 2024 23:52

LillyJadeKatrin force-pushed the retroachievements-gecko branch from afb9862 to b508ab4 Compare October 4, 2024 01:43

LillyJadeKatrin marked this pull request as ready for review October 5, 2024 03:07

LillyJadeKatrin force-pushed the retroachievements-gecko branch from 96a8a0e to 4c1bff9 Compare October 5, 2024 04:09

LillyJadeKatrin force-pushed the retroachievements-gecko branch 4 times, most recently from 8644e1f to 6458d6e Compare October 6, 2024 22:31

Convert FilterApprovedPatches to Template

24a3bb7

LillyJadeKatrin force-pushed the retroachievements-gecko branch from 6458d6e to d68e897 Compare October 6, 2024 22:50

LillyJadeKatrin marked this pull request as draft October 7, 2024 05:15

LillyJadeKatrin force-pushed the retroachievements-gecko branch from d68e897 to 164580a Compare October 15, 2024 02:00

LillyJadeKatrin added 5 commits October 14, 2024 22:03

Add Gecko Code Whitelist Approval

bce55f7

Add AR Code Whitelist Approval

9477682

Scale back hardcore code block

f50a91a

Now that patches and codes are enabled on a case by case basis, remove patcher code blocking codes entirely in hardcore mode, and reword the warning to be more accurate.

Add Gecko and AR codes to the Patch Allowlist Test

c564cb3

Approve Super Mario Sunshine Widescreen Gecko Code

4f3e3d2

Due to a strange compatibility bug with creator name on Gecko codes, this also adds the SSL patch on all versions of the Nintendo Channel.

LillyJadeKatrin force-pushed the retroachievements-gecko branch from 164580a to 4f3e3d2 Compare October 15, 2024 02:03

LillyJadeKatrin marked this pull request as ready for review October 15, 2024 02:03

JosJuice reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Gecko Codes to Achievements Whitelist #12955

Add Support for Gecko Codes to Achievements Whitelist #12955

LillyJadeKatrin commented Jul 25, 2024

JosJuice commented Jul 25, 2024

LillyJadeKatrin commented Jul 25, 2024

JosJuice commented Jul 26, 2024

JosJuice commented Jul 27, 2024

Tilka commented Aug 11, 2024

AdmiralCurtiss commented Aug 13, 2024

JosJuice commented Aug 14, 2024

LillyJadeKatrin commented Aug 16, 2024

LillyJadeKatrin commented Oct 4, 2024

AdmiralCurtiss commented Oct 6, 2024

LillyJadeKatrin commented Oct 6, 2024

dolphin-ci bot commented Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice Oct 15, 2024

JosJuice commented Oct 15, 2024

JosJuice commented Oct 15, 2024

	void AchievementManager::FilterApprovedIni(std::vector<T>& patches,
	void AchievementManager::FilterApprovedIni(std::vector<T>& codes,

	// Approved patches list failed to hash
	// Approved codes list failed to hash

	INFO_LOG_FMT(ACHIEVEMENTS, "Verifying patch {}", patch_itr->name);
	INFO_LOG_FMT(ACHIEVEMENTS, "Verifying code {}", patch_itr->name);

	m_text = new QLabel(tr("Unapproved codes will not be applied in hardcore mode."));
	m_text = new QLabel(tr("In hardcore mode, only approved codes will be applied."));

Add Support for Gecko Codes to Achievements Whitelist #12955

Are you sure you want to change the base?

Add Support for Gecko Codes to Achievements Whitelist #12955

Conversation

LillyJadeKatrin commented Jul 25, 2024

JosJuice commented Jul 25, 2024

LillyJadeKatrin commented Jul 25, 2024

JosJuice commented Jul 26, 2024

JosJuice commented Jul 27, 2024

Tilka commented Aug 11, 2024

AdmiralCurtiss commented Aug 13, 2024

JosJuice commented Aug 14, 2024

LillyJadeKatrin commented Aug 16, 2024

LillyJadeKatrin commented Oct 4, 2024

AdmiralCurtiss commented Oct 6, 2024

LillyJadeKatrin commented Oct 6, 2024

dolphin-ci bot commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosJuice commented Oct 15, 2024

JosJuice commented Oct 15, 2024