Repro/3062 vulconus xophs blood#10030
Open
ethannguyen128 wants to merge 2 commits into
Open
Conversation
…ldingCommunity#3062) Adds a failing system test demonstrating that a permanent source of the Avatar of Fire keystone (as Xoph's Blood grants) does not force Vulconus's 'while you (do not) have Avatar of Fire' state. The character always has Avatar of Fire, yet Condition:HaveAvatarOfFire stays unset (it is only ever set by the Vulconus config checkbox), so the '+2000 Armour while you do not have Avatar of Fire' line remains active. The test asserts the correct behaviour and therefore fails on current code, capturing the bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…athOfBuildingCommunity#3062) PoB gated Vulconus's "while you (do not) have Avatar of Fire" modifiers on Condition:HaveAvatarOfFire, which was only ever set by the Vulconus config checkbox. That checkbox models Vulconus's temporary buff, but a PERMANENT source of the Avatar of Fire keystone (Xoph's Blood, or a tree allocation) means the character always has Avatar of Fire. In that case the condition stayed unset, so the "+2000 Armour while you do not have Avatar of Fire" line remained active even though the character permanently had the keystone. Set Condition:HaveAvatarOfFire whenever the Avatar of Fire keystone is active (env.keystonesAdded), right after keystones are merged in calcs.perform. This mirrors the existing env.keystonesAdded idiom used for other keystone-driven behaviour (CalcDefence, CalcOffence) and leaves the config-checkbox path unchanged, since Vulconus never grants the keystone permanently on its own. Also fixes the repro test: assert.is_falsy(Flag(...)) errors when Flag returns nil (Lua drops the trailing nil, so luassert sees zero args); use assert.is_nil to match the codebase idiom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Hi @Wires77 this is my first contribution to this project. I've fixed #3062 by setting the Condition:HaveAvatarOfFire flag whenever the Avatar of Fire keystone is actually active (e.g. from Xoph's Blood or the passive tree), not just when the Vulconus config checkbox is ticked so Vulconus's "while you (do not) have Avatar of Fire" modifiers now resolve correctly with a permanent keystone source. Would appreciate a review when you have time! |
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.
Fixes # .
Description of the problem being solved:
Vulconus grants Avatar of Fire only temporarily, so PoB gates its "while you (do not) have Avatar of Fire" modifiers behind a Vulconus config checkbox that sets Condition:HaveAvatarOfFire. That flag was set only by the checkbox, so when a permanent source of the Avatar of Fire keystone is equipped (e.g. Xoph's Blood, or a passive-tree allocation) the character always has Avatar of Fire yet the condition stayed unset. As a result Vulconus's +2000 Armour while you do not have Avatar of Fire line remained incorrectly active (and, conversely, the "while you have Avatar of Fire" crit/conversion lines were gated off).
The fix sets Condition:HaveAvatarOfFire whenever the Avatar of Fire keystone is actually active, keyed off env.keystonesAdded immediately after keystones are merged in calcs.perform. This reuses the existing keystone-detection idiom already used in CalcDefence/CalcOffence, and leaves the config-checkbox path untouched. Vulconus never grants the keystone permanently on its own, so the two paths compose without conflict.
Steps taken to verify a working solution:
Added a system test in spec/System/TestDefence_spec.lua reproducing the bug: with Vulconus equipped, adding a permanent Avatar of Fire source (Amber Amulet with the Avatar of Fire line, as Xoph's Blood grants) now sets Condition:HaveAvatarOfFire and drops the +2000 Armour line by exactly 2000. This test fails on the pre-fix code and passes after the fix.
Ran the full Busted suite (docker compose up): 238 passed / 0 failed / 0 errors, confirming no regressions.
Confirmed via the Vulconus unique data that it never grants the keystone permanently, so a Vulconus-only build still responds to the config checkbox exactly as before.
Link to a build that showcases this PR:
https://maxroll.gg/poe/pob/n8puug0t
Before screenshot:
After screenshot: