Repository: RetroAchievements/RAIntegration
Pull request #1106 (2024-07-21 21:54:40)

use published achievement trigger when reverting
Fixes a crash that occurs when reverting an achievement currently loaded in the editor.

The revert was causing the runtime to associate the original achievement trigger with the AchievementModel, and then freeing the local achievement trigger, but the editor was still referencing the local achievement trigger, so when it tried to update the hitcounts for the editor, it crashed.

Solution: If the achievement definition is reverted to the published state, immediately associate the original achievement trigger, preventing the editor from getting a reference to memory that would later be released.


Patch:
diff --git a/src/data/models/AchievementModel.cpp b/src/data/models/AchievementModel.cpp
index b3794a0f..2e4700e3 100644
--- a/src/data/models/AchievementModel.cpp
+++ b/src/data/models/AchievementModel.cpp
@@ -403,6 +403,15 @@ void AchievementModel::SyncTrigger()
             m_pAchievement->trigger = nullptr;
         }
 
+        const auto* pPublishedAchievementInfo = pRuntime.GetPublishedAchievementInfo(m_pAchievement->public_.id);
+        if (pPublishedAchievementInfo && memcmp(pPublishedAchievementInfo->md5, md5, sizeof(md5)) == 0)
+        {
+            Expects(pPublishedAchievementInfo->trigger != nullptr);
+            m_pAchievement->trigger = pPublishedAchievementInfo->trigger;
+            rc_reset_trigger(m_pAchievement->trigger);
+            return;
+        }
+
         const auto nSize = rc_trigger_size(sTrigger.c_str());
         if (nSize > 0)
         {
diff --git a/src/data/models/LeaderboardModel.cpp b/src/data/models/LeaderboardModel.cpp
index 234fe637..60b59e0d 100644
--- a/src/data/models/LeaderboardModel.cpp
+++ b/src/data/models/LeaderboardModel.cpp
@@ -325,6 +325,15 @@ void LeaderboardModel::SyncDefinition()
             m_pLeaderboard->lboard = nullptr;
         }
 
+        const auto* pPublishedLeaderboardInfo = pRuntime.GetPublishedLeaderboardInfo(m_pLeaderboard->public_.id);
+        if (pPublishedLeaderboardInfo && memcmp(pPublishedLeaderboardInfo->md5, md5, sizeof(md5)) == 0)
+        {
+            Expects(pPublishedLeaderboardInfo->lboard != nullptr);
+            m_pLeaderboard->lboard = pPublishedLeaderboardInfo->lboard;
+            rc_reset_lboard(m_pLeaderboard->lboard);
+            return;
+        }
+
         const auto nSize = rc_lboard_size(sMemAddr.c_str());
         if (nSize > 0)
         {
diff --git a/src/services/AchievementRuntime.cpp b/src/services/AchievementRuntime.cpp
index b3b74c5d..2b05e586 100644
--- a/src/services/AchievementRuntime.cpp
+++ b/src/services/AchievementRuntime.cpp
@@ -803,6 +803,29 @@ public:
         return DetachMemory(m_pSubsetWrapper->vAllocatedMemory, pMemory);
     }
 
+    const rc_client_achievement_info_t* GetPublishedAchievementInfo(ra::AchievementID nId) const
+    {
+        for (auto* pSubset = m_pPublishedSubset; pSubset; pSubset = pSubset->next)
+        {
+            const auto* pAchievement = FindAchievement(pSubset, nId);
+            if (pAchievement != nullptr)
+                return pAchievement;
+        }
+
+        return nullptr;
+    }
+
+    const rc_client_leaderboard_info_t* GetPublishedLeaderboardInfo(ra::LeaderboardID nId) const
+    {
+        for (auto* pSubset = m_pPublishedSubset; pSubset; pSubset = pSubset->next)
+        {
+            const auto* pLeaderboard = FindLeaderboard(pSubset, nId);
+            if (pLeaderboard != nullptr)
+                return pLeaderboard;
+        }
+
+        return nullptr;
+    }
 };
 
 void AchievementRuntime::SyncAssets()
@@ -850,6 +873,22 @@ rc_trigger_t* AchievementRuntime::GetAchievementTrigger(ra::AchievementID nId) c
     return (achievement != nullptr) ? achievement->trigger : nullptr;
 }
 
