Repository: RetroAchievements/RAIntegration
Pull request #847 (2021-12-16 02:40:13)

revalidate assets after memory is initialized to check for out of range addresses
Ensures committed Unofficial/Local achievements with invalid memory addresses get flagged when reopening the game.


Patch:
diff --git a/src/data/models/AchievementModel.cpp b/src/data/models/AchievementModel.cpp
index 524b8dde..40f85e72 100644
--- a/src/data/models/AchievementModel.cpp
+++ b/src/data/models/AchievementModel.cpp
@@ -119,7 +119,7 @@ void AchievementModel::CommitTransaction()
 
 bool AchievementModel::ValidateAsset(std::wstring& sError)
 {
-    const auto& sTrigger = GetLocalAssetDefinition(m_pTrigger);
+    const auto& sTrigger = GetAssetDefinition(m_pTrigger);
     return TriggerValidation::Validate(sTrigger, sError, AssetType::Achievement);
 }
 
diff --git a/src/data/models/LeaderboardModel.cpp b/src/data/models/LeaderboardModel.cpp
index 59183739..f07fd422 100644
--- a/src/data/models/LeaderboardModel.cpp
+++ b/src/data/models/LeaderboardModel.cpp
@@ -71,6 +71,7 @@ void LeaderboardModel::OnValueChanged(const IntModelProperty::ChangeArgs& args)
 bool LeaderboardModel::ValidateAsset(std::wstring& sError)
 {
     const auto& sStartTrigger = GetAssetDefinition(m_pStartTrigger);
+
     if (sStartTrigger.empty())
     {
         sError = L"No Start condition";
diff --git a/src/services/AchievementRuntime.cpp b/src/services/AchievementRuntime.cpp
index 4ba45dfc..977df9f0 100644
--- a/src/services/AchievementRuntime.cpp
+++ b/src/services/AchievementRuntime.cpp
@@ -967,9 +967,20 @@ void AchievementRuntime::OnTotalMemorySizeChanged()
     auto& pEmulatorContext = ra::services::ServiceLocator::GetMutable<ra::data::context::EmulatorContext>();
     pEmulatorContext.RemoveNotifyTarget(*this);
 
+    // schedule the callback for 100ms from now in case more changes are coming.
+    // we've unsubscribed from the event, so the callback will only be called once.
     auto& pThreadPool = ra::services::ServiceLocator::GetMutable<ra::services::IThreadPool>();
     pThreadPool.ScheduleAsync(std::chrono::milliseconds(100), [this]()
     {
+        // re-validate the assets in case they have invalid memory references
+        auto& pGameAssets = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>().Assets();
+        for (gsl::index nIndex = 0; nIndex < gsl::narrow_cast<gsl::index>(pGameAssets.Count()); ++nIndex)
+        {
+            auto* pAsset = pGameAssets.GetItemAt(nIndex);
+            if (pAsset != nullptr)
+                pAsset->Validate();
+        }
+
         DetectUnsupportedAchievements();
     });
 }
@@ -987,6 +998,7 @@ void AchievementRuntime::InvalidateAddress(ra::ByteAddress nAddress) noexcept
 
 size_t AchievementRuntime::DetectUnsupportedAchievements()
 {
+    // if memory hasn't been registered yet, wait for it
     auto& pEmulatorContext = ra::services::ServiceLocator::GetMutable<ra::data::context::EmulatorContext>();
     if (pEmulatorContext.TotalMemorySize() == 0)
     {


Test patch:
diff --git a/tests/data/models/AchievementModel_Tests.cpp b/tests/data/models/AchievementModel_Tests.cpp
index 5fea76fc..e0e9339a 100644
--- a/tests/data/models/AchievementModel_Tests.cpp
+++ b/tests/data/models/AchievementModel_Tests.cpp
@@ -246,6 +246,45 @@ public:
         Assert::AreEqual(0, pLocalBadges.GetReferenceCount(L"local\\22-ABC.png", false));
         Assert::AreEqual(0, pLocalBadges.GetReferenceCount(L"local\\22-ABC.png", true));
     }
+
+    TEST_METHOD(TestValidate)
+    {
+        AchievementModelHarness achievement;
+        achievement.mockGameContext.SetGameId(22);
+        achievement.AddLocalBadgesModel();
+        achievement.SetBadge(L"12345.png");
+        achievement.CreateServerCheckpoint();
+        achievement.CreateLocalCheckpoint();
+
+        Assert::IsTrue(achievement.Validate());
+        Assert::AreEqual(std::wstring(), achievement.GetValidationError());
+
+        achievement.SetID(1U);
+        achievement.SetTrigger("A:0xH1234=1");
+
+        // Validation checks the uncommited state.
+        Assert::IsFalse(achievement.Validate());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), achievement.GetValidationError());
+
+        // Validation checks the uncommited state.
+        achievement.UpdateLocalCheckpoint();
+        achievement.SetTrigger("0xH1234=1");
+
+        Assert::IsTrue(achievement.Validate());
+        Assert::AreEqual(std::wstring(), achievement.GetValidationError());
+
+        // Restoring the local checkpoint automatically updates the validation error.
+        achievement.RestoreLocalCheckpoint();
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), achievement.GetValidationError());
+
+        // Fixing the error does not automatically update the validation error.
+        achievement.SetTrigger("0xH1234=1");
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), achievement.GetValidationError());
+
+        // Committing the fix does update the validation error.
+        achievement.UpdateLocalCheckpoint();
+        Assert::AreEqual(std::wstring(), achievement.GetValidationError());
+    }
 };
 
 
