From 317a99f252fd874300234c23a1ba65a76c25831b Mon Sep 17 00:00:00 2001 From: Mickael Cagnion Date: Sun, 18 Oct 2026 17:05:30 +0200 Subject: [PATCH 1/4] Make the custom mod browser list independent of table order The mod browser built its list from tree nodes and item mods iterated with pairs(), whose order changes between PoB launches. Two places kept the first value seen: - addModLine kept the first line met for each numeric template, so the shown tier changed between launches ("Minions deal 10%" or "12%"). - addItemMod kept only the first mod met per mod group. Tiers of one group do not always share wording ("reduced"/"increased" damage taken, "Lucky"/"Unlucky" block), so about 60 entries appeared or disappeared depending on the launch. addModLine now keeps the line that sorts first among equally supported lines, which restores the intent of PR 10112, and every item mod line is added, as before PR 10168. The list grows from about 4860 to 5010 entries and is the same on every launch. The new spec runs the browser with pairs forced to ascending and then descending key order and requires the same list. It fails on the previous code and when only the representative rule is removed. The minion expectation returns to 10%, the value PR 10112 tested. --- spec/System/TestCustomModControl_spec.lua | 49 +++++++++++++++++- src/Modules/ConfigModBrowser.lua | 62 ++++++++++++----------- 2 files changed, 79 insertions(+), 32 deletions(-) diff --git a/spec/System/TestCustomModControl_spec.lua b/spec/System/TestCustomModControl_spec.lua index f76eceeff42..84844be3974 100644 --- a/spec/System/TestCustomModControl_spec.lua +++ b/spec/System/TestCustomModControl_spec.lua @@ -54,10 +54,55 @@ describe("Custom modifier controls", function() end end - assert.are.same({ "Minions deal 12% increased Damage" }, minionDamageEntries) + assert.are.same({ "Minions deal 10% increased Damage" }, minionDamageEntries) popup.controls.listControl.selIndex = minionDamageIndex popup.controls.save.onClick() - assert.are.equal("Minions deal 12% increased Damage", blockData.text) + assert.are.equal("Minions deal 10% increased Damage", blockData.text) + end) + + it("lists the same modifiers regardless of source iteration order", function() + local function listModsWithOrderedPairs(descending) + local orderedTables = { } + -- busted runs specs in their own environment; the browser module reads the real global + local originalPairs = _G.pairs + _G.pairs = function(tbl) + orderedTables[tbl] = true + local keys = { } + for key in next, tbl do + table.insert(keys, key) + end + table.sort(keys, function(a, b) + local aKey, bKey = type(a) .. tostring(a), type(b) .. tostring(b) + if descending then + return aKey > bKey + end + return aKey < bKey + end) + local index = 0 + return function() + index = index + 1 + local key = keys[index] + if key ~= nil then + return key, tbl[key] + end + end + end + local ok, popup = pcall(openModBrowser) + _G.pairs = originalPairs + assert(ok, popup) + local mods = { } + for _, mod in ipairs(popup.controls.listControl.list) do + table.insert(mods, { text = mod.text, sources = mod.sources }) + end + main:ClosePopup() + -- the comparison proves nothing unless the browser's own source loops used the ordered pairs + local tree = build.treeTab.specList[build.treeTab.activeSpec].tree + assert.is_true(orderedTables[tree.nodes], "tree nodes were not iterated with the ordered pairs") + assert.is_true(orderedTables[build.data.itemMods], "item mods were not iterated with the ordered pairs") + return mods + end + + assert.are.same(listModsWithOrderedPairs(false), listModsWithOrderedPairs(true)) end) it("orders modifiers alphabetically while ignoring numeric values", function() diff --git a/src/Modules/ConfigModBrowser.lua b/src/Modules/ConfigModBrowser.lua index 2408963bb6f..3549173032a 100644 --- a/src/Modules/ConfigModBrowser.lua +++ b/src/Modules/ConfigModBrowser.lua @@ -60,6 +60,10 @@ local function addModLine(set, line, source, supported) set[template] = modEntry else modEntry.sources[source] = true + -- keep the line that sorts first so the entry doesn't depend on source iteration order + if line < modEntry.text and (supported or not modEntry.supported) then + modEntry.text = line + end end if supported ~= nil then if supported and not modEntry.supported then @@ -68,34 +72,33 @@ local function addModLine(set, line, source, supported) modEntry.supported = modEntry.supported or supported end end ----@param seenGroups table +---@param set table +---@param line string +---@param source string +local function addItemModLine(set, line, source) + -- note that exactly 0.5 range is necessary as that is what the + -- mod cache uses + local rangedLine = itemLib.applyRange(line, 0.5) + local modList, extra = modLib.parseMod(rangedLine) + addModLine(set, rangedLine, source, (not not modList) and not extra) +end ---@param set table ---@param mod table ---@param source string -local function addItemMod(seenGroups, set, mod, source) - if not seenGroups[mod.group] then - seenGroups[mod.group] = true - -- the actual trade hashes aren't relevant, but this - -- field is separated by stat description, which - -- means that getting rid of split lines won't - -- result in accidentally combining two separate - -- stats - if mod.tradeHashes then - for _, statDescription in pairs(mod.tradeHashes) do - local line = table.concat(statDescription, " ") - -- note that exactly 0.5 range is necessary as that is what the - -- mod cache uses - local rangedLine = itemLib.applyRange(line, 0.5) - local modList, extra = modLib.parseMod(rangedLine) - addModLine(set, rangedLine, source, (not not modList) and not extra) - end - else - -- crafted mods have a different format - for _, line in ipairs(mod) do - local rangedLine = itemLib.applyRange(line, 0.5) - local modList, extra = modLib.parseMod(rangedLine) - addModLine(set, rangedLine, source, (not not modList) and not extra) - end +local function addItemMod(set, mod, source) + -- the actual trade hashes aren't relevant, but this + -- field is separated by stat description, which + -- means that getting rid of split lines won't + -- result in accidentally combining two separate + -- stats + if mod.tradeHashes then + for _, statDescription in pairs(mod.tradeHashes) do + addItemModLine(set, table.concat(statDescription, " "), source) + end + else + -- crafted mods have a different format + for _, line in ipairs(mod) do + addItemModLine(set, line, source) end end end @@ -184,29 +187,28 @@ function M.OpenAddModPopup(configTab, blockData) end end end - local recordedGroups = {} if bData.masterMods then for _, mod in ipairs(bData.masterMods) do - addItemMod(recordedGroups, modSet, mod, "Crafted mod") + addItemMod(modSet, mod, "Crafted mod") end end if bData.itemMods then for catName, catMods in pairs(bData.itemMods) do if (catName ~= "Item" and catName ~= "JewelCluster") and type(catMods) == "table" then for _, mod in pairs(catMods) do - addItemMod(recordedGroups, modSet, mod, "Item mod") + addItemMod(modSet, mod, "Item mod") end end end end if bData.veiledMods then for _, mod in pairs(bData.veiledMods) do - addItemMod(recordedGroups, modSet, mod, "Veiled mod") + addItemMod(modSet, mod, "Veiled mod") end end if bData.beastCraft then for _, mod in pairs(bData.beastCraft) do - addItemMod(recordedGroups, modSet, mod, "Item mod") + addItemMod(modSet, mod, "Item mod") end end From 5def70b7461f406b5bce80a6cdbb0192268f54c2 Mon Sep 17 00:00:00 2001 From: Mickael Cagnion Date: Sun, 18 Oct 2026 17:06:02 +0200 Subject: [PATCH 2/4] Cache ranged item mod lines in the custom mod browser With every item mod line now listed, opening the browser runs itemLib.applyRange and modLib.parseMod on about 30,000 lines (18,000 distinct). Keep the ranged line and its support flag per raw line for the session, as parseMod already does for its own results. Measured headless on a new build, first open / later opens: before the previous commit 110 / 84 ms, after it about 250 / 200 ms, with this cache 189 / 52 ms. --- src/Modules/ConfigModBrowser.lua | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Modules/ConfigModBrowser.lua b/src/Modules/ConfigModBrowser.lua index 3549173032a..5a0e06738d0 100644 --- a/src/Modules/ConfigModBrowser.lua +++ b/src/Modules/ConfigModBrowser.lua @@ -72,15 +72,21 @@ local function addModLine(set, line, source, supported) modEntry.supported = modEntry.supported or supported end end +local rangedItemLineCache = {} ---@param set table ---@param line string ---@param source string local function addItemModLine(set, line, source) - -- note that exactly 0.5 range is necessary as that is what the - -- mod cache uses - local rangedLine = itemLib.applyRange(line, 0.5) - local modList, extra = modLib.parseMod(rangedLine) - addModLine(set, rangedLine, source, (not not modList) and not extra) + local ranged = rangedItemLineCache[line] + if not ranged then + -- note that exactly 0.5 range is necessary as that is what the + -- mod cache uses + local rangedLine = itemLib.applyRange(line, 0.5) + local modList, extra = modLib.parseMod(rangedLine) + ranged = { line = rangedLine, supported = (not not modList) and not extra } + rangedItemLineCache[line] = ranged + end + addModLine(set, ranged.line, source, ranged.supported) end ---@param set table ---@param mod table From 1bc63cd1a32d165e7f398d6a797bd6c1d033c07a Mon Sep 17 00:00:00 2001 From: Mickael Cagnion Date: Sun, 18 Oct 2026 17:18:29 +0200 Subject: [PATCH 3/4] Clarify names and comments in the custom mod browser Rename the cache entry to rangedEntry with a rangedLine field, since rangedLine is a plain string elsewhere in the code, and say "table iteration order" instead of "source iteration order" because source already names the origin label passed to addModLine. The addModLine comment now states both rules that pick an entry's representative line. No behavior change. --- spec/System/TestCustomModControl_spec.lua | 12 ++++++------ src/Modules/ConfigModBrowser.lua | 17 ++++++++++------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/spec/System/TestCustomModControl_spec.lua b/spec/System/TestCustomModControl_spec.lua index 84844be3974..446a218de03 100644 --- a/spec/System/TestCustomModControl_spec.lua +++ b/spec/System/TestCustomModControl_spec.lua @@ -60,13 +60,13 @@ describe("Custom modifier controls", function() assert.are.equal("Minions deal 10% increased Damage", blockData.text) end) - it("lists the same modifiers regardless of source iteration order", function() + it("lists the same modifiers regardless of table iteration order", function() local function listModsWithOrderedPairs(descending) - local orderedTables = { } + local iteratedTables = { } -- busted runs specs in their own environment; the browser module reads the real global local originalPairs = _G.pairs _G.pairs = function(tbl) - orderedTables[tbl] = true + iteratedTables[tbl] = true local keys = { } for key in next, tbl do table.insert(keys, key) @@ -95,10 +95,10 @@ describe("Custom modifier controls", function() table.insert(mods, { text = mod.text, sources = mod.sources }) end main:ClosePopup() - -- the comparison proves nothing unless the browser's own source loops used the ordered pairs + -- the comparison only holds if the browser's loops over tree nodes and item mods used the ordered pairs local tree = build.treeTab.specList[build.treeTab.activeSpec].tree - assert.is_true(orderedTables[tree.nodes], "tree nodes were not iterated with the ordered pairs") - assert.is_true(orderedTables[build.data.itemMods], "item mods were not iterated with the ordered pairs") + assert.is_true(iteratedTables[tree.nodes], "tree nodes were not iterated with the ordered pairs") + assert.is_true(iteratedTables[build.data.itemMods], "item mods were not iterated with the ordered pairs") return mods end diff --git a/src/Modules/ConfigModBrowser.lua b/src/Modules/ConfigModBrowser.lua index 5a0e06738d0..05801ce3c7a 100644 --- a/src/Modules/ConfigModBrowser.lua +++ b/src/Modules/ConfigModBrowser.lua @@ -51,7 +51,9 @@ end ---@param line string ---@param source string ---@param supported boolean? --- either adds a new entry or adds the source to an existing entry +-- either adds a new entry or adds the source to an existing entry, keeping one +-- representative line per template: a supported line wins over an unsupported +-- one, and among lines with the same support the one that sorts first wins local function addModLine(set, line, source, supported) local template = getModTemplate(line) local modEntry = set[template] @@ -60,7 +62,7 @@ local function addModLine(set, line, source, supported) set[template] = modEntry else modEntry.sources[source] = true - -- keep the line that sorts first so the entry doesn't depend on source iteration order + -- the text order makes the result independent of table iteration order if line < modEntry.text and (supported or not modEntry.supported) then modEntry.text = line end @@ -72,21 +74,22 @@ local function addModLine(set, line, source, supported) modEntry.supported = modEntry.supported or supported end end +-- raw item mod line -> { rangedLine, supported }, kept for the session local rangedItemLineCache = {} ---@param set table ---@param line string ---@param source string local function addItemModLine(set, line, source) - local ranged = rangedItemLineCache[line] - if not ranged then + local rangedEntry = rangedItemLineCache[line] + if not rangedEntry then -- note that exactly 0.5 range is necessary as that is what the -- mod cache uses local rangedLine = itemLib.applyRange(line, 0.5) local modList, extra = modLib.parseMod(rangedLine) - ranged = { line = rangedLine, supported = (not not modList) and not extra } - rangedItemLineCache[line] = ranged + rangedEntry = { rangedLine = rangedLine, supported = (not not modList) and not extra } + rangedItemLineCache[line] = rangedEntry end - addModLine(set, ranged.line, source, ranged.supported) + addModLine(set, rangedEntry.rangedLine, source, rangedEntry.supported) end ---@param set table ---@param mod table From 53f8fae4fddde71eb6f7b1d42fa0469ce3115f87 Mon Sep 17 00:00:00 2001 From: Mickael Cagnion Date: Sun, 18 Oct 2026 23:20:17 +0200 Subject: [PATCH 4/4] Shorten the mod browser order test The order test sorts keys with one comparator and walks them backwards for the reverse run, compares the displayed lists directly and checks with one assertion that the browser used the replaced pairs. The addModLine header comment also fits on two lines. --- spec/System/TestCustomModControl_spec.lua | 29 ++++++----------------- src/Modules/ConfigModBrowser.lua | 5 ++-- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/spec/System/TestCustomModControl_spec.lua b/spec/System/TestCustomModControl_spec.lua index 446a218de03..b292551e3a6 100644 --- a/spec/System/TestCustomModControl_spec.lua +++ b/spec/System/TestCustomModControl_spec.lua @@ -61,7 +61,7 @@ describe("Custom modifier controls", function() end) it("lists the same modifiers regardless of table iteration order", function() - local function listModsWithOrderedPairs(descending) + local function listModsWithOrderedPairs(reverse) local iteratedTables = { } -- busted runs specs in their own environment; the browser module reads the real global local originalPairs = _G.pairs @@ -71,35 +71,20 @@ describe("Custom modifier controls", function() for key in next, tbl do table.insert(keys, key) end - table.sort(keys, function(a, b) - local aKey, bKey = type(a) .. tostring(a), type(b) .. tostring(b) - if descending then - return aKey > bKey - end - return aKey < bKey - end) - local index = 0 + table.sort(keys, function(a, b) return type(a) .. tostring(a) < type(b) .. tostring(b) end) + local index = reverse and #keys + 1 or 0 return function() - index = index + 1 - local key = keys[index] - if key ~= nil then - return key, tbl[key] - end + index = index + (reverse and -1 or 1) + return keys[index], tbl[keys[index]] end end local ok, popup = pcall(openModBrowser) _G.pairs = originalPairs assert(ok, popup) - local mods = { } - for _, mod in ipairs(popup.controls.listControl.list) do - table.insert(mods, { text = mod.text, sources = mod.sources }) - end main:ClosePopup() - -- the comparison only holds if the browser's loops over tree nodes and item mods used the ordered pairs local tree = build.treeTab.specList[build.treeTab.activeSpec].tree - assert.is_true(iteratedTables[tree.nodes], "tree nodes were not iterated with the ordered pairs") - assert.is_true(iteratedTables[build.data.itemMods], "item mods were not iterated with the ordered pairs") - return mods + assert.is_true(iteratedTables[tree.nodes] and iteratedTables[build.data.itemMods], "the browser did not use the ordered pairs") + return popup.controls.listControl.list end assert.are.same(listModsWithOrderedPairs(false), listModsWithOrderedPairs(true)) diff --git a/src/Modules/ConfigModBrowser.lua b/src/Modules/ConfigModBrowser.lua index 05801ce3c7a..2143336c6f8 100644 --- a/src/Modules/ConfigModBrowser.lua +++ b/src/Modules/ConfigModBrowser.lua @@ -51,9 +51,8 @@ end ---@param line string ---@param source string ---@param supported boolean? --- either adds a new entry or adds the source to an existing entry, keeping one --- representative line per template: a supported line wins over an unsupported --- one, and among lines with the same support the one that sorts first wins +-- either adds a new entry or adds the source to an existing entry +-- the entry keeps a supported line over an unsupported one, then the line that sorts first local function addModLine(set, line, source, supported) local template = getModTemplate(line) local modEntry = set[template]