fix: FVTT HP sync writes Max HP modifiers to tempmax, not max (#1158)#1394
Open
0xguy07 wants to merge 1 commit into
Open
fix: FVTT HP sync writes Max HP modifiers to tempmax, not max (#1158)#13940xguy07 wants to merge 1 commit into
0xguy07 wants to merge 1 commit into
Conversation
…to#1158) The dnd5e system stores Max HP modifiers (Aid, Heroes' Feast, Specter life-drain, etc.) in `attributes.hp.tempmax`, while `attributes.hp.max` is the actor's base maximum. Foundry derives the effective displayed max as `max + tempmax`. Before this change, updateHP wrote DDB's `total` directly into `.max` on every sync. That permanently overwrote the actor's base max whenever DDB reported a temporarily-buffed total, and erased the distinction between a real level-up and a 1-hour Aid bump. Refactor the dnd5e payload to compute `tempmax = total - actorMax` and write that to `.tempmax`, leaving `.max` alone. Per actor (not once before the loop), since different tokens of the same name could resolve to actors with different base max values. When `actorMax` is 0 (uninitialized actor — never opened in Foundry), fall back to the previous behavior of writing `.max = total` so the first sync still gives the actor a usable max. The SWS (Star Wars Saga) health path is unchanged — that system has no tempmax equivalent. AI assistance: data-model analysis (dnd5e attributes.hp shape) and the buildDnd5eUpdate refactor drafted with Claude (Opus 4.7). I cannot run Foundry locally to verify behavior in a live session; the logic was checked against the dnd5e public actor data model and matches what the issue reporter described as the expected target field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Stop overwriting the actor's base Max HP in Foundry on every sync. Surface DDB's temporary Max HP modifiers (Aid, Heroes' Feast, Specter life-drain, etc.) as
attributes.hp.tempmax— the field the dnd5e system actually uses for this — so the displayed effective max is right and a real level-up isn't indistinguishable from a 1-hour Aid bump.Type of change
What was AI-assisted: Data-model analysis (dnd5e's
attributes.hp.maxvstempmaxsplit, Foundry's effective-max derivation) and thebuildDnd5eUpdaterefactor were drafted with Claude (Anthropic, Opus 4.7). I (the PR author) read the existingupdateHPflow, identified the two call sites insrc/fvtt/page-script.js, and verified the patch's logic against what the issue reporter described in the screenshots. I cannot run Foundry locally, so behavior is verified against the dnd5e data model documentation — see Reviewer Notes for the specific test checklist.Motivation & context
The dnd5e system splits maximum HP into two attributes:
attributes.hp.max— the actor's base maximum (level-up, racial bonuses, manually-set value).attributes.hp.tempmax— temporary modifier from buffs/debuffs. May be positive (Aid) or negative (Specter life-drain). Foundry computes the displayed effective max asmax + tempmax.updateHPwas unconditionally writing DDB'stotalinto.max. Two consequences:What changed?
src/fvtt/page-script.js,updateHP(lines 473-528 after the patch):dnd5e_dataobject with a per-actorbuildDnd5eUpdate(actorMax)factory. Necessary because different tokens of the same name can resolve to actors with different base max values.attributes.hp.valueandattributes.hp.tempas before, but routes the max delta throughattributes.hp.tempmax = total - actorMaxinstead of overwriting.max.actorMax === 0(uninitialized — never opened or attribute-derived in Foundry yet), the update writes.max = totalso the first sync still gives the actor a usable maximum. This preserves the prior behavior for that specific case and avoids breaking the "link a brand-new Foundry actor to a DDB character" flow.healthpath is unchanged — that system has no tempmax equivalent.No changes to:
src/roll20/page-script.js:9-152, separate code path).hp-updatemessage contract.How to test
Reviewer should verify in live Foundry; my checklist for the things I'd look at:
Aid baseline — Cast Aid on a DDB character. Beyond20 sync should:
+5(the Aid bump).base + 5.Aid expires — Same character, Aid effect drops off in DDB.
0.Specter life-drain (negative modifier) — Foundry should accept a negative tempmax. The displayed max should drop accordingly.
Real level-up — Take a level on DDB.
totalincreases.tempmax = +level_hp_deltarather than incrementing.max. Trade-off: the displayed effective max is correct, but the user has to manually bump their Foundry actor's base.maxon level-up to make the change permanent. This is the standard Foundry workflow when you manage the actor in Foundry directly and is the intended behavior — see Reviewer Notes.Brand-new actor (uninitialized) — Create a fresh actor with
attributes.hp.max = 0, link to a DDB character, trigger a sync. The actor should get.max = total(initialization fallback) rather than tempmax = total.Token-driven sync — Same checklist but with a token on the canvas matching by name. Both branches (
tokens.length == 0actor path, and the per-token loop) go through the samebuildDnd5eUpdate.SWS sheet (non-dnd5e) — Confirm the Star Wars Saga
healthpath still works unchanged.Reviewer notes
.maxonce it's nonzero. The alternative — try to distinguish "level-up" from "buff" — would need either a per-character base-max cache or a heuristic on delta size, neither of which DDB exposes a clean signal for. The simpler trade-off (one-time manual base-max bump in Foundry on level-up) seemed strictly better than the current behavior where a temporary buff permanently changes the actor's max. Happy to revisit if you have a preferred shape.prefix = fvtt_isNewer(fvttVersion, \"10\") ? \"system\" : \"data\"switch, so v9 (data.attributes.hp.tempmax) and v10+ (system.attributes.hp.tempmax) are both addressed without new version logic.updateHPrequires a live Foundry session. I considered adding a small unit test forbuildDnd5eUpdate's payload shape but didn't want to introduce a test framework as a side effect of this fix.@dmportellaor@kakarotowould rather take it directly.