[#1865] Add dnd5e-specific rolling enrichers

[arbron]

[krbz999]

Seems a bit counter intuitive to me in the case of the 2nd group of examples to use check for what is actually skills.

Is something like /skill acr int 20 possible?

[Fyorl]

Seems a bit counter intuitive to me in the case of the 2nd group of examples to use check for what is actually skills.

Skills are ability checks.

[Fyorl]

This looks good to me, thanks Jeff, only minor suggestions on the code side, though I had some more high-level thoughts:

1. Spellcasting ability checks are fairly frequently called for. Something like [[/check spell 20]] -> DC 20 Spellcasting ability check would be nice to support. Easy enough to add parsing support for but I'm unsure if it's a lot of work on the execution side, so we can save it for follow-up work if necessary.
2. I can imagine maybe wanting some dynamic calculations for the DC. I think that rapidly becomes very complicated in terms of parsing and evaluating though, so I'm very much not suggesting we do it as part of this work.
3. On Zhell's suggestion of supporting [[/skill per str 20]], it's pretty much the equivalent of [[/check]] but with the arguments the other way around. In which case I might suggest we just have [[/skill]] be an alias of [[/check]] and allow the arguments to be in any order. It would cause some issues if there were ever an overlap of ability keys with skill keys, but it makes the API more ergonomic as you don't need to remember the order of arguments. We remove enrichSkill, and enrichCheck becomes:

let ability;
let skill;
let dc;
while ( config.length ) {
  const arg = config.shift();
  if ( arg in CONFIG.DND5E.abilities ) ability = arg;
  else if ( arg in CONFIG.DND5E.skills ) skill = arg;
  else if ( Number.isNumeric(arg) ) dc = Number(arg);
} 

4. With the above changes it also becomes quite easy to support tool checks since the parsing could be handled in the same way but checking against CONFIG.DND5E.toolIds instead. The execution path would need to be a little different though.

[arbron]

Okay, I've merged check and skill so you can use those terms interchangeably and the terms can be in any order. I've also added the ability to use formulas for DCs, as long as they don't include spaces:

[Fyorl]

Thanks Jeff, this looks good to go, I'm just going to kick it over to Atro for final sign off here. In the meantime, did you have any thoughts on the tool checks suggestion? You might have missed it since I edited it into this comment #2500 (review) later. It feels like it would be reasonably easy to add support for it here in enrichCheck, maybe with another /tool alias. It can be follow-up work though.

[arbron]

Thanks Jeff, this looks good to go, I'm just going to kick it over to Atro for final sign off here. In the meantime, did you have any thoughts on the tool checks suggestion? You might have missed it since I edited it into this comment #2500 (review) later. It feels like it would be reasonably easy to add support for it here in enrichCheck, maybe with another /tool alias. It can be follow-up work though.

Didn't catch that before, I'll work on that for a follow up PR.

[arbron]

Updated based on feedback:

[Fyorl]

This is shaping up nicely, thanks Jeff. I've left some additional suggestions around argument parsing that you can consider. Also, two other pieces from discord discussion:

(1) Support for a 'format' option, i.e. format=long or format=short which determines whether the full 'check' or 'saving throw' is appended and part of the clickable link. It seems Atro's preference would be for the default to be format=short (which is what this PR currently defaults to), so we should continue with that so long as there's no objection.

(2) There was some suggestion to allow for the full skill/ability name to be used since the abbreviations can be hard to remember (tools probably don't need this). So we would support, e.g. [[/check dexterity acrobatics]]. There are some open questions as to where we put these keys (on CONFIG? just in enrichers.mjs? exported?) and localisation issues. If the user changes their language, the check should not suddenly stop being enriched, for example.

My suggestion, then, since the relevant CONFIG objects are already full objects, is that we just append something like fullKey as a hard-coded English key (much like tools already have) that we can reference here:

DND5E.skills = {
  acr: { label: "DND5E.SkillAcr", ability: "dex", fullKey: "acrobatics" }
};

And then we perform some static initialisation in enrichers.mjs at the top of the file:

import DND5E from "./config.mjs";

const fullSkills = Object.fromEntries(Object.values(DND5E.skills).map(skl => [skl.fullKey, skl]));

[Fyorl]

An addendum to the above that I forgot that was also discussed on discord: If we add boolean parameters that will allow us to add the passive option that converts a check to a passive description (this would not need to roll anything, it would just be for display/localisation purposes).

[arbron]

An addendum to the above that I forgot that was also discussed on discord: If we add boolean parameters that will allow us to add the passive option that converts a check to a passive description (this would not need to roll anything, it would just be for display/localisation purposes).

[Ikaguia]

This looks real nice, would it be possible to add an additional parameter to force a specific roll mode when clicking that tag?
I.E: [[/check ability=wis skill=per rollmode=blind]] or [[/check ability=int skill=arc rollmode=blind]] would allow the dm to easily ask for checks that the players don't know the result. (if no rollmode was set, it would default to using the user's default roll mode.

[Fyorl]

This looks real nice, would it be possible to add an additional parameter to force a specific roll mode when clicking that tag? I.E: [[/check ability=wis skill=per rollmode=blind]] or [[/check ability=int skill=arc rollmode=blind]] would allow the dm to easily ask for checks that the players don't know the result. (if no rollmode was set, it would default to using the user's default roll mode.

I'm not sure about allowing authors to force a particular roll mode. Different DMs have different preferences around 'rolling in the open' or not, and I imagine they would find it quite annoying if they are forced to do it one way or the other because whoever wrote the content forced an explicit roll mode.

[Fyorl]

This looks great, thank you Jeff. Just one or two suggestions, which you can take or leave, feel free to merge when ready.

[arbron]

This looks real nice, would it be possible to add an additional parameter to force a specific roll mode when clicking that tag?
I.E: [[/check ability=wis skill=per rollmode=blind]] or [[/check ability=int skill=arc rollmode=blind]] would allow the dm to easily ask for checks that the players don't know the result. (if no rollmode was set, it would default to using the user's default roll mode.

I think rather than baking that as a requirement for all rolls, we can incorporate that as an option when handling the followup issue #2513 so GMs can decide in the moment what type of roll to request.