DF Manual Rolls: Damage rolls not working in PF2e system

[dogstarrb]

Module Manual Rolls v2.4.0

Describe the issue When rolling damage rolls in PF2e v4.6.x (post IWR release)

To Reproduce Steps to reproduce the behavior:

1. Roll an attack from a weapon or spell (works as it should)
2. Click on 'Damage' in spell/attack card
3. Damage is rolled by system dice, rather than prompting for manual input

Expected behavior Prompt window to input manual dice results should appear before printing damage totals.

[dogstarrb]

Update to Reproduction steps: Sometimes spell card damage prompts correctly; can confirm Scorching Ray single action prompts, Ray of Frost damage does not, Heal 2-action on Living does not

[flamewave000]

Unfortunately PF2 still uses this deprecated synchronous rolls. To deal with this you will need to enable Legacy mode in the module settings.

[stwlam]

Unfortunately PF2 still uses this deprecated synchronous rolls. To deal with this you will need to enable Legacy mode in the module settings.

Friendly FYI: this isn't true and hasn't been for about a year now. Though from looking at the module's current implementation, I suspect support won't be extended unless/until Foundry itself switches to a similar term data structure.

[flamewave000]

@stwlam based on the issues that this user (#453) has reported, PF2e is indeed still using synchronous rolls for their "critical damage rolls" and some "spell damage rolls".

[this-gavagai]

I think I'm beginning to understand the issue here. PF2e is passing the async=true argument to evaluate calls like it is supposed to, but under some circumstances Foundry is ignoring the argument and rolling synchronously anyway.

The problem seems to come from damage formulas with multiple terms. 1d6 is fine but 1d6 + 1 doesn't get picked up by this module. I think that's because formulas with multiple terms get routed through the pf2e class ArithmeticExpression, which under some circumstances will call the evaluate method on DiceTerm objects here:

https://github.com/foundryvtt/pf2e/blob/cb783f811a37a2227b8217a020ee5dbb0c09d626/src/module/system/damage/terms.ts#L174

Since DiceTerm doesn't implement an evaluate method, it falls back onto RollTerm, which contains the following code:

  /* -------------------------------------------- */
  /*  RollTerm Methods                            */
  /* -------------------------------------------- */

  /**
   * Evaluate the term, processing its inputs and finalizing its total.
   * @param {object} [options={}]           Options which modify how the RollTerm is evaluated
   * @param {boolean} [options.minimize=false]    Minimize the result, obtaining the smallest possible value.
   * @param {boolean} [options.maximize=false]    Maximize the result, obtaining the largest possible value.
   * @param {boolean} [options.async=false]       Evaluate the term asynchronously, receiving a Promise as the returned value.
   *                                              This will become the default behavior in version 10.x
   * @returns {RollTerm}                     The evaluated RollTerm
   */
  evaluate({minimize=false, maximize=false, async=false}={}) {
    if ( this._evaluated ) {
      throw new Error(`The ${this.constructor.name} has already been evaluated and is now immutable`);
    }
    this._evaluated = true;
    return async ? this._evaluate({minimize, maximize}) : this._evaluateSync({minimize, maximize});
  }

  /**
   * Evaluate the term.
   * @param {object} [options={}]           Options which modify how the RollTerm is evaluated, see RollTerm#evaluate
   * @returns {Promise<RollTerm>}
   * @private
   */
  async _evaluate({minimize=false, maximize=false}={}) {
    return this._evaluateSync({minimize, maximize});
  }

  /**
   * This method is temporarily factored out in order to provide different behaviors synchronous evaluation.
   * This will be removed in 0.10.x
   * @private
   */
  _evaluateSync({minimize=false, maximize=false}={}) {
    return this;
  }

If I'm understanding this correctly, and it's very possible I'm not, it appears that the RollTerm.evaluate method will call DiceRoll._evaluateSync even when async=true.

[stwlam]

@this-gavagai Doesn't a Die by its lonesome also invariably hit an _evaluateSync method?

[stwlam]

@stwlam based on the issues that this user (#453) has reported, PF2e is indeed still using synchronous rolls for their "critical damage rolls" and some "spell damage rolls".

@flamewave000 All pf2e system rolls are async without exception. I believe the issue with the module is that it expects all dice to be represented in a Roll's top-level terms with the lone exception of PoolTerms. Core Rolls incidentally meet this expectation, but pf2e system DamageRolls contain PoolTerms that may also have constituent Rolls with dice not among their top-level terms.

E.g., a plain Roll with a single PoolTerm:

The same but a DamageRoll:

[this-gavagai]

Doesn't a Die by its lonesome also invariably hit an _evaluateSync method?

Not here in RollTerm, at least not that I'm seeing. As to why, I believe you've hit the nail on the head below.

All pf2e system rolls are async without exception. I believe the issue with the module is that it expects all dice to be represented in a Roll's top-level terms with the lone exception of PoolTerms. Core Rolls incidentally meet this expectation, but pf2e system DamageRolls contain PoolTerms that may also have constituent Rolls with dice not among their top-level terms.

I think this is right. Specifically, every time the wrapped DiceTerm.prototype._evaluateSync method is getting called, it seems the consequence of L95 here: https://github.com/flamewave000/dragonflagon-fvtt/blob/9a307430fdb7b0d9f497f1d9d72ec941b671b237/df-manual-rolls/src/ManualRolls.ts#L91-L97

Notably, when evaluating 1d6 + 1, the property this.rollPrompt is undefined. When evaulating 1d6, however, it is defined properly. I think that might be because the check here:

https://github.com/flamewave000/dragonflagon-fvtt/blob/9a307430fdb7b0d9f497f1d9d72ec941b671b237/df-manual-rolls/src/ManualRolls.ts#L69-L72

...is looking for DiceTerms and not larger groupings, like @stwlam suggested.

[George1044]

I figured out a way to fix this using a recursive method to search for DiceTerms inside the terms instead of relying on the top-level term being a DiceTerm. However, I don't have a Linux device on hand to build the project etc. so I was testing directly on the Foundry data module. This is what the function looks like:

static setRollPromptRecursively(obj, rollPrompt) {   
          if (obj instanceof DiceTerm) {
            // If the object is a DiceTerm, set the rollPrompt
            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)) {
                ManualRolls.setRollPromptRecursively(obj[key], rollPrompt);
              }
            }
          }
        }

You would then replace the following code:

 for (const of this.terms) { 
    if (!(term instanceof DiceTerm)) continue; 
    (<any>term).rollPrompt = rollPrompt; 
 } 

by this:

for (const term of this.terms) {
            ManualRolls.setRollPromptRecursively(term, rollPrompt);
            }

This will make the module work for damage rolls in my experience, so if someone can do a pull request to fix this, feel free.