fix: only add quest items for active quests, cap to needed count by thanhtong89 · Pull Request #58 · azerothcore/mod-aoe-loot

thanhtong89 · 2026-02-23T08:21:07Z

AOE loot was adding quest items from nearby corpses regardless of whether the player had an active quest requiring them, causing inventory clutter. Now validates with HasQuestForItem(), calculates the exact count still needed, and caps additions accordingly.

Testing

Non-active quest item clutter fix

Ensure you do not have the quest The Everstill Bridge (https://www.wowhead.com/classic/quest=89/the-everstill-bridge).
Then, go kill Redridge Brutes / Mystics gnolls near Lakeshire.
You should no longer see quest item drops from this quest. Currently, AoE looting will deposit these quest items even when you're not actively on the quest.

Looting extra items fix

Accept a quest requiring e.g. 5 of an item. Collect 3 manually so you have 3/5.
Kill a large pack of mobs (5+) that all drop that quest item.
Loot one corpse.
Expected: You receive at most 2 of the quest item (capping you at 5/5). You should not end up with extras beyond what the quest requires.

AOE loot was adding quest items from nearby corpses regardless of whether the player had an active quest requiring them, causing inventory clutter. Now validates with HasQuestForItem(), calculates the exact count still needed, and caps additions accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

pangolp · 2026-02-24T01:18:26Z

In short, it ensures the player doesn't obtain more items than they need to complete the mission, right? Or did I misunderstand? If you could add a series of steps to the pull request for testing it, it would be a bit easier to provide feedback.

thanhtong89 · 2026-02-26T05:20:12Z

I've updated the description with the tests you can perform!

Copilot

Pull request overview

This PR fixes a bug in the AOE loot system where quest items were being added to the player's inventory from nearby corpses regardless of whether the player had an active quest requiring those items, and without respecting the quest's required item count. The fix validates quest ownership, calculates the exact needed count across all active quests, and caps the auto-looted quest items accordingly.

Changes:

Added validation to check if player has an active quest for each quest item before auto-looting
Implemented calculation of the maximum required count across all active quests for each item
Added tracking of owned items plus pending additions to prevent over-collection
Implemented capping logic to limit auto-looted quest items to the exact needed amount

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T07:15:03Z

src/aoe_loot.cpp

    }

-    // Add quest items to player inventory
+    // Add quest items to player inventory (re-check need in case main loot already satisfies quest)


The comment states "re-check need in case main loot already satisfies quest", but this is misleading. Quest items in the main loot window (mainLoot->quest_items) are not automatically added to the player's inventory - they must be manually looted from the window. The re-check only verifies HasQuestForItem() without recalculating the needed count, which doesn't prevent over-collection if items from questItemsToAdd plus items from mainLoot->quest_items exceed the requirement.

Suggested change

// Add quest items to player inventory (re-check need in case main loot already satisfies quest)

// Add quest items directly to player inventory

// Note: this only re-checks that the player still has a quest for the item;

// it does not recalculate the remaining needed count or account for items in mainLoot->quest_items.

Copilot · 2026-02-26T07:15:03Z

src/aoe_loot.cpp

            if ((mainLoot->items.size() + itemsToAdd.size() + mainLoot->quest_items.size() + questItemsToAdd.size()) >= 15)
                break;


The code uses a hardcoded value of 15 for the loot window size limit, but the header file defines MAX_LOOT_ITEMS = 16. For maintainability and consistency, the code should use the named constant. Note that if the intended limit is 15 (0-indexed to 14), the constant should be updated to match, or if the limit is 16, the checks should be updated to use >= 16 instead of >= 15.

Copilot · 2026-02-26T07:15:04Z

src/aoe_loot.cpp

-            questItemsToAdd.push_back(loot->quest_items[i]);
+            LootItem const& questItem = loot->quest_items[i];
+
+            // Skip items the player doesn't need (no active quest or quest already satisfied)


The comment states "no active quest or quest already satisfied", but HasQuestForItem() only checks if the player has an active quest requiring the item. It doesn't check if the quest objective is already satisfied (i.e., player already collected enough items). The satisfaction check happens later at lines 187-188 and 198-199. The comment should be updated to accurately reflect that this check only validates whether the player has an active quest for the item.

Suggested change

// Skip items the player doesn't need (no active quest or quest already satisfied)

// Skip items the player doesn't need (no active quest requiring this item)

Actually, HasQuestForItem() does check both — it verifies q_status.ItemCount[j] < qinfo->RequiredItemCount[j] (PlayerQuest.cpp:2329), so it
returns false when the quest objective is already satisfied. That said, the comments were still imprecise.

Copilot · 2026-02-26T07:15:04Z

src/aoe_loot.cpp

+            // Calculate how many the player still needs across all active quests
+            uint32 maxNeeded = 0;
+            for (uint8 slot = 0; slot < MAX_QUEST_LOG_SIZE; ++slot)
+            {
+                uint32 questId = player->GetQuestSlotQuestId(slot);
+                if (!questId)
+                    continue;
+
+                Quest const* quest = sObjectMgr->GetQuestTemplate(questId);
+                if (!quest)
+                    continue;
+
+                for (uint8 j = 0; j < QUEST_ITEM_OBJECTIVES_COUNT; ++j)
+                {
+                    if (quest->RequiredItemId[j] == questItem.itemid && quest->RequiredItemCount[j] > maxNeeded)
+                        maxNeeded = quest->RequiredItemCount[j];
+                }
+            }


The nested loops at lines 170-185 iterate through all quest slots (typically up to 25) and all quest item objectives (typically 4-6) for each quest item from each corpse. With up to 10 corpses potentially having multiple quest items each, this creates a performance concern: O(corpses × items_per_corpse × quest_slots × objectives_per_quest).