+const rc_client_achievement_info_t* AchievementRuntime::GetPublishedAchievementInfo(ra::AchievementID nId) const
+{
+    if (m_pClientSynchronizer != nullptr)
+        return m_pClientSynchronizer->GetPublishedAchievementInfo(nId);
+
+    return nullptr;
+}
+
+const rc_client_leaderboard_info_t* AchievementRuntime::GetPublishedLeaderboardInfo(ra::LeaderboardID nId) const
+{
+    if (m_pClientSynchronizer != nullptr)
+        return m_pClientSynchronizer->GetPublishedLeaderboardInfo(nId);
+
+    return nullptr;
+}
+
 std::string AchievementRuntime::GetAchievementBadge(const rc_client_achievement_t& pAchievement)
 {
     std::string sBadgeName = pAchievement.badge_name;
diff --git a/src/services/AchievementRuntime.hh b/src/services/AchievementRuntime.hh
index d1dbe61b..a7a237f8 100644
--- a/src/services/AchievementRuntime.hh
+++ b/src/services/AchievementRuntime.hh
@@ -63,6 +63,7 @@ public:
     /// Gets the raw trigger for the achievement.
     /// </summary>
     rc_trigger_t* GetAchievementTrigger(ra::AchievementID nId) const noexcept;
+    const rc_client_achievement_info_t* GetPublishedAchievementInfo(ra::AchievementID nId) const;
 
     static std::string GetAchievementBadge(const rc_client_achievement_t& pAchievement);
 
@@ -75,6 +76,7 @@ public:
     /// Gets the raw definition for the leaderboard.
     /// </summary>
     rc_lboard_t* GetLeaderboardDefinition(ra::LeaderboardID nId) const noexcept;
+    const rc_client_leaderboard_info_t* GetPublishedLeaderboardInfo(ra::LeaderboardID nId) const;
 
     void ReleaseLeaderboardTracker(ra::LeaderboardID nId) noexcept;
 
diff --git a/src/ui/viewmodels/MemoryViewerViewModel.cpp b/src/ui/viewmodels/MemoryViewerViewModel.cpp
index aeb8a075..5082e048 100644
--- a/src/ui/viewmodels/MemoryViewerViewModel.cpp
+++ b/src/ui/viewmodels/MemoryViewerViewModel.cpp
@@ -279,15 +279,22 @@ void MemoryViewerViewModel::UpdateColors()
 
 void MemoryViewerViewModel::UpdateInvalidRegions()
 {
+    const auto nVisibleLines = ra::to_unsigned(GetNumVisibleLines());
+    const auto nFirstAddress = GetFirstAddress();
+    const auto nVisibleBytes = std::min(m_nTotalMemorySize - nFirstAddress, nVisibleLines * 16);
+
     const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
     if (pEmulatorContext.HasInvalidRegions())
     {
-        const auto nVisibleLines = GetNumVisibleLines();
-        const auto nFirstAddress = GetFirstAddress();
-
-        for (int i = 0; i < nVisibleLines * 16; ++i)
+        for (unsigned i = 0; i < nVisibleBytes; ++i)
             m_pInvalid[i] = pEmulatorContext.IsValidAddress(nFirstAddress + i) ? 0 : 1;
     }
+    else
+    {
+        memset(m_pInvalid, 0, nVisibleBytes);
+    }
+
+    memset(&m_pInvalid[nVisibleBytes], 1, MaxLines * 16 - nVisibleBytes);
 }
 
 void MemoryViewerViewModel::UpdateHighlight(ra::ByteAddress nAddress, int nNewLength, int nOldLength)
@@ -443,6 +450,7 @@ void MemoryViewerViewModel::OnValueChanged(const IntModelProperty::ChangeArgs& a
             const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
             pEmulatorContext.ReadMemory(nFirstAddress, m_pMemory, gsl::narrow_cast<size_t>(args.tNewValue) * 16);
 
+            UpdateInvalidRegions();
             UpdateColors();
         }
 
@@ -812,10 +820,7 @@ void MemoryViewerViewModel::OnTotalMemorySizeChanged()
             SetFirstAddress(nMaxFirstAddress);
     }
 
-    if (pEmulatorContext.HasInvalidRegions())
-        UpdateInvalidRegions();
-    else
-        memset(m_pInvalid, 0, MaxLines * 16);
+    UpdateInvalidRegions();
 
     ResetSurface();
 }
@@ -1222,7 +1227,7 @@ void MemoryViewerViewModel::RenderMemory()
     const auto nFirstAddress = GetFirstAddress();
     auto nVisibleLines = GetNumVisibleLines();
     if (nFirstAddress + nVisibleLines * 16 > m_nTotalMemorySize)
-        nVisibleLines = (m_nTotalMemorySize - nFirstAddress) / 16;
+        nVisibleLines = (m_nTotalMemorySize - nFirstAddress + 15) / 16;
 
     const bool bBigEndian = (GetSize() == MemSize::ThirtyTwoBitBigEndian);
     const int nWordSpacing = NibblesPerWord() + 1;
@@ -1319,7 +1324,7 @@ void MemoryViewerViewModel::RenderAddresses()
     m_pSurface->FillRectangle(0, s_szChar.Height, s_szChar.Width * 8, nVisibleLines * s_szChar.Height, pEditorTheme.ColorBackground());
 
     if (nFirstAddress + nVisibleLines * 16 > m_nTotalMemorySize)
