Investigate potential issue with collection method
godejord opened this issue ยท 11 comments
Earlier today I tried to add one of the two new mounts that require X amount of item Y to obtain. I am pretty sure I added all the thingys correctly, but I am unable to get any recorded attempts. What be the issue he wonders.
My belief is that one need to use collectedItemId
to set the Y item to collect, and chance
to set the amount X.
Mount: https://ptr.wowhead.com/item=186651/dusklight-razorwing
Code: https://gist.github.com/godejord/07000be473f83814912cd909291b2d4f
Nice one! This should indeed remove the ostacles in theory, but I may have found another problem (who would have thought).
If I interpret the other issue correctly, the new 9.1 collection items are not affected by that. There is no point doing the collection more than once, and to my knowledge, the required items do not even drop after completing the collection in the first place :)
Let's go on a fun adventure!
This is how collectedItemId
is used in the codebase:
- Addon loading process: Items with it are added to some special list (cache). I'll ignore it.
OnBagUpdate
event: Branching paths - is it an item that requires multiple different items? If so, we add up all the counts for all items. If it's just one, we sum up only the count for that item. Then we announce an attempt if more than the required amount has been found... I think, anyway.Serialization
: Don't really care about this.- GUI: Doesn't really pertain to the issue either.
- Database and schema validation: Ditto.
It looks like the code we want to analyse in depth is part 2. And what a thing of beauty it is:
-- Check for an increase in quantity of any items we're watching for
for k, v in pairs(Rarity.bagitems) do
-- Handle collection items
if Rarity.items[k] then
if Rarity.items[k].method == CONSTANTS.DETECTION_METHODS.COLLECTION then
local bagCount = (Rarity.bagitems[k] or 0)
-- Our items hashtable only saves one item for this collected item, so we have to scan to find them all now.
-- Earlier, we pre-built a list of just the items that are COLLECTION items to save some time here.
for kk, vv in pairs(Rarity.collection_items) do
-- This item is a collection of several items; add them all up and check for attempts
if type(vv.collectedItemId) == "table" then
-- This item is a collection of a single type of item
if vv.enabled ~= false then
local total = 0
local originalCount = (vv.attempts or 0)
local goal = (vv.chance or 100)
for kkk, vvv in pairs(vv.collectedItemId) do
if (Rarity.bagitems[vvv] or 0) > 0 then
total = total + Rarity.bagitems[vvv]
end
end
if total > originalCount then
vv.attempts = total
if originalCount < goal and total >= goal then
self:OnItemFound(vv.itemId, vv)
elseif total > originalCount then
self:OutputAttempts(vv)
end
end
end
else
if
vv.enabled and
(vv.collectedItemId == Rarity.items[k].collectedItemId or
table_contains(Rarity.items[k].collectedItemId, vv.collectedItemId))
then
local originalCount = (vv.attempts or 0)
local goal = (vv.chance or 100)
vv.lastAttempts = 0
if vv.attempts ~= bagCount then
vv.attempts = bagCount
end
if originalCount < bagCount and originalCount < goal and bagCount >= goal then
self:OnItemFound(vv.itemId, vv)
elseif originalCount < bagCount then
self:OutputAttempts(vv)
end
end
end
end
end
end
-- Other items
if (Rarity.bagitems[k] or 0) > (Rarity.tempbagitems[k] or 0) then -- An inventory item went up in count
if
Rarity.items[k] and Rarity.items[k].enabled ~= false and
Rarity.items[k].method ~= CONSTANTS.DETECTION_METHODS.COLLECTION
then
self:OnItemFound(k, Rarity.items[k])
end
end
end
I deciphered most parts of this and got an intermediate result, at which point I'm stopping because the comments seem confusing (inverted table/single value checks?):
for item in pairs(bagItems) do
HandleCollectionItem(item)
HandleOtherItem(item)
end
function HandleCollectionItem(item)
if not item:IsCollectionItem() then
return
end
if WeDontKnowWhatTheHeckThisItemIs(item) then
return
end
local bagCount = (Rarity.bagitems[k] or 0)
-- Our items hashtable only saves one item for this collected item, so we have to scan to find them all now.
-- Earlier, we pre-built a list of just the items that are COLLECTION items to save some time here.
for kk, vv in pairs(Rarity.collection_items) do
-- This item is a collection of several items; add them all up and check for attempts
if type(vv.collectedItemId) == "table" then
-- This item is a collection of a single type of item
if vv.enabled ~= false then
local total = 0
local originalCount = (vv.attempts or 0)
local goal = (vv.chance or 100)
for kkk, vvv in pairs(vv.collectedItemId) do
if (Rarity.bagitems[vvv] or 0) > 0 then
total = total + Rarity.bagitems[vvv]
end
end
if total > originalCount then
vv.attempts = total
if originalCount < goal and total >= goal then
self:OnItemFound(vv.itemId, vv)
elseif total > originalCount then
self:OutputAttempts(vv)
end
end
end
else
if
vv.enabled and
(vv.collectedItemId == Rarity.items[k].collectedItemId or
table_contains(Rarity.items[k].collectedItemId, vv.collectedItemId))
then
local originalCount = (vv.attempts or 0)
local goal = (vv.chance or 100)
vv.lastAttempts = 0
if vv.attempts ~= bagCount then
vv.attempts = bagCount
end
if originalCount < bagCount and originalCount < goal and bagCount >= goal then
self:OnItemFound(vv.itemId, vv)
elseif originalCount < bagCount then
self:OutputAttempts(vv)
end
end
end
end
end
function HandleOtherItem(item)
if item:IsCollectionItem() then
return
end
if DidWeHaveLessOfThisItemWhenWeLastChecked(item) then -- This item went up in count
if item:IsTrackingEnabled() and not item:IsCollectionItem() then -- Why another check when we already asserted the 2nd part ?_?
HandleOnItemFoundEvent() -- Adding the attempts, presumably
end
end
end
That's it, two lines of code later I've managed to get the collections working. Remember, children, never do this:
local someTable = { "12345", "23456" }
and then use the values (which are string
s) as keys for another table, if what you actually want to have there is numbers. Instead we can use a proper lookup table:
local someTable = {
[12345] = true,
[23456] = true
}
Not only is it more intuitive and easy to check via if someTable[itemID] then doSomething() end
but the keys will also be integers in both cases and therefore allow lookups without accessing the hash portion (string
keys).
With the default Lua interpreter (I tested only 5.1) the second approach is actually quite a bit slower on its own, but if you add the required tonumber
calls (even localized as upvalue) the first is basically taking twice as long as the second approach overall.
If performance is any concern LuaJIT is going to be the used anyway, and the optimized version takes exactly the same time for both approaches. I haven't compared the memory usage, but who cares about that with LuaJIT's tiny footprint?
Anyway, that's it for today's Lua lesson. Remember to do your homework and eat your vegetables.
Now excuse me while I cry in a corner and then prepare a PR for this ๐ฉ
More weirdness:
/tinspect Rarity.items
shows the collection item exists in the table, but /dump Rarity.items[itemID
returns nil
Since Rarity
is created via AceAddon
, I suspect metatable shenanigans.
Update: Apparently not, since /dump getmetatable(Rarity)
only shows a __tostring
method
Found a problem: The custom items are added with string
key, but the default ones are indexed with a number
. Is this the problem, though?
Update: Okay, this actually got me a bit further, in that the collection logic now at least tries to add the individual amounts. It still fails to actually add them, however.
Oh snap, the madman actually did it. This is some good detective skill right here. Could this also be related to the wrongfully set achievement text?
You mean #164? No, that's just an uninitialized variable being used to determine the displayed text.
I think this removes the obstacle for adding the new 9.1 collection items, correct? @godejord
Please also take a look at #395 and let me know if that's relevant for the items you wanted to add. If so, I'll have to try and fix it ASAP. But if not, we can probably move it to "soon" and pretend it doesn't exist ๐