Consider caching the quest requirements at the start of the function. Build a map of itemId -> maxNeeded by scanning the player's active quests once, then reuse this map for all quest items from all corpses. This would reduce the complexity to O(quest_slots × objectives_per_quest + corpses × items_per_corpse).

The loops are bounded by small constants: maxCorpses=10, MAX_QUEST_LOG_SIZE=25, QUEST_ITEM_OBJECTIVES_COUNT=4-6. In practice this is ~1000
iterations worst case — negligible. Adding a cache would increase complexity without meaningful benefit.

Copilot · 2026-02-26T07:15:04Z

src/aoe_loot.cpp

            if ((mainLoot->items.size() + itemsToAdd.size() + mainLoot->quest_items.size() + questItemsToAdd.size()) >= 15)
                break;


The space check includes mainLoot->quest_items.size() in the calculation, but quest items from nearby corpses are added directly to the player's inventory via player->AddItem() at line 233, not to the main loot window. The mainLoot->quest_items represents quest items from the main creature that will appear in the loot window, not items being auto-looted.

This means the space check is incorrectly counting items that won't actually occupy space in the 15-item loot window limit. The check should only consider items that will appear in the loot window: mainLoot->items.size() + itemsToAdd.size().

Copilot · 2026-02-26T07:15:04Z

src/aoe_loot.cpp

+            // Count how many the player already has (including items we're about to add)
+            uint32 ownedCount = player->GetItemCount(questItem.itemid, true);
+            for (auto const& pending : questItemsToAdd)
+            {
+                if (pending.itemid == questItem.itemid)
+                    ownedCount += pending.count;
+            }


The calculation of stillNeeded doesn't account for quest items in the main creature's loot (mainLoot->quest_items). When quest items from nearby corpses are auto-added to the player's inventory at line 233, followed by displaying the loot window at line 237, the player can manually loot additional quest items from the main creature. This can result in exceeding the quest requirement.

Example: Player needs 5 items, has 3. Main creature loot has 2 quest items. Nearby corpse has 2 quest items. The code calculates stillNeeded = 2 and auto-adds 2 from the nearby corpse (player now has 5/5). Then the loot window shows with the main creature's 2 quest items. If the player loots those, they end up with 7/5.

The ownedCount calculation should include quest items from mainLoot->quest_items to prevent this over-collection.

thanhtong89

let me update the loop to include items from teh creature's loot.

thanhtong89 · 2026-02-27T06:37:31Z

src/aoe_loot.cpp

-            questItemsToAdd.push_back(loot->quest_items[i]);
+            LootItem const& questItem = loot->quest_items[i];
+
+            // Skip items the player doesn't need (no active quest or quest already satisfied)


Actually, HasQuestForItem() does check both — it verifies q_status.ItemCount[j] < qinfo->RequiredItemCount[j] (PlayerQuest.cpp:2329), so it
returns false when the quest objective is already satisfied. That said, the comments were still imprecise.

thanhtong89 · 2026-02-27T06:38:05Z

src/aoe_loot.cpp

+            // Calculate how many the player still needs across all active quests
+            uint32 maxNeeded = 0;
+            for (uint8 slot = 0; slot < MAX_QUEST_LOG_SIZE; ++slot)
+            {
+                uint32 questId = player->GetQuestSlotQuestId(slot);
+                if (!questId)
+                    continue;
+
+                Quest const* quest = sObjectMgr->GetQuestTemplate(questId);
+                if (!quest)
+                    continue;
+
+                for (uint8 j = 0; j < QUEST_ITEM_OBJECTIVES_COUNT; ++j)
+                {
+                    if (quest->RequiredItemId[j] == questItem.itemid && quest->RequiredItemCount[j] > maxNeeded)
+                        maxNeeded = quest->RequiredItemCount[j];
+                }
+            }


The loops are bounded by small constants: maxCorpses=10, MAX_QUEST_LOG_SIZE=25, QUEST_ITEM_OBJECTIVES_COUNT=4-6. In practice this is ~1000
iterations worst case — negligible. Adding a cache would increase complexity without meaningful benefit.

- Replace hardcoded magic number 15 with MAX_LOOT_ITEMS constant - Account for mainLoot->quest_items in owned count calculation - Fix misleading comments about HasQuestForItem behavior

sudlud requested a review from Copilot February 26, 2026 07:09

Copilot started reviewing on behalf of sudlud February 26, 2026 07:09 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

thanhtong89 commented Feb 27, 2026

View reviewed changes

fix(Core): address PR review comments for quest item loot

9873543

- Replace hardcoded magic number 15 with MAX_LOOT_ITEMS constant - Account for mainLoot->quest_items in owned count calculation - Fix misleading comments about HasQuestForItem behavior

-    // Add quest items to player inventory (re-check need in case main loot already satisfies quest)
+    // Add quest items directly to player inventory
+    // Note: this only re-checks that the player still has a quest for the item;
+    //       it does not recalculate the remaining needed count or account for items in mainLoot->quest_items.

		if ((mainLoot->items.size() + itemsToAdd.size() + mainLoot->quest_items.size() + questItemsToAdd.size()) >= 15)
		break;

	// Skip items the player doesn't need (no active quest or quest already satisfied)
	// Skip items the player doesn't need (no active quest requiring this item)

Conversation

thanhtong89 commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

pangolp commented Feb 24, 2026

Uh oh!

thanhtong89 commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

thanhtong89 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

thanhtong89 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

thanhtong89 left a comment

Choose a reason for hiding this comment

Uh oh!

thanhtong89 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

thanhtong89 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thanhtong89 commented Feb 23, 2026 •

edited

Loading