fix(Core/Player): Loot Chest eligibility and re-send loot rolls#26469
Conversation
for GameObject Loot chests only: If a player is outside of the instance, when a raid loot chest is opened, that player remains eligible for the loot. The player outside will receive an emblem, and items can be traded. Tested with master loot and group loot
Players not inside the instance will now show up in the list, but result in a "Player not found" UI error message
fix issue where rolls are "lost" if a player disconnects, logs out, leaves the map; or is corpserunning at the moment when a loot chest is opened
github-actions
Bot
added
CORE
📝 WalkthroughWalkthroughGroup loot eligibility in GroupLoot, NeedBeforeGreed, and MasterLoot now accepts players in a gameobject's allowed-looter list in addition to distance-based checks. A new Group::SendPendingRollsToPlayer method resends pending roll-start packets on map entry, and Player::SendLoot pre-fills non-normal loot for allowed looters. ChangesAllowed-looter eligibility and pending roll resend
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/game/Groups/Group.cpp`:
- Around line 1242-1246: The NeedBeforeGreed quest-item roll path in
Group::RollForGroup should use the same allowed-looter fallback as the other
loot roll paths, since it currently only checks IsAtGroupRewardDistance() and
blocks eligible out-of-range players. Update the quest-item loop to mirror the
canLoot logic used elsewhere in Group.cpp by checking
lootedObject->GetAllowedLooters().empty() and then
HasAllowedLooter(playerToRoll->GetGUID()) before deciding whether the player can
roll.
- Around line 901-905: The loot handling in Group::SendLootStart (the branch
using loot->sourceGameObject) should not rely on the raw GameObject* because it
can dangle before pending rolls are sent. Change this path to resolve the object
from sourceWorldObjectGUID, mirroring the creature branch, then use the resolved
GameObject to read m_groupLootTimer and set lootedObject only if the lookup
succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0204b25-e2c8-4a79-92c8-fa10c3c23981
📒 Files selected for processing (3)
src/server/game/Entities/Player/Player.cppsrc/server/game/Groups/Group.cppsrc/server/game/Groups/Group.h
| if (GameObject* go = loot->sourceGameObject) | ||
| { | ||
| remainingTime = go->m_groupLootTimer; | ||
| lootedObject = go; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Loot members used here exist and their types
rg -nP '\b(sourceGameObject|sourceWorldObjectGUID)\b' --type=cpp --type=h -C2 src/server/game/Loot/
ast-grep run --pattern 'class Loot { $$$ }' --lang cpp $(fd -a LootMgr.h) 2>/dev/null | rg -n 'sourceGameObject|sourceWorldObjectGUID' -C1Repository: azerothcore/azerothcore-wotlk
Length of output: 4844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant loot member definitions and nearby usage.
grep -RIn --line-number --context=3 -E '\bsourceGameObject\b|\bsourceWorldObjectGUID\b' src/server/game/Loot src/server/game/Groups/Group.cpp
# Show the Loot definition around the member declarations.
file=$(git ls-files 'src/server/game/Loot/LootMgr.h' | head -n1)
sed -n '300,380p' "$file"
# Show the Group.cpp loot branch in context.
sed -n '880,930p' src/server/game/Groups/Group.cpp
# Find where sourceGameObject is assigned or cleared.
grep -RIn --line-number -E 'sourceGameObject\s*=|sourceGameObject\b.*nullptr|sourceGameObject\b.*=' src/server/game/Loot src/server/game | head -n 50Repository: azerothcore/azerothcore-wotlk
Length of output: 13760
Use sourceWorldObjectGUID instead of loot->sourceGameObject. Loot::sourceGameObject is a raw GameObject*, so this can dangle if the object despawns before pending rolls are sent. Resolve the gameobject by GUID, as in the creature branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Groups/Group.cpp` around lines 901 - 905, The loot handling
in Group::SendLootStart (the branch using loot->sourceGameObject) should not
rely on the raw GameObject* because it can dangle before pending rolls are sent.
Change this path to resolve the object from sourceWorldObjectGUID, mirroring the
creature branch, then use the resolved GameObject to read m_groupLootTimer and
set lootedObject only if the lookup succeeds.
| bool canLoot = playerToRoll->IsAtGroupRewardDistance(lootedObject); | ||
| if (!canLoot && !lootedObject->GetAllowedLooters().empty()) | ||
| canLoot = lootedObject->HasAllowedLooter(playerToRoll->GetGUID()); | ||
|
|
||
| if (canLoot) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/server/game/Groups/Group.cpp"
# Show the relevant regions around the referenced lines and nearby loot-roll logic.
sed -n '1060,1355p' "$file" | cat -nRepository: azerothcore/azerothcore-wotlk
Length of output: 13227
NeedBeforeGreed quest items should honor allowed looters src/server/game/Groups/Group.cpp:247-274
The quest-item loop still gates rolls on IsAtGroupRewardDistance() only. Add the same allowed-looter fallback used in the other roll paths so eligible out-of-range players can roll on NBG quest items.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Groups/Group.cpp` around lines 1242 - 1246, The
NeedBeforeGreed quest-item roll path in Group::RollForGroup should use the same
allowed-looter fallback as the other loot roll paths, since it currently only
checks IsAtGroupRewardDistance() and blocks eligible out-of-range players.
Update the quest-item loop to mirror the canLoot logic used elsewhere in
Group.cpp by checking lootedObject->GetAllowedLooters().empty() and then
HasAllowedLooter(playerToRoll->GetGUID()) before deciding whether the player can
roll.
There was a problem hiding this comment.
Pull request overview
This PR adjusts group loot eligibility and client loot-roll messaging so that players who were present for a kill but are temporarily outside the map/instance can still be treated as eligible (e.g., for chest loot/master loot lists), and adds logic to re-send pending loot roll windows when a player re-enters an instance.
Changes:
- Expands roll/master-loot candidate selection to include players listed in the loot source’s
AllowedLooters, even if they fail distance/map checks. - Adds
Group::SendPendingRollsToPlayer()and calls it on map entry to re-send unresolved roll windows. - For chest group-loot rules, fills non-normal loot visibility for all allowed looters (not only those currently near the chest).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/server/game/Groups/Group.h | Adds SendPendingRollsToPlayer() declaration to support re-sending unresolved roll windows. |
| src/server/game/Groups/Group.cpp | Implements pending-roll re-send; adjusts eligibility checks to consider AllowedLooters for group/master loot. |
| src/server/game/Entities/Player/Player.cpp | Fills loot visibility for allowed looters on chest loot generation; triggers pending-roll re-send on instance entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| canNeed = (player->CanRollForItemInLFG(proto, lootedObject) == EQUIP_ERR_OK); | ||
| } | ||
|
|
||
| SendLootStartRollToPlayer(remainingTime, map->GetId(), player, canNeed, *roll); |
| bool canNeed = true; | ||
| if (GetLootMethod() == NEED_BEFORE_GREED) | ||
| { | ||
| ItemTemplate const* proto = sObjectMgr->GetItemTemplate(roll->itemid); | ||
| if (proto && lootedObject) | ||
| canNeed = (player->CanRollForItemInLFG(proto, lootedObject) == EQUIP_ERR_OK); | ||
| } |
| bool canLoot = playerToRoll->IsAtGroupRewardDistance(lootedObject); | ||
| if (!canLoot && !lootedObject->GetAllowedLooters().empty()) | ||
| canLoot = lootedObject->HasAllowedLooter(playerToRoll->GetGUID()); | ||
|
|
||
| if (canLoot) |
sogladev
changed the title
|
Creature drops passed the test, but gameobject drops are wrong. .go c id 32906 (No quest looting window appeared) AND .go c id 32930 (There's a loot window you can ROLL, but no loot message). I also found another mistake. After A kills the boss(B wasn’t in the instance when killing the boss), when B enters the instance, the team has finished rolling and passed on some items, B can still loot them! Is this correct? I remember that someone who didn't participate in the kill shouldn't be able to loot. |
This is a bug specific to Freya where loot isn't tied to Freya. Freya's Chest is summoned by also reported by copilot: |
Works fine after using #26592. |
|
-- Creature ID 33515 -- Creature ID 32906 -- Creature ID 32930 |
|
During testing, we found that there’s a bug in the biological looting. It should be an error that existed before the PR and has nothing to do with the PR. The PR has been tested. |
|
Items from Thorim's chest, after the rolling process, still fail to appear in players' inventories. Is this the same issue? |
Did this bug happen before this PR? |
Changes Proposed:
This PR proposes changes to:
This contains 3 commits:
AI-assisted Pull Requests
Important
While the use of AI tools when preparing pull requests is not prohibited, contributors must clearly disclose when such tools have been used and specify the model involved.
Contributors are also expected to fully understand the changes they are submitting and must be able to explain and justify those changes when requested by maintainers.
Deepseek V4 Pro
Issues Addressed:
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
2 characters are required
GameObject testing:
.tele horsemenCreature testing:
.tele patchwBoth characters must be present when killing the bosses. But, before looting, teleport the second character outside the map. Then try to award loot, roll, or trade items to the second character.
How to Test the Changes:
Known Issues and TODO List:
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.