Skip to content

adv/dis modifiers not evaluated first #6871

Description

@thatlonelybugbear

In the current dnd5e.mjs, Reliable Talent is implemented by setting roll.options.reliableTalent, which then becomes a native min10 modifier on the d20 term via D20Roll.configureModifiers() / D20Die.applyRange().

If we trigger a Reliable Talent relevant roll with disadvantage, it works as intended, and we end up with a formula of 1d20dismin10.

Because of that, a raw d20 term like 1d20dismin10 would be expected to behave the same way and never keep a disadvantaged result below 10.
However, testing shows that 1d20dismin10 can resolve below 10.

Going through the code base, adv/dis are not included in isValidModifier() which establishes which modifiers can be included by the die string serializer

  /**
   * The die modifiers.
   * @type {string}
   */
  get mods() {
    if ( !this.modifiers ) return "";
    return this.modifiers.reduce((acc, mod) => {
      return acc + (dnd5e.utils.isValidDieModifier(mod) ? mod : "");
    }, "");
  }
/**
 * Return whether a string is a valid reroll, explosion, min, or max dice modifier.
 * @param {string} mod      The modifier to test.
 * @returns {boolean}
 */
export function isValidDieModifier(mod) {
  const regex = {
    reroll: /rr?([0-9]+)?([<>=]+)?([0-9]+)?/i,
    explode: /xo?([0-9]+)?([<>=]+)?([0-9]+)?/i,
    minimum: /(?:min)([0-9]+)/i,
    maximum: /(?:max)([0-9]+)/i,
    dropKeep: /[dk]([hl])?([0-9]+)?/i,
    count: /(?:c[sf])([<>=]+)?([0-9]+)?/i
  };
  return Object.values(regex).some(rgx => rgx.test(mod));
}

Instead adv/dis support is added separately in BasicDie.MODIFIERS

/**
 * Add support for system-specific modifiers to the base die.
 */
class BasicDie extends Die {
  /** @inheritDoc */
  static MODIFIERS = {
    ...super.MODIFIERS,
    adv: "advantage",
    dis: "advantage"
  };

This seems to me a bit inconsistent which might also make a modifier-order/evaluation bug plausible.

/r 1d20dismin10 min10 is completely dropped from the modifiers

Image

/r 1d20min10dis

Image Image

One could still use something like 2d20min10kl and it will work correctly, but on the other hand, not 2d20klmin10 which again drops entirely the min10 from the term modifiers.

Metadata

Metadata

Assignees

Labels

system: diceDice rolling functionality

Type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions