Port "Improve buy similar matching" (#10284)

spec/System/TestCompareBuySimilar_spec.lua

@@ -2,6 +2,25 @@ describe("Buy similar mod stat matching", function()
 	local bs = LoadModule("Classes/CompareBuySimilar")
 
 	describe("addModEntries mod matching", function()
+		it("prefers the exact Instant Recovery trade stat over its percentage alternative", function()
+			local item = new("Item"):Item("Rarity: Unique\nOlroth's Resolve\nUltimate Life Flask\nImplicits: 0\nInstant Recovery")
+			local entries = bs.addModEntries(item, { { list = item.explicitModLines, type = "explicit" } })
+
+			assert.equal(1, #entries)
+			assert.same({ "explicit.stat_1526933524" }, entries[1].tradeIds)
+			assert.is_nil(entries[1].value)
+			assert.is_false(entries[1].isOption)
+		end)
+
+		it("keeps the numeric stat for partial instant recovery", function()
+			local item = new("Item"):Item("Rarity: Magic\nUltimate Life Flask\nImplicits: 0\n25% of Recovery applied Instantly")
+			local entries = bs.addModEntries(item, { { list = item.explicitModLines, type = "explicit" } })
+
+			assert.equal(1, #entries)
+			assert.same({ "explicit.stat_2503377690" }, entries[1].tradeIds)
+			assert.equal(25, entries[1].value)
+		end)
+
 		it("matches from nothing mods as options", function()
 			local fromNothing = new("Item"):Item([[
 From Nothing
@@ -113,9 +132,9 @@ Implicits: 1
 			main:ClosePopup()
 		end)
 
-		local function openPopup()
-			local item = new("Item"):Item("Rarity: Rare\nTest Ring\nRuby Ring\nImplicits: 0\n+50 to maximum Life")
-			bs.openPopup(item, "Ring", build)
+		local function openPopup(raw, slotName)
+			local item = new("Item"):Item(raw or "Rarity: Rare\nTest Ring\nRuby Ring\nImplicits: 0\n+50 to maximum Life")
+			bs.openPopup(item, slotName or "Ring", build)
 			local controls = main.popups[1].controls
 			searchEnv = getfenv(controls.search.onClick)
 			originalCopy = originalCopy or searchEnv.Copy
@@ -125,6 +144,19 @@ Implicits: 1
 			return controls
 		end
 
+		it("searches for Instant Recovery without a percentage minimum", function()
+			local controls = openPopup("Rarity: Unique\nOlroth's Resolve\nUltimate Life Flask\nImplicits: 0\nInstant Recovery", "Flask 1")
+			controls.mod1Check.state = true
+			controls.mod1Check.changeFunc(true)
+			controls.search.onClick()
+			local queryJson = copiedUrl:match("%?q=(.*)"):gsub("%%(%x%x)", function(hex)
+				return string.char(tonumber(hex, 16))
+			end)
+			local query = require("dkjson").decode(queryJson)
+
+			assert.same({ { type = "and", filters = { { id = "explicit.stat_1526933524" } } } }, query.query.stats)
+		end)
+
 		it("rebuilds the URL when league and listed status change", function()
 			local controls = openPopup()
 			controls.search.onClick()

spec/System/TestTradeHelpers_spec.lua

@@ -81,6 +81,7 @@ describe("TradeHelpers trade hash matching", function()
 			assert.is_true(shouldNegate)
 			assert.equal(1, #ids)
 		end)
+
 		it("detects mods with lua pattern characters correctly", function()
 			-- there is a form of this line which is literally 3.5% without a variable
 			local ids, value = tradeHelpers.findTradeHash("Socketed Gems have +3.5% Critical Hit Chance")
@@ -108,6 +109,48 @@ describe("TradeHelpers trade hash matching", function()
 				HashStats({ "attack_critical_strike_chance_+%", "local_jewel_mod_stats_added_to_notable_passives" })))
 			assert.equal(7, value)
 		end)
+
+		it("picks the canonical stat when the descriptor orders values right to left", function()
+			-- Kitava's Thirst's vestigial implicit. its descriptor is "{1}% chance ... {0} Mana ...",
+			-- so the canonical stat (2) is the first value in the text, not the second
+			local ids, value = tradeHelpers.findTradeHash(
+				"25% chance to Trigger Socketed Spells when you Spend at least 100 Mana on an\nUpfront Cost to Use or Trigger a Skill, with a 0.1 second Cooldown")
+			assert.is_truthy(isValueInArray(ids,
+				HashStats({ "cast_socketed_spells_on_X_mana_spent", "cast_socketed_spells_on_mana_spent_%_chance" })))
+			assert.equal(25, value)
+		end)
+
+		it("picks the canonical stat on a single line right to left descriptor", function()
+			-- "{1}% increased Movement Speed for {0} seconds on Throwing a Trap"
+			local ids, value = tradeHelpers.findTradeHash("15% increased Movement Speed for 9 seconds on Throwing a Trap")
+			assert.is_truthy(isValueInArray(ids,
+				HashStats({ "movement_speed_bonus_when_throwing_trap_ms", "movement_speed_+%_on_throwing_trap" })))
+			assert.equal(15, value)
+		end)
+
+		it("uses the value implied by the limits when the canonical stat has no value group", function()
+			-- this form is only used when the chance is 100, so the chance is left out of the text
+			-- even though it is the canonical stat
+			local ids, value = tradeHelpers.findTradeHash("Flasks gain 2 Charges when you take a Critical Hit")
+			assert.is_truthy(isValueInArray(ids,
+				HashStats({ "gain_flask_charge_when_crit_%", "gain_flask_charge_when_crit_amount" })))
+			assert.equal(100, value)
+		end)
+
+		it("does not repeat a hash when several forms of one stat match", function()
+			local ids = tradeHelpers.findTradeHash("Regenerate 5% of maximum Life per second")
+			assert.equal(1, #ids)
+		end)
+
+		it("matches a descriptor with an indexed group and no canonical stat flag", function()
+			-- this descriptor bundles three stats and only renders {1}, with no canonical stat flag
+			local ids, value = tradeHelpers.findTradeHash("You can only Socket 1 Emerald Jewel in this item")
+			assert.is_truthy(isValueInArray(ids, HashStats({
+				"local_can_socket_x_ruby_jewels_exclude_disallowed_types",
+				"local_can_socket_x_emerald_jewels_exclude_disallowed_types",
+				"local_can_socket_x_sapphire_jewels_exclude_disallowed_types" })))
+			assert.equal(1, value)
+		end)
 	end)
 	describe("findTradeIdOption", function()
 		it("matches a '#'-valued option and returns its value", function()

src/Classes/CompareBuySimilar.lua

@@ -9,11 +9,11 @@ local dkjson = require "dkjson"
 local tradeHelpers = LoadModule("Classes/TradeHelpers")
 local tradeStats = tradeHelpers.getTradeStats()
 
--- used to check what stats actually exist on the trade site.
+-- Map available trade IDs to their canonical text.
 local existingStats = {}
 for _, cat in ipairs(tradeStats or {}) do
 	for _, entry in ipairs(cat.entries) do
-		existingStats[entry.id] = true
+		existingStats[entry.id] = entry.text:lower()
 	end
 end
 
@@ -252,10 +252,20 @@ function M.addModEntries(item, modTypeSources)
 							local resultHashes, value, invert = tradeHelpers.findTradeHash(resolvedLine)
 							-- convert hashes to string ids
 							local resultIds = {}
+							local exactMatch = false
+							local tradeLine = resolvedLine:gsub("\n", " "):lower()
 							if resultHashes then
 								for idx = 1, #resultHashes do
 									local id = string.format("%s.stat_%s", source.type, resultHashes[idx])
-									if existingStats[id] then
+									if existingStats[id] == tradeLine then
+										-- Prefer literal trade text over alternate numeric forms, e.g. Instant Recovery.
+										if not exactMatch then
+											resultIds = {}
+											value, invert = nil, false
+											exactMatch = true
+										end
+										t_insert(resultIds, id)
+									elseif existingStats[id] and not exactMatch then
 										t_insert(resultIds, id)
 									end
 								end

src/Classes/TradeHelpers.lua

@@ -4,32 +4,64 @@
 -- Stateless trade mod lookup/matching and item display helper functions
 --
 local m_floor = math.floor
-local statDescData = require("Data.StatDescriptions.stat_descriptions")
 
--- precalculate patterns used for matching stat lines
+-- The stat description data is big and is only needed once a trade lookup actually runs,
+-- so it and its precalculated patterns are built lazily
 local numberPattern = "%%d%+%%.%?%%d*"
-for _, statDescEntry in ipairs(statDescData) do
-	for _, desc in ipairs(statDescEntry[1] or {}) do
-		desc.pat = desc.text
-			-- ignore uppercase letters to help custom items match
-			:lower()
-			-- remove minus and plus signs
-			:gsub("%-{", "{")
-			:gsub("%+{", "{")
-			-- escape existing characters
-			:gsub("([%(%)%.%%%+%-%*%?%[%]%^%$])", "%%%1")
-			-- match # to # as one block since the trade site uses the midpoint. these don't seem to
-			-- ever have plus or minus signs, and can't be negative as even flat damage turns into
-			-- flat damage against you instead of being negative
-			:gsub("{.-} to {.-}", string.format("(%s to %s)", numberPattern, numberPattern))
-
-			-- match number variables like {}, {0}, {0:-d}, {0:+d}, or {:d}
-			:gsub("{.-}",
-				-- and add optional plus and number signs. this is not necessarily correct as some
-				-- stats do require the plus sign to parse, but this simplifies handling reflected
-				-- mods
-				"%%%+%?(%%%-%?" .. numberPattern .. ")")
+local statDescData
+local function getStatDescData()
+	-- this is not perfect. some currently known issues include:
+	-- death's oath chaos damage line: formatted as a # to # value on trade site which shows 3 to 450. nonsensical
+	-- life flask: immunity frozen/chill. probably caused by pob splitting the mods
+	if statDescData then return statDescData end
+	statDescData = LoadModule("Data/StatDescriptions/stat_descriptions")
+	for _, statDescEntry in ipairs(statDescData) do
+		for _, desc in ipairs(statDescEntry[1] or {}) do
+			-- stat descriptors don't necessarily have the stats ordered from
+			-- left to right. for example a text might have {2} {1}. in this
+			-- case if it also has a canonical stat defined, we need to keep
+			-- track of these group ids to know where the stat actually is. See
+			-- for example:
+			-- "{1}% chance to Trigger Socketed Spells when you Spend at least {0} Life on an\nUpfront Cost to Use or Trigger a Skill, with a 0.1 second Cooldown"
+			local groupIndexes = {}
+			local leftToRightIdx = 1
+			for valueGroup in desc.text:gmatch("{.-}") do
+				local groupIdx = valueGroup:match("{(%d+):?.*}")
+				if groupIdx then
+					table.insert(groupIndexes,
+						-- canonical stat is 1 indexed while group ids are zero indexed
+						tonumber(groupIdx) + 1)
+				else
+					table.insert(groupIndexes, leftToRightIdx)
+				end
+				leftToRightIdx = leftToRightIdx + 1
+			end
+			desc.groupIndexes = groupIndexes
+			-- pob doesn't parse this as a part of other mods
+			desc.text = desc.text:gsub("\nPassage", "")
+			desc.pat = desc.text
+				-- ignore uppercase letters to help custom items match
+				:lower()
+				-- remove minus and plus signs
+				:gsub("%-{", "{")
+				:gsub("%+{", "{")
+				-- escape existing characters
+				:gsub("([%(%)%.%%%+%-%*%?%[%]%^%$])", "%%%1")
+				-- match # to # as one block since the trade site uses the midpoint. these don't seem to
+				-- ever have plus or minus signs, and can't be negative as even flat damage turns into
+				-- flat damage against you instead of being negative
+				:gsub("{.-} to {.-}", string.format("(%s to %s)", numberPattern, numberPattern))
+
+				-- match number variables like {}, {0}, {0:-d}, {0:+d}, or {:d}
+				:gsub("{.-}",
+					-- and add optional plus and number signs. this is not necessarily correct as some
+					-- stats do require the plus sign to parse, but this simplifies handling reflected
+					-- mods
+					"%%%+%?(%%%-%?" .. numberPattern .. ")")
+			desc.pat = "^" .. desc.pat .. "$"
+		end
 	end
+	return statDescData
 end
 
 local M = {}
@@ -66,13 +98,9 @@ function M.modLineValue(line, onlyFromTo)
 	return tonumber(line:match("%-?[%d]+%.?[%d]*"))
 end
 
-local _tradeStats
-
 ---@return table? tradeStats
 function M.getTradeStats()
-	if _tradeStats then return _tradeStats end
-	_tradeStats = LoadModule("Data/TradeSiteStats")
-	return _tradeStats
+	return require("Data.TradeSiteStats")
 end
 
 local _optionTradeStatMap
@@ -159,6 +187,21 @@ function M.findTradeIdOption(modLine, modType)
 	end
 end
 
+-- some forms leave the trade site stat out of their text entirely, because that
+-- form is only used when the stat has a known value, which is recorded in the
+-- form's limits. this is for example common with % chance, where 100% chance
+-- omits the chance text
+---@return number? value
+local function impliedValue(statForm, canonical_stat)
+	local limit = statForm.limit and statForm.limit[canonical_stat or 1]
+	return limit and tonumber(limit[1])
+end
+
+local function insertUniqueHash(resultIds, tradeHash)
+	if not isValueInArray(resultIds, tradeHash) then
+		table.insert(resultIds, tradeHash)
+	end
+end
 -- Helper: find the trade stat ID for a mod line
 ---@param modLine  string
 ---@return table[] results Can include more than one result if the results are ambiguous
@@ -182,17 +225,16 @@ function M.findTradeHash(modLine)
 			break
 		end
 	end
-	for _, statDescEntry in ipairs(statDescData) do
+	for _, statDescEntry in ipairs(getStatDescData()) do
 		local statDescriptions = statDescEntry[1]
 		if not statDescriptions then
 			goto continue
 		end
 		-- by default, the trade site uses the first form listed in the stat descriptions, but there
 		-- can be a flag that says otherwise
-		-- local canonical_line = 1
 		-- the stat descriptions default to using the first stat for the trade site, but this
 		-- flag can define it to be another one
-		local canonical_stat = 1
+		local canonical_stat
 		local canonical_negated = false
 		for statFormIdx, statForm in ipairs(statDescriptions) do
 			local negate = false
@@ -209,7 +251,7 @@ function M.findTradeHash(modLine)
 				end
 			end
 		end
-		for statFormIdx, statForm in ipairs(statDescriptions) do
+		for _, statForm in ipairs(statDescriptions) do
 			local negate = false
 			for _, flag in ipairs(statForm) do
 				if (flag.k == "negate" or flag.k == "negate_and_double") and flag.v == 1 then
@@ -219,28 +261,47 @@ function M.findTradeHash(modLine)
 			-- stat has no variables
 			if modLine == statForm.text:lower() then
 				local tradeHash = HashStats(statDescEntry.stats, extraStat)
-				table.insert(resultIds, tradeHash)
+				insertUniqueHash(resultIds, tradeHash)
 				shouldNegate = false
 				-- it's hard to know the correct value, but many stats have a form with no variables when the chance to do something is 100%. this should assign a value for those
-				value = tonumber(statForm.limit[statFormIdx] and statForm.limit[statFormIdx][1])
+				value = impliedValue(statForm, canonical_stat)
 				goto continue
 			end
 			-- ensure no false positives by requiring a full line match. this is not possible in gmatch as it doesn't support ^
-			if modLine:match("^" .. statForm.pat .. "$") then
+			if modLine:match(statForm.pat) then
 				local idx = 1
-				for match in modLine:gmatch(statForm.pat) do
+				local matchedCanonical = false
+				local matches = { modLine:match(statForm.pat) }
+				for _, match in ipairs(matches) do
 					-- note that if the desired value isn't the first match and this is a # to #,
 					-- this will break as it contains two values. however, there is only a single
 					-- example where # to # are not the first two values currently
 					local number = tonumber(match) or M.modLineValue(match)
-					if number and idx == canonical_stat then
+					-- we assume that the desired trade value is either the
+					-- first value from the left, or defined by a canonical_stat
+					-- flag, which refers to the ids which are in the stat
+					-- descriptor, e.g. {2}
+					if number and (not canonical_stat) or (statForm.groupIndexes[idx] == canonical_stat) then
 						shouldNegate = negate ~= canonical_negated
 						local tradeHash = HashStats(statDescEntry.stats, extraStat)
-						table.insert(resultIds, tradeHash)
+						insertUniqueHash(resultIds, tradeHash)
 						value = number
+						matchedCanonical = true
+						break
 					end
 					idx = idx + 1
 				end
+				-- the canonical stat is missing from the form text: take it
+				-- from the limits. this can be e.g. 100% chance omitting the
+				-- chance.
+				if canonical_stat and not matchedCanonical then
+					local implied = impliedValue(statForm, canonical_stat)
+					if implied then
+						shouldNegate = negate ~= canonical_negated
+						insertUniqueHash(resultIds, HashStats(statDescEntry.stats, extraStat))
+						value = implied
+					end
+				end
 			end
 		end
 		::continue::