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

DF Manual Rolls: roll for melee damage does not use manual rolls, compatibility problem with PF2e Target Damage #488

Open
dbavirt opened this issue Aug 14, 2023 · 4 comments · May be fixed by #493
Assignees
Labels
bug Something isn't working

Comments

@dbavirt
Copy link

dbavirt commented Aug 14, 2023

Notable that DF Manual Rolls 2.4.0 shows a warning that last verified version is 10.288. I'm using Foundry 11 build 307 and Pathfinder2e 5.3.2.

Module
DF Manual Rolls 2.4.0

Describe the issue
Clicking damage or critical buttons in chat does not prompt for manual roll.

When PF2e Target Damage module is enabled, clicking damage or critical buttons in chat does nothing for melee strikes if manual rolls are enabled. Workaround: disable manual rolling, click the required button, re-enable manual rolling.

To Reproduce
Steps to reproduce the behavior:

  1. Ensure DF Manual Rolls is enabled and manual rolling is active.
  2. Trigger a melee attack from a character sheet, note that manual roll is prompted for.
  3. In the chat window, click on damage or critical button.
  4. If PF2e Target Damage module is enabled, the buttons do nothing. If this module is not enabled, automatic dice rolls happen as though manual rolling was deactivated; no manual roll prompt is displayed.

Expected behavior
Manual roll prompt should be displayed when rolling for damage.

@dbavirt dbavirt added the bug Something isn't working label Aug 14, 2023
@George1044
Copy link

I have implemented a fix for this that works for (probably) all dice rolls. What's the process for pushing it so that everyone can use it?

@tcm151
Copy link

tcm151 commented Sep 20, 2023

@George1044 setup a PR that can be reviewed and then approved. Would you mind sharing the solution here?

@Guldred
Copy link

Guldred commented Sep 20, 2023

I have implemented a fix for this that works for (probably) all dice rolls. What's the process for pushing it so that everyone can use it?

Awesome! Can you create a pull request for that? Was quite the annoying bug!

@George1044
Copy link

Done I have setup a PR #493 .

The issue was related to relying on having DiceTerms be on the top-level terms of the initial Roll, whereas PF2e (or maybe other systems as well) have much more complicated data structures. For example, PF2e creates nested ArithmeticExpressions where each expression combines two terms (dice and number or two dice or dice and ArithmeticExpression etc.). I solved it by running a recursive function through the roll's terms to find any ````DiceTerms and adding a rollPrompt``` to these dice.

There was also an issue with how the terms were being evaluated, where it was also only evaluating the top-level term, so I, through the same recursive function, added all RollTerms to be evaluated as well.

The core solution lies here:

static async setRollPromptRecursively(obj: any, rollPrompt: RollPrompt, termsToRoll: RollTerm[]) {
if (obj instanceof RollTerm) {
termsToRoll.push(obj);
}
if (obj instanceof PoolTerm){
return;
}
// If the object is a DiceTerm, set the rollPrompt
if (obj instanceof DiceTerm) {
(<any>obj).rollPrompt = rollPrompt;
} else if (typeof obj === 'object') {
// If the object is an object, recursively check its properties
for (const key in obj) {
if (obj.hasOwnProperty(key)) {
await ManualRolls.setRollPromptRecursively(obj[key], rollPrompt, termsToRoll);
}
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants