Repository navigation
Conversation
The unique DB loader skipped injecting the runeforging base variants whenever the item already declared base variants of its own, but it tested newItem.baseLines for that. Item parsing fills baseLines with the item's own base on every parse, so with the enclosing `if newItem.base` already guaranteeing a recognised base, the guard was always true and the injection never ran: 0 of 443 uniques came back with a baseList while 272 were eligible. Test newItem.baseList instead, which stays nil unless a "Base Variant:" line is actually present. The existing variant tests only fed raw text that already declared base variants, exercising the item machinery rather than the loader, so add coverage that goes through main.uniqueDB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Hey, thanks for the report. You're correct that I intended for the variants to be automatic, but it seems this bug went unnoticed in the original PR, because adding them like this is actually incorrect. According to @LocalIdentity it should be done using the Verisium crafts dat table. There's also another issue in the export scripts in that they're keyed by name, which means the uniques that have multiple runemastered base options aren't working in the program in general. I'll have a look at reworking this code in a PR soon |
Thank for pointing this out it. In that case I will close this PR and wait for your proper fix. Btw do you need any help on it? |
3 participants
Regression from #2511 — no issue filed.
Description of the problem being solved:
The Runeforged/Runemastered base variants added in #2511 never appeared on any unique, so the whole feature was inert.
The unique DB loader in
src/Modules/Main.luaskips injecting the runeforging bases when the unique already declares base variants of its own, but it tested that withnewItem.baseLines:ItemClass:ParseRawfillsbaseLineswith the item's own base on every parse (src/Classes/Item.lua,self.baseLines[baseName] = { ... }), and the enclosingif newItem.base thenalready guarantees a base was recognised. SohasBaseVariantswas true for every unique and the injection below it was dead code.newItem.baseListis the field that actually answers the question: it staysnilunless aBase Variant:line is present. The guard now tests that instead. ThenewItem.baseList = newItem.baseList ?? {}a few lines down was both mis-indented and now provably always nil-coalescing, so it became a plain= { }.Measured headless, before the fix: 0 of 443 uniques came back with a
baseList, while 272 were eligible (their base has aRuneforged/Runemasteredcounterpart indata.itemBases). After the fix all 272 get one.The existing tests missed this because
TestItemVariants_spec.luafeeds in raw text that already declaresBase Variant:lines — it exercises the item machinery, which works, and never the loader. The added tests go throughmain.uniqueDB.Steps taken to verify a working solution:
spec/System/TestItemVariants_spec.luathat read frommain.uniqueDBrather than raw text: Wanderlust gets{ "Regular Base", "Runeforged", "Runemastered" }with correctly taggedbaseLinesandselectedBase == 1; the variants survive a raw round trip and selecting base 3 resolves toRunemastered Wrapped Sandals; and every eligible unique gets abaseList. All three fail ondevand pass with the fix.spec/Systemsuite inghcr.io/pathofbuildingcommunity/pathofbuilding-tests:latest: 51 specs, 0 failures, 0 errors.REGENERATE_MOD_CACHE=1 luajit HeadlessWrapper.lua;git diff src/Data/ModCache.luais empty, so parsing the 272 extra base-variant items causes no cache churn.Link to a build that showcases this PR:
N/A — reproduced headlessly; the numbers above come from iterating
main.uniqueDB.list.Before screenshot:
After screenshot:
🤖 Generated with Claude Code