Skip to content
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

Draft
wants to merge 2 commits into
base: 1.8.x
Choose a base branch
from

Conversation

bakbakbakbakbak
Copy link
Collaborator

@bakbakbakbakbak bakbakbakbakbak commented Mar 28, 2023

In draft until discussions are solved, and the fix is corrected. See comments by @anthonyronda below.

@bakbakbakbakbak bakbakbakbakbak marked this pull request as draft March 28, 2023 05:08
@bakbakbakbakbak bakbakbakbakbak marked this pull request as ready for review March 28, 2023 17:38
@anthonyronda anthonyronda self-requested a review March 28, 2023 18:51
Copy link
Member

@anthonyronda anthonyronda left a 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

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")
Copy link
Member

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)

Copy link
Collaborator Author

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}`
Copy link
Member

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

image

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

@anthonyronda
Copy link
Member

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

@anthonyronda
Copy link
Member

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

@anthonyronda anthonyronda added this to the 2.0.0 milestone Apr 6, 2023
@anthonyronda
Copy link
Member

Don't delete this branch, don't close! I'll make a new enhancement issue as a suggestion for moving this work forward

@bakbakbakbakbak
Copy link
Collaborator Author

I'll leave the branch as-is, with the capital letter roll fix, and just rebase it to the latest 1.8.x.

@bakbakbakbakbak bakbakbakbakbak marked this pull request as draft April 9, 2023 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants