Repository navigation
Conversation
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the problem being solved:
The custom modifier browser (Configuration tab, Custom Modifiers, Add Mod) builds its list by iterating tree nodes and item mod tables with
pairs(), and keeps the first value it meets. LuaJIT seeds string hashing per process, so that order changes every time PoB starts:addModLinekeeps the first line met for each numeric template, so the tier shown for an entry changes between launches. This is what makesTestCustomModControl_spec.lua"collapses numeric tiers into a single entry and imports it" fail intermittently (10% instead of 12%), for example in therun_testsjob of Weighted Score: shared multi-stat sort across PoB #9845.addItemModkeeps only the first mod met per mod group. Tiers of one group do not always share wording ("reduced"/"increased" damage taken, Lucky/Unlucky block, "an additional"/"3 additional" projectiles), so around 60 entries appear or disappear depending on the launch.This PR makes the list the same on every launch:
addModLinenow keeps the line that sorts first among lines with the same template, and a line the parser supports still wins over one it does not. This is the rule Improve disable mods and custom mods box #10112 used ("the retained representative is deterministic"), and the minion test goes back to the 10% that Improve disable mods and custom mods box #10112 expected.Steps taken to verify a working solution:
pairsforced to ascending and then descending key order and requires the same list (text and sources). It fails on the current code, and also when only the representative rule is removed.Before screenshot:
After screenshot: