Rarity

Rarity

17M Downloads

Investigate potential issue with collection method

godejord opened this issue ยท 11 comments

commented

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

commented

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 :)

commented

Let's go on a fun adventure!

This is how collectedItemId is used in the codebase:

  1. Addon loading process: Items with it are added to some special list (cache). I'll ignore it.
  2. 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.
  3. Serialization: Don't really care about this.
  4. GUI: Doesn't really pertain to the issue either.
  5. 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
commented

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 strings) 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 ๐Ÿ˜ฉ

commented

Observation:

image

This is what the tooltip says when adding a custom item using the COLLECTION method. To be determined:

  • Can there actually be more than one item that is accumulated? The tooltip suggest no, but the code comments say yes
  • Options.lua says YES, we can enter a CSV list as follows

image

commented

Next odditiy: When adding a custom item, it's not added to the Rarity.items table, which is iterated over to check for collection items. Therefore attempts are never counted.

TBD: Does this also happen for items that are part of the other groups?

image

commented

It IS added to Rarity.collection_items, but the collectedItemId is a table even though only one item was added, and according to the tooltip only one item CAN be added:

image

image

commented

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

commented

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.

commented

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?

commented

You mean #164? No, that's just an uninitialized variable being used to determine the displayed text.

commented

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 ๐Ÿ˜‚