flamewave000 / dragonflagon-fvtt

DragonFlagon FoundryVTT Modules
BSD 3-Clause "New" or "Revised" License
64 stars 62 forks source link

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

Open dbavirt opened 1 year ago

dbavirt commented 1 year ago

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.

George1044 commented 1 year ago

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 commented 1 year ago

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

Guldred commented 1 year ago

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 commented 1 year ago

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: https://github.com/flamewave000/dragonflagon-fvtt/blob/527ad9cc64388d4e8ca291e1d435d54e2a210f44/df-manual-rolls/src/ManualRolls.ts#L39-L58