diff --git a/tests/services/AchievementRuntime_Tests.cpp b/tests/services/AchievementRuntime_Tests.cpp
index 0b5ef390..2710692c 100644
--- a/tests/services/AchievementRuntime_Tests.cpp
+++ b/tests/services/AchievementRuntime_Tests.cpp
@@ -1,6 +1,7 @@
 #include "services\AchievementRuntime.hh"
 
 #include "tests\RA_UnitTestHelpers.h"
+#include "tests\data\DataAsserts.hh"
 #include "tests\mocks\MockEmulatorContext.hh"
 #include "tests\mocks\MockFileSystem.hh"
 #include "tests\mocks\MockGameContext.hh"
@@ -1232,6 +1233,63 @@ public:
         Assert::AreEqual({ 1U }, vChanges.size());
         AssertChange(vChanges, AchievementRuntime::ChangeType::LeaderboardCancelTriggered, 6U);
     }
+
+    TEST_METHOD(TestDetectUnsupportedAchievements)
+    {
+        std::array<unsigned char, 2> memory{ 0x00, 0x00 };
+
+        AchievementRuntimeHarness runtime;
+        auto& pAch1 = runtime.mockGameContext.Assets().NewAchievement();
+        pAch1.SetTrigger("0xH0000=1");
+        pAch1.UpdateLocalCheckpoint();
+        pAch1.Activate();
+        auto& pAch2 = runtime.mockGameContext.Assets().NewAchievement();
+        pAch2.SetTrigger("0xH0002=1");
+        pAch1.UpdateLocalCheckpoint();
+        pAch2.Activate();
+        auto& pAch3 = runtime.mockGameContext.Assets().NewAchievement();
+        pAch3.SetTrigger("0xH0002=1");
+        pAch1.UpdateLocalCheckpoint();
+
+        Assert::AreEqual(ra::data::models::AssetState::Waiting, pAch1.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Waiting, pAch2.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Inactive, pAch3.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+
+        // no memory registered, can't detect yet
+        runtime.DetectUnsupportedAchievements();
+        Assert::AreEqual(ra::data::models::AssetState::Waiting, pAch1.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Waiting, pAch2.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Inactive, pAch3.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+
+        // unsupported achievements will be detected 100ms after the memory is registered
+        runtime.mockEmulatorContext.MockMemory(memory);
+        runtime.mockThreadPool.AdvanceTime(std::chrono::milliseconds(100));
+
+        // actually disable the achievements that were flagged
+        std::vector<AchievementRuntime::Change> vChanges;
+        runtime.Process(vChanges);
+        Assert::AreEqual({ 2U }, vChanges.size());
+        AssertChange(vChanges, AchievementRuntime::ChangeType::AchievementActivated, pAch1.GetID());
+        AssertChange(vChanges, AchievementRuntime::ChangeType::AchievementDisabled, pAch2.GetID());
+
+        // sync the state
+        pAch1.DoFrame();
+        pAch2.DoFrame();
+        pAch3.DoFrame();
+
+        Assert::AreEqual(ra::data::models::AssetState::Active, pAch1.GetState());
+        Assert::AreEqual(std::wstring(), pAch1.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Disabled, pAch2.GetState());
+        Assert::AreEqual(std::wstring(L"Condition 1: Address 0002 out of range (max 0001)"), pAch2.GetValidationError());
+        Assert::AreEqual(ra::data::models::AssetState::Inactive, pAch3.GetState());
+        Assert::AreEqual(std::wstring(L"Condition 1: Address 0002 out of range (max 0001)"), pAch3.GetValidationError());
+    }
 };
 
 } // namespace tests