-        nVisibleLines = (m_nTotalMemorySize - nFirstAddress) / 16;
+        nVisibleLines = (m_nTotalMemorySize - nFirstAddress + 15) / 16;
 
     const wchar_t* sFormat = (m_nTotalMemorySize > 0x01000000) ? L"%08x" : L"0x%06x";
 


Test patch:
diff --git a/tests/services/AchievementRuntime_Tests.cpp b/tests/services/AchievementRuntime_Tests.cpp
index adc151e8..d1727618 100644
--- a/tests/services/AchievementRuntime_Tests.cpp
+++ b/tests/services/AchievementRuntime_Tests.cpp
@@ -552,6 +552,65 @@ public:
         Assert::AreNotEqual(static_cast<const void*>(pNewerTrigger), static_cast<const void*>(pNewestTrigger));
     }
 
+    TEST_METHOD(TestSyncAssetsModifiedCoreAchievement)
+    {
+        AchievementRuntimeHarness runtime;
+        runtime.MockGame();
+
+        auto* pAchievement = runtime.MockAchievement(12345U, "0xH0000=1");
+        pAchievement->public_.category = gsl::narrow_cast<uint8_t>(ra::etoi(ra::data::models::AssetCategory::Core));
+        pAchievement->public_.title = "Achievement Name";
+        pAchievement->public_.description = "Do something cool";
+        pAchievement->public_.points = 25;
+
+        auto vmNewAchievement = std::make_unique<ra::data::models::AchievementModel>();
+        vmNewAchievement->Attach(*pAchievement, ra::data::models::AssetCategory::Core, "0xH0000=1");
+        auto& vmAchievement = reinterpret_cast<ra::data::models::AchievementModel&>(runtime.mockGameContext.Assets().Append(std::move(vmNewAchievement)));
+
+        // SyncAssets should generate the core subset
+        runtime.SyncAssets();
+
+        auto* pSubset = runtime.GetClient()->game->subsets;
+        Expects(pSubset != nullptr);
+        Assert::AreEqual("Game Title", pSubset->public_.title);
+        Assert::AreEqual(1U, pSubset->public_.id);
+        Assert::AreEqual("012345", pSubset->public_.badge_name);
+        Assert::AreEqual(1U, pSubset->public_.num_achievements);
+        Assert::IsTrue(pSubset->active);
+        Assert::IsNull(pSubset->next);
+        const auto* pOriginalTrigger = pSubset->achievements->trigger;
+
+        Assert::AreEqual(std::wstring(L"Achievement Name"), vmAchievement.GetName());
+        Assert::AreEqual(std::wstring(L"Do something cool"), vmAchievement.GetDescription());
+        Assert::AreEqual(25, vmAchievement.GetPoints());
+        Assert::AreEqual(12345U, vmAchievement.GetID());
+        Assert::IsNotNull(pAchievement->trigger);
+
+        // directly modifying the achievement trigger should rebuild the underlying trigger
+        runtime.mockGameContext.Assets().FindAchievement(12345U)->SetTrigger("0xH0000=99"); // force memref allocation
+        runtime.mockGameContext.Assets().FindAchievement(12345U)->SetTrigger("0xH0000=2"); // no memref allocation, can be freed
+        Assert::IsNotNull(pAchievement->trigger);
+        const auto* pNewTrigger = pSubset->achievements->trigger;
+        Assert::AreNotEqual(static_cast<const void*>(pOriginalTrigger), static_cast<const void*>(pNewTrigger));
+
+        runtime.SyncAssets();
+
+        pSubset = runtime.GetClient()->game->subsets;
+        const auto* pNewerTrigger = pSubset->achievements->trigger;
+        Assert::AreEqual(static_cast<const void*>(pNewerTrigger), static_cast<const void*>(pNewTrigger));
+        Assert::IsNull(pNewerTrigger->alternative); // simple check to make sure memory is valid - debug build will fill freed memory with 0xDD
+
+        // revert to the original definition - original trigger should be used
+        runtime.mockGameContext.Assets().FindAchievement(12345U)->RestoreServerCheckpoint();
+
+        runtime.SyncAssets();
+
+        pSubset = runtime.GetClient()->game->subsets;
+        const auto* pNewestTrigger = pSubset->achievements->trigger;
+        Assert::AreNotEqual(static_cast<const void*>(pNewerTrigger), static_cast<const void*>(pNewestTrigger));
+        Assert::AreEqual(static_cast<const void*>(pOriginalTrigger), static_cast<const void*>(pNewestTrigger));
+    }
+
     TEST_METHOD(TestDoFrameTriggerAchievement)
     {
         std::array<unsigned char, 1> memory{ 0x00 };