fix(LuaEngine): Fix quest objective progress not updating in Player:CompleteQuest

src/LuaEngine/methods/PlayerMethods.h

@@ -2820,74 +2820,112 @@ namespace LuaPlayer
 
         Quest const* quest = eObjectMgr->GetQuestTemplate(entry);
 
-        // If player doesn't have the quest
-        if (!quest || player->GetQuestStatus(entry) == QUEST_STATUS_NONE)
+        // Only handle quests that are still incomplete; this also prevents
+        // reviving failed quests and re-triggering completion side effects.
+        if (!quest || player->GetQuestStatus(entry) != QUEST_STATUS_INCOMPLETE)
             return 0;
 
-        // Add quest items for quests that require items
-        for (uint8 x = 0; x < QUEST_ITEM_OBJECTIVES_COUNT; ++x)
+        // Fill quest items by the inventory gap; reward validation ignores bank items.
+        for (uint8 i = 0; i < QUEST_ITEM_OBJECTIVES_COUNT; ++i)
         {
-            uint32 id = quest->RequiredItemId[x];
-            uint32 count = quest->RequiredItemCount[x];
-
-            if (!id || !count)
+            uint32 itemId = quest->RequiredItemId[i];
+            uint32 requiredCount = quest->RequiredItemCount[i];
+            if (!itemId || !requiredCount)
                 continue;
 
-            uint32 curItemCount = player->GetItemCount(id, true);
+            uint32 currentCount = player->GetItemCount(itemId);
+            if (currentCount >= requiredCount)
+                continue;
 
+            uint32 missingCount = requiredCount - currentCount;
             ItemPosCountVec dest;
-            uint8 msg = player->CanStoreNewItem(NULL_BAG, NULL_SLOT, dest, id, count - curItemCount);
+            uint8 msg = player->CanStoreNewItem(NULL_BAG, NULL_SLOT, dest, itemId, missingCount);
             if (msg == EQUIP_ERR_OK)
             {
-                Item* item = player->StoreNewItem(dest, id, true);
-                player->SendNewItem(item, count - curItemCount, true, false);
+                Item* item = player->StoreNewItem(dest, itemId, true);
+                player->SendNewItem(item, missingCount, true, false);
             }
         }
 
-        // All creature/GO slain/cast (not required, but otherwise it will display "Creature slain 0/10")
+        // Items cannot change quest status, but re-check defensively in case of future changes.
+        if (player->GetQuestStatus(entry) != QUEST_STATUS_INCOMPLETE)
+            return 0;
+
+        // Credit creature/GO objectives by the missing count to avoid duplicate
+        // achievement updates and script hooks on repeated calls.
         for (uint8 i = 0; i < QUEST_OBJECTIVES_COUNT; ++i)
         {
-            int32 creature = quest->RequiredNpcOrGo[i];
-            uint32 creatureCount = quest->RequiredNpcOrGoCount[i];
+            int32 objective = quest->RequiredNpcOrGo[i];
+            uint32 requiredCount = quest->RequiredNpcOrGoCount[i];
+            if (!objective || !requiredCount)
+                continue;
+
+            uint32 currentCount = player->GetReqKillOrCastCurrentCount(entry, objective);
+            uint32 missingCount = currentCount < requiredCount ? requiredCount - currentCount : 0;
 
-            if (creature > 0)
+            if (objective > 0)
             {
-                if (CreatureTemplate const* creatureInfo = sObjectMgr->GetCreatureTemplate(creature))
-                    for (uint16 z = 0; z < creatureCount; ++z)
+                if (CreatureTemplate const* creatureInfo = sObjectMgr->GetCreatureTemplate(objective))
+                    for (uint32 count = 0; count < missingCount; ++count)
                         player->KilledMonster(creatureInfo, ObjectGuid::Empty);
             }
-            else if (creature < 0)
-                for (uint16 z = 0; z < creatureCount; ++z)
-                    player->KillCreditGO(creature);
+            else
+            {
+                // RequiredNpcOrGo stores GO entries as negative values; KillCreditGO
+                // expects a positive entry. Negate via int64 to avoid UB on INT32_MIN.
+                uint32 goEntry = static_cast<uint32>(-static_cast<int64>(objective));
+                for (uint32 count = 0; count < missingCount; ++count)
+                    player->KillCreditGO(goEntry);
+            }
         }
 
