An assorted list of loot-related bugs and design concerns #28157
Replies: 4 comments 2 replies
-
Ok, I see the point of Still, this could've been achieved in a different way. Coincidentally this is actually what I'm doing with my AoE loot implementation. Using a persistent LootView-of-sorts to store information about the real whereabouts of the items presented in it - which corpses and in which slots.
|
Beta Was this translation helpful? Give feedback.
-
I've traced the origin of that // questitems use the blocked field for other purposes
if (!qitem && item->is_blocked) in The addition of the code that checks for group loot method, though, stems from d583642, and boy do I wish to know what were these "couple of small changes" meant to fix... |
Beta Was this translation helpful? Give feedback.
-
tl;dr, its a mess |
Beta Was this translation helpful? Give feedback.
-
So I've had the misfortune of deciding to implement AoE looting on 3.3.5, the fancy kind, with partial stack looting, with lootview for the player updating automatically when someone else loots any of the corpses they're currently AoE-looting, etc. This, of course, has prompted me to dig deeper into the loot system code, and, frankly, I'm now scared for my sanity.
It took me a while to figure out how all these 6 loot containers work, but I'm still not 100% certain I understood it correctly, so let me explain how I'm currently understanding them, and you can tell me if I'm correct or not.
If
LootItem::needs_quest
- it will be placed inLoot::quest_items
and a reference to it will be added toLoot::PlayerQuestItems
. Except that loot slot, for the purposes of being passed to the client, is needlessly overcomplicated and requires special code everywhere to be dealt with, but more on this later.Otherwise, if
LootItem::freeforall
- it will be placed inLoot::items
and a reference to it will be added toLoot::PlayerFFAItems
.Otherwise, if
!LootItem::conditions.empty()
- it will be placed inLoot::items
and a reference to it will be added toLoot::PlayerNonQuestNonFFAConditionalItems
.Otherwise, it will be placed in
Loot::items
.Loot::PlayerQuestItems
,Loot::PlayerFFAItems
,Loot::PlayerNonQuestNonFFAConditionalItems
exist both to determine if a player is allowed to loot an item (if player's vector is missing the lootslot in question - they aren't allowed to loot it), and to tell if the player has already looted the item BUT ONLY IFLootItem::freeforall
. A quest item can beLootItem::freeforall
too, butLoot::PlayerQuestItems
will be used to store the info about whether the player already looted it or not, instead ofLoot::PlayerFFAItems
. Likewise, if a loot item has!LootItem::conditions.empty()
, but it's alsoLootItem::freeforall
-Loot::PlayerFFAItems
will be used instead ofLoot::PlayerNonQuestNonFFAConditionalItems
.If these assumptions are correct: good, continue reading, if not - stop and correct me immediately.
Now then, I have a lot of questions regarding the implementation.
1. Why is everything so overcomplicated?
Why are quest items being sent to client not with their index inAn assorted list of loot-related bugs and design concerns #28157 (comment)Loot::quest_items
, but with the index of their reference in player's personal vector inLoot::PlayerQuestItems
? As far as I understand client doesn't give a shit about which slots you pass to it, they're used just for communication.Why even store quest items in a different container (An assorted list of loot-related bugs and design concerns #28157 (comment)Loot::quest_items
) than non-quest items (Loot::items
) in the first place? This just requires a lot of extra code to deal with quest items separately, because they need to be both retrieved from a different container, and use a personalized index that first needs to be transformed back to container index.Loot::PlayerQuestItems
,Loot::PlayerFFAItems
,Loot::PlayerNonQuestNonFFAConditionalItems
if seemingly only one is ever used for each item in the loot? Because of thisoperator<<(ByteBuffer, LootView)
now has 4 separate loops instead of 1 loop over all theLoot::items
+Loot::quest_items
elements, which can then checkLootItem::needs_quest
,LootItem::freeforall
andLootItem::conditions
to take a different codepath if needed.2.
How in the world isAn assorted list of loot-related bugs and design concerns #28157 (comment)LootItem::is_blocked
used by quest items?In Player.cpp there is a comment:
// questitems use the blocked field for other purposes
And it indeed seems to be that way. There is a code in
Loot::FillQuestLoot
that does something that I simply cannot understand: it setsLootItem::is_blocked = true
if the player is not in a group or the group's loot method isn't "Group Loot" or "Round Robin".It seems to be used to prevent the item from being counted twice toLoot::unlootedCount
, yetLootItem::is_counted
exists and is being used for that purpose inLoot::FillNonQuestNonFFAConditionalLoot
, so why isLootItem::is_blocked
being used for quest items?LootItem::is_blocked
continues to be used for the purposes of group loot rolling! IfLootItem::follow_loot_rules
then the item will be subject to group rolls, and group rolls useLootItem::is_blocked
to stop the item from being lootable while the roll is ongoing, whatever the code inLoot::FillQuestLoot
is doing seems to clash with group roll's usage of that variable.LootItem::is_blocked
is used inLootView
to make the slot look non-lootable on the client. But that codepath only works for items withLootItem::follow_loot_rules
. So it's meant to, as mentioned above, prevent the item from being lootable during group roll. Which, again, clashes with whateverLoot::FillQuestLoot
is doing, because that functions just setsis_blocked
totrue
regardless of whether the itemfollow_loot_rules
or not.ALL_PERMISSION
the resulting slottype will beLOOT_SLOT_TYPE_ALLOW_LOOT
regardless ofis_blocked
value, but why set it at all then?3. Umm, why does non-quest-non-ffa-conditional loot even have a separate container
Loot::PlayerNonQuestNonFFAConditionalItems
? What is its purpose? Items that get added to it aren'tLootItem::freeforall
, soNotNormalLootItem::is_looted
won't be used to track if each player had looted the item -LootItem::is_looted
will. Is the intention to use it to hide items from players if they failed the condition check? Well, in the "normal loot" loop ofLootView
there's already tests ofLootItem::AllowedForPlayer()
being performed for every item, you could've just removed the checkl.items[i].conditions.empty()
and conditions would then be tested too, and the item would be omitted fromLootView
if the player fails the conditions check.4. Looking at
Loot::LootItemInSlot
, it seems like the players would be allowed to loot the item if the player is NOT found inPlayerFFAItems
andPlayerNonQuestNonFFAConditionalItems
. Maybe the looting in these cases is prohibited elsewhere, but this looks like an exploitable hole? If the player is even able to start looting, they could spoof packets with attempts to loot a slot that's not visible to them but hasn't yet been looted by other players - sois_looted = item->is_looted
would still yieldtrue
- andLoot::LootItemInSlot
will return a valid item, allowing it to be looted.5. I don't think
friend ByteBuffer& operator<<(ByteBuffer& b, LootView const& lv);
inLoot
(and the forward declarations thereof) is even needed, I'm not seeing that operator using any of theLoot
's private fields.6. Is there any point to this code in
Player::LootSlotItem
?:After all, if we reached this point (conditem) that means the item is not FFA, therefore this code from a few lines down will run:
Marking the item itself as looted, thus we kinda shouldn't care about what
PlayerNonQuestNonFFAConditionalItems
says?Yeah, I see that
Loot::LootItemInSlot
overridesLootItem::is_looted
withNotNormalLootItem::is_looted
before it's checked, but, like, is that even a good idea? Shouldn'tLootItem::is_looted
signify that the item has been removed from the loot and the function should immediate return null in that case?7. How on earth do quest items get group rolled without breaking anything? If group roll is starting for a quest item, its index from
Loot::quest_items
gets added toLoot::items.size()
and that's what gets stored inRoll::itemSlot
. And then later, when the roll ends, the item is accessed via this nightmare fuel:LootItem* item = &(roll->itemSlot >= roll->getLoot()->items.size() ? roll->getLoot()->quest_items[roll->itemSlot - roll->getLoot()->items.size()] : roll->getLoot()->items[roll->itemSlot])
. So far so good, right? Buuuuut thenLoot::NotifyItemRemoved
gets called with thatRoll::itemSlot
, which for quest items will ALWAYS BE HIGHER thanitems.size()
. However, when loot was sent to the client, loot slots for quest items were generated PERSONALLY FOR THAT PLAYER. If two quest items drop, and a player could only lootquest_items[1]
but notquest_items[0]
, then the player will seequest_items[1]
with slot=items.size()+0
, not+1
, because in player's personal quest list that item is index 0, it being the only quest item the player can see. And thus, when the roll forquest_items[0]
will end,Loot::NotifyItemRemoved(items.size() + 0)
will be called, which will visually erase THE FIRST QUEST ITEM each player can see, regardless of whether or not it really wasquest_items[0]
. Wrong item disappears from the loot window and the player has to reopen it to see their quest item again.8. There might be a bug in
Player::isAllowedToLoot
. If a player killed a creature while without a group (thuscreature->GetLootRecipientGroup()
will be null), and then was invited to a group -Player::isAllowedToLoot
will start returning false for this corpse. This doesn't have any immediate effect, becausePlayer::isAllowedToLoot
is not checked whenever a player attempts to loot a creature, but if the the creature disappears and reappears in player's vision, or if dynamic flag update is triggered -UNIT_DYNFLAG_LOOTABLE
will start getting stripped inUnit::BuildValuesUpdate
and the client will start displaying the corpse as not lootable. The solution would be to changeto
There are more potential issues, tho. What if the group, that killed the creature, disbands?
Creature::GetLootRecipientGroup
will start returning null, since it tries to retrieve the group fromGroupMgr
by guid. So if a group disbands - the corpse might start being lootable by the player whose turn it was to loot, sinceGetLootRecipient()
will be returning him, butGetLootRecipientGroup()
will be returning null despitem_lootRecipientGroup
not being 0. This can potentially be exploited - if you see that the corpse in the loot is round-robined to you (it says in the tooltip), and you're in a group of two - you can leave the group (thus disbanding it) before looting and grab all the loot for yourself, thus avoiding starting a group roll and risking not winning it. Hell, you can even join another group and open the loot then, it will start a group roll for your new group. Whether all this is the intended behavior - I don't know.9. Why is
ObjectGuid::Empty
being passed toGroup::SendLootRoll
andGroup::SendLootRollWon
??? Why notroll->itemGUID
like in ALL THE OTHER CASES? This prevents the client from correctly finding the ongoing roll. One of the side effects of this is that if that a player doesn't vote on a roll while someone else does, when the roll expires - it won't get removed from the first player's screen, becauseSendLootRollWon
sent him a packet about a roll with empty guid, and so the client cannot figure out which roll just ended and needs to be removed.10. The GUID mentioned in the point above is meant to be the looted object's guid (i.e. creature corpse, player corpse, gameobject, lootable container item). This allows the client to automatically unlock the item in the looted corpse if everyone passes on the roll (mimicing
item->is_blocked = false
on the server) or if someone wins the roll (emphasis on "someone" - loot slot will be unlocked even if it isn't the player who won the roll), thus allowing it to be picked up without reopening the loot to receive a fresh new LootView constructed withis_blocked == false
in mind. This, in turn, will expose the problem that, I see, you already have fixed:Player::StoreLootItem
used to not checkitem->rollWinnerGUID
.11. Group roll straight up doesn't work if ONLY quest-items-the-follow-group-rules dropped, because
Group::CountRollVote
returns ifroll->getLoot()->items.empty()
, but doesn't check forquest_items
.12. When a player leaves the group during a loot roll, there is a code in
Group::RemoveMember
that's meant to remove the player's vote from the roll and attempt to finish the roll if all players (minus the leaver) already cast their votes. The latter does not work.CountRollVote
is called, but the player has already been erased fromroll->playerVote
, thus the check inGroup::CountRollVote
fails and nothing happens. The call toCountRollVote
should just be replaced withBut that, in turn, will expose another problem that is currently being avoided because the aforementioned call does nothing: after
CountTheRoll
the roll will be deleted and erased from the container, thus "corrupting" the iteratorRolls::iterator it
used inGroup::RemoveMember
. Because the container is a vector, it won't wreck the iterator entirely, but it will now be pointing at the next roll in the container. After++it
happens, this will either skip erasing the leaves from the next roll, or, if this was the last roll in the vector, it will escape the vector's boundaries and eventually crash the server.13. There are also seemingly no checks in place to see if the player is even allowed to select that vote type. It probably can't happen under normal circumstances, but with packet spoofing this can allow players in "Need before greed" groups to roll Need on items that they shouldn't be allowed to Need due to not being able to equip the item, or because of
ITEM_FLAG2_CAN_ONLY_ROLL_GREED
.14. This whole spiel about quest items being stored separately from normal items and being addressed via
PlayerQuestItems
creates a problem with quest gameobjects (not entirely sure about TC, but it did on my server): if a player opens a gameobject that contains quest loot for them, but doesn't loot the items, and instead just closes the loot window - now the loot is "bound" to that player viaPlayerQuestItems
- only that player has a reference to it added toPlayerQuestItems
. If another player then attempts to loot it - they will no longer see the item, because it wasn't "generated for them". But they can't force the object to reset its loot either - after all, the object is NOT fully looted, thereforeWorldSession::DoLootRelease
will no clear the loot, just setGO_ACTIVATED
again. If the chest haschestRestockTime
then potentially the issue will be fixed in due time, but most quest chests do not. Automatic respawn of chests 15 mins after they were interacted with isn't implemented either. They will probably remain bugged until the server restarts. Players can abuse this to deny quest loot to others.15. Random selection of a roll winner in case there's a tie isn't implemented. (added in patch 1.3.0)
16. There are no checks of whether the item has already been
is_looted
once the group roll ends. The only thing that saved us from having a glaring loot dupe exploit is the lineitem.count = 0;
inWorldSession::HandleLootMasterGiveOpcode
. Thanks to it, when the group roll finishes, it still succeeds, but the server attempts to add 0 items to the player's inventory and thus nothing bad happens, but if it weren't for it - players could 1. use group loot method 2. start grouproll 3. switch to masterloot method 4. masterloot the item that's currently being rolled to someone 5. finish grouproll and the winner would receive yet another duplicate of the item.17. Commit 3f21b14 changed the code that was supposed to reduce the corpse decay timer by remaining*rate when all the loot is removed, but it instead made the decay timer reset to full*rate. This allows players to wait until the last second before the corpse is meant to despawn, loot it, and thus reset the timer back to the full decay time, effectively increasing the amount of time the corpse stays in the world by 50%, instead of reducing it by 50% (assuming Rate.Corpse.Decay.Looted is the default 0.5).
18. In addition to the point above, the expression
now + m_corpseDelay * decayRate
will be offloat
type (time_t + float
results in afloat
, apparently), and given how unix timestamps are approaching the limit of int32, they are pretty much guaranteed to lose precision when stored in a 32-bit float type. Thusm_corpseRemoveTime
will vary wildly in the result of this code, often wildly (1661725519+0.0f == 1661725568, a difference of 49 seconds).19. Player corpses and bones that can be looted (on BG, BF) store their loot recipient by pointer, not by GUID. If the player that qualified for the loot logs out and back in - they will no longer be able to loot the corpse/bones, because their
Player
has most likely been allocated at another memory address.20. In 3.3.5
loot->RemoveLooter
is being called at the very end ofWorldSession::DoLootRelease
, after a bunch ofif
checks that can end the execution of the function early. This can leave the player permanently stuck in the corpse's looters list if, for example, the distance between them increased beyond interaction distance between the opening and the closure of the loot window - this can happen if either the corpse or the player is on a transport and the transport moves away after the player starts looting, or if the server lags and the player runs away from the corpse before the packets about loot opening reach them (as they will subsequently send the release packet while being away from the corpse) etc.There will probably be more questions as my struggle continues. My AoE looting seems to be working at the moment, but I expect to find many edge cases I didn't anticipate, and I'm also trying to rewrite some code to be less insane (like the aforementioned
LootView
operator with 4 loops - I'm trying to rewrite it to just one), but I'm terrified of discovering that something broke and not even knowing if it was me who broke it, or it never functioned properly in the first place.Beta Was this translation helpful? Give feedback.
All reactions