-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(actor): monster hd rolls #372
base: 1.8.x
Are you sure you want to change the base?
Conversation
b1a4299
to
e6fb3e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am so sorry! all of these tests must have been a big effort to write, but this implementation is based on adapting to our current (wrong) implementation of the game rules. As it is we'll have to completely rethink this. See my explanation below
src/module/actor/entity.js
Outdated
if (actorType === "character") { | ||
rollParts.push(actorData.scores.con.mod * actorData.details.level); | ||
const hd = `${actorData.details.level}${actorData.hp.hd.slice( | ||
actorData.hp.hd.indexOf("d") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors on e.g. 2D6
which is a legal diceterm string (Foundry API lowercases it to 2d6
before rolling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, I'll fix that up
)}`; | ||
rollParts.push(hd); | ||
rollParts.push( | ||
`${actorData.scores.con.mod} * ${actorData.details.level}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the discussion here going all the way to the end
https://discord.com/channels/929420928528568390/929511321488203819/1061817746238738482
Particularly here, where the Moldvay Basic wording of the rules is written, and we're given to understand that the OSE rules are meant to emulate the Moldvay Basic rules procedure without repeating them word-for-word
There was a later conversation that I can't find, where I finally understood the rules
In short: our current implementation is actually incorrect, due to me choosing a different reading of the rules than was apparently intended. We should not be multiplying the con bonus by level
What I think should happen when a character calls rollHitDice based on my understanding of the rules: if (actorType === "character") {
rollparts.push(`max(${actorData.hp.hd}+${actorData.scores.con.mod}, 1)`)
....etc |
It's very likely we can port this work over to Character Generator hit dice rolls, given it can generate starting hp for levels >1 |
Don't delete this branch, don't close! I'll make a new enhancement issue as a suggestion for moving this work forward |
I'll leave the branch as-is, with the capital letter roll fix, and just rebase it to the latest 1.8.x. |
e6fb3e7
to
ac5df13
Compare
In draft until discussions are solved, and the fix is corrected. See comments by @anthonyronda below.