+        // Player kill objective; the API itself clamps to the actual missing count.
+        if (quest->HasSpecialFlag(QUEST_SPECIAL_FLAGS_PLAYER_KILL))
+            if (uint32 requiredPlayers = quest->GetPlayersSlain())
+                player->KilledPlayerCreditForQuest(requiredPlayers, quest);
+
+        // Exploration/event objective; the API skips it unless the quest is incomplete.
+        if (quest->HasSpecialFlag(QUEST_SPECIAL_FLAGS_EXPLORATION_OR_EVENT))
+            player->AreaExploredOrEventHappens(entry);
 
-        // If the quest requires reputation to complete
-        if (uint32 repFaction = quest->GetRepObjectiveFaction())
+        // Reputation objectives (mirrors cs_quest.cpp): compare as int32 and
+        // cast to float for SetReputation.
+        if (uint32 factionId = quest->GetRepObjectiveFaction())
         {
-            uint32 repValue = quest->GetRepObjectiveValue();
-            uint32 curRep = player->GetReputationMgr().GetReputation(repFaction);
-            if (curRep < repValue)
-                if (FactionEntry const* factionEntry = sFactionStore.LookupEntry(repFaction))
-                    player->GetReputationMgr().SetReputation(factionEntry, repValue);
+            int32 requiredValue = quest->GetRepObjectiveValue();
+            if (player->GetReputationMgr().GetReputation(factionId) < requiredValue)
+            {
+                if (FactionEntry const* faction = sFactionStore.LookupEntry(factionId))
+                    player->GetReputationMgr().SetReputation(faction, static_cast<float>(requiredValue));
+            }
+        }
+
+        if (uint32 factionId = quest->GetRepObjectiveFaction2())
+        {
+            int32 requiredValue = quest->GetRepObjectiveValue2();
+            if (player->GetReputationMgr().GetReputation(factionId) < requiredValue)
+            {
+                if (FactionEntry const* faction = sFactionStore.LookupEntry(factionId))
+                    player->GetReputationMgr().SetReputation(faction, static_cast<float>(requiredValue));
+            }
         }
 
-        // If the quest requires a SECOND reputation to complete
-        if (uint32 repFaction = quest->GetRepObjectiveFaction2())
+        // Fill the money requirement by the gap; a negative value means money the
+        // player must pay. Negate via int64 to avoid UB on INT32_MIN.
+        int32 requiredMoney = quest->GetRewOrReqMoney(player->GetLevel());
+        if (requiredMoney < 0)
         {
-            uint32 repValue2 = quest->GetRepObjectiveValue2();
-            uint32 curRep = player->GetReputationMgr().GetReputation(repFaction);
-            if (curRep < repValue2)
-                if (FactionEntry const* factionEntry = sFactionStore.LookupEntry(repFaction))
-                    player->GetReputationMgr().SetReputation(factionEntry, repValue2);
+            uint32 money = static_cast<uint32>(-static_cast<int64>(requiredMoney));
+            if (player->GetMoney() < money)
+                player->ModifyMoney(money - player->GetMoney());
         }
 
-        // If the quest requires money
-        int32 ReqOrRewMoney = quest->GetRewOrReqMoney();
-        if (ReqOrRewMoney < 0)
-            player->ModifyMoney(-ReqOrRewMoney);
+        // Credit APIs may auto-complete the quest when the last objective is satisfied,
+        // and Player::CompleteQuest is not idempotent (script hooks, aura updates,
+        // quest tracker). Call it only once while the quest is still incomplete.
+        if (player->GetQuestStatus(entry) == QUEST_STATUS_INCOMPLETE)
+            player->CompleteQuest(entry);
 
-        player->CompleteQuest(entry);
         return 0;
     }