foundryvtt / dnd5e

An implementation of the 5th Edition game system for Foundry Virtual Tabletop (http://foundryvtt.com).
MIT License
308 stars 212 forks source link

Refactor roll pipeline #2138

Open Fyorl opened 1 year ago

Fyorl commented 1 year ago

There are two main goals for this refactor:

  1. Separate out the various 'options groups' that are passed around as part of a roll. At the moment these are all bundled into a single, huge options parameter. These options fall into three main groups:

    • Options that configure the intermediary roll dialog that prompts users for advantage/normal/disadvantage.
    • Options that are passed to the D20Roll constructor to configure the roll itself.
    • Options that control the chat message that is created as part of the roll.
  2. Iron out the roll pipeline to accommodate a user changing fundamental aspects of a roll in the intermediary roll dialog (e.g. #1530). The current pipeline operates as follows:

    • Dialog data, roll data, and message data is prepared in the roll method, e.g. Actor5e#rollSkill.
    • We instantiate a D20Roll instance with the configuration from the previous step.
    • We check whether the roll is fast-forwarded, and roll immediately if it is, otherwise continue.
    • We spawn an intermediary roll dialog with D20Roll#configureDialog that prompts a user for a situational bonus, advantage or disadvantage, or certain other choices specific to the roll like the ability score to use for the roll.
    • We then perform surgery on the roll terms themselves in D20Roll#_onDialogSubmit in order to splice in any changes that were made in the dialog (like changing the @mod term if the ability score was changed). This is missing some roll-specific logic, like with the remarkable athlete bug above.

There are two draft PRs that tackle this refactor in different ways, so we can decide which one we prefer to proceed with (or none of them). (1) is common to both PRs and is more or less optional, but I feel it makes the code easier to follow and improves the API surface. (2) is where each of the PRs diverge.

2139 is the simpler solution that straightens out the pipeline in a more understandable way, in my view, but has some downsides such as making the formula preview less useful (or removing it entirely).

2140 is a more involved solution that still solves the problem but makes the pipeline a bit harder to follow, in my view.

JPMeehan commented 7 months ago

Want to express support for resolving this issue. Example use case from a module I'm working on that's implementing the Level Up ruleset by EN Publishing: Characters can gain an "Expertise die" as a bonus to skills, ranging from d4 to d8. Some of these benefits are passive, while others are situational. I would like to have a select field ranging from blank to d8, with the initial value based on the passive configuration from the actor's flags. Unfortunately, there's no way to have that data translate either INTO the roll dialog (by smuggling it as the starting value of the situational bonus) or OUT of the roll dialog (with a render-inserted select field)