[#1852] Allow for choosing a skill's ability on the character sheet

[Fyorl]

• Closes Allow for selection of skill ability on the character sheet #1852

Unblocks #1760 & #1650

[arbron]

Code looks good, but at least on Firefox the dropdown chevron ends up closer to the modifier than the ability, which is a bit visually confusing:

Perhaps a bit of an extra margin after the <select> would help:

[Varriount]

I feel like this makes the character sheet a tad too noisy - is performing the selection in the roll dialogue really not possible? Alternatively, could the chevrons be hidden until mouseover, or some other visual signal be used?

[Fyorl]

I feel like this makes the character sheet a tad too noisy - is performing the selection in the roll dialogue really not possible?

It's of course possible since that's how it works currently. But it complicates our rolling pipeline, causing us to have to essentially re-prepare the roll again once the dialog is confirmed in case the ability used in the initial preparation changed.

[Varriount]

In case the ability used in the initial preparation changed

What preparation logic is carried out before the dialog is shown?

[arbron]

What preparation logic is carried out before the dialog is shown?

The whole roll formula and all of its data are prepared before hand because it is needed in case the dialog is skipped and to display the proper initial values in the dialog itself.

[arbron]

It might be good to add the ability selection to the skill config dialog also.

[Varriount]

So, just to make sure I understand this change correctly, what are the new workflows for:

• Rolling a single skill check with a non-default ability?
• Setting the default ability for a skill check?

[Fyorl]

So, just to make sure I understand this change correctly, what are the new workflows for:

• Rolling a single skill check with a non-default ability?
• Setting the default ability for a skill check?

You select the ability to use for the skill on the sheet, then roll it. There's no distinction between default or non-default, it will use whichever ability is currently selected when you click to roll it.

[Varriount]

Hm, then I feel like this is a case where code simplification is being done at the expense of user-friendliness:

• A part of the roll workflow is being split off in a way that violates typical expectations of consistency.
  Given that there are dialogues for per-roll skill check configuration and default skill check configuration, users are typically going to expect that all per-roll configuration options will be in the former dialogue, and all default configuration options will be in the latter dialogue.
  I've seen this kind of inconsistency enough with other pieces of software (*cough* Windows *cough*) to be confident in stating that this will annoy users.
• A single UI component (the new selector) is being used for two different tasks.
  The new component is acting both as a default ability selector and a per-roll ability selector. While it performs the former function well-enough, for the latter function the user must always remember to set the skill back to its previous value.
• A UI component with explicitly labelled behavior (in the per-roll skill check configuration) is being swapped for a component with implicit, unlabeled behavior. I have players with varying degrees of technical intuition, and can definitely see them either being confused by this new UI component works, or simply not noticing it. I can even see them knowing to use the component to change the default ability to use for a skill, but not initially thinking to use it to change the skill on a per-roll basis.

[Fyorl]

A part of the roll workflow is being split off in a way that violates typical expectations of consistency.
Given that there are dialogues for per-roll skill check configuration and default skill check configuration, users are typically going to expect that all per-roll configuration options will be in the former dialogue, and all default configuration options will be in the latter dialogue.

Can you expand on this? Which part of the roll workflow is being split off, and which two dialogs are you talking about?

[Varriount]

The two dialogues I'm talking about are the "Configure [Skill Name]" and "[Skill Name] Skill Check" dialogues, respectively:

This PR removes the the "Ability Modifier" field from the "[Skill Name] Skill Check" dialogue and places it on the character sheet. On the sheet, it is meant to function as a way to set the ability modifier to use for a particular skill, for all rolls of that skill.

Before this PR, a user had to perform these steps to override the ability modifier for a single skill check roll:

• Click the skill name.
• In the dialogue that appears, change the "Ability Modifier" selector to the overriding ability, along with any other customizations for that single roll.
• Click "Submit".

After this PR, the user must (for a single skill check roll):

• Select the ability modifier from the drop-down list next to the skill,
• Click the skill name.
• Input any customizations for that single roll.
• Click "Submit".
• Select the previous ability modifier from the drop-down list next to the skill.

These changes remove any way for the user to change the ability modifier for only a single skill check roll, replacing it with a way to change the ability modifier for all skill check rolls (of a particular skill). Furthermore, this functionality's corresponding UI component is not located where similar components are located (in the "Configure [Skill Name]" and "[Skill Name] Skill Check" dialogues).

[Fyorl]

We can (and probably should) add the ability configuration to the skill configuration dialog, Jeff made a similar comment.

I recognise that this is a change to how variable ability skill rolls are performed, though I don't really agree with the implication that it's strictly less user friendly. It's just different, and it has different trade-offs. For example, having the ability selection in the intermediary roll dialog means that it's impossible to perform a fast-forward skill roll with a non-default ability. With this change, fast-forward and non-fast-forward rolls can both have a different ability selected in a consistent way.

One other big point of contention I think I have is with the framing of the intermediary roll dialog as 'the skill check roll dialog'. It's titled as such, but this dialog is actually shared across all ability checks, saving throws and attack rolls, and it's intended for generic modifications to any roll. You'll notice that skill rolls are an aberration here with the option to change something specific like the ability modifier used.

Finally, this feature to adjust the ability modifier used as part of skill checks is actually relatively new. There was no way to do this in the system before (via the UI). With the benefit of hindsight I believe this feature implemented in its current state would not have been approved in the first place (I was the one that approved it), given some of the tech debt it has incurred. So rather than removing the feature entirely, this PR is an attempt to retain it while also smoothing back out some of the wrinkles it has caused in our rolling pipeline. I'm open to any alternative proposals here that might solve this issue in a different way.

[Varriount]

I'd still propose specializing the intermediary skill-check dialogue with the ability modifier to use (for that specific roll). Even with the functionality to change the default ability modifier added in the most recent commit, users are still unable to change the ability modifier in only one-off scenarios - they must always remember to change the ability modifier back to what it was previously set to.

I have real-world experience to back this assertion too. Roll20's 5e character sheet has a similar component for selecting whether rolls should be made normally, with advantage, or with disadvantage:

When a user needs to make a roll with advantage through their character sheet, they must click the "Advantage" toggle, then make the relevant roll. If only that particular roll should have been made with advantage, they then must remember to click the "Normal" toggle after the roll to ensure future rolls are made normally.

I can personally attest that, in my weekly game with 4 players, most sessions involve at least one instance of someone forgetting to set their rolls back to normal.

In this case the potential for annoyance is even worse; it's not just a simple matter of selecting "Normal", but rather selecting the correct ability the skill was originally set to use (which may not even be the "default" ability that the 5e rules dictate).

[Fyorl]

I'd still propose specializing the intermediary skill-check dialogue with the ability modifier to use (for that specific roll).

Assuming this is off the table due to the associated technical cost and the fact that it does not address fast-forward rolls, do you have any additional suggestions?

The only things I've been able to come up with currently are:

1. Indicate, in some way, when a non-default ability is currently selected.
2. Reset the drop-down to the default ability once a roll has been executed.

I think I'd only consider (2) if it could be implemented cleanly, and there are some additional UX considerations that come with that option.

[krbz999]

Indicate, in some way, when a non-default ability is currently selected.

DDB does so by changing the color of the skill to blue.

What if there were keybinding shortcuts for resetting the skill's ability to default? However just having option (2) seems like a decently clean solution that mostly satisfies everyone.

[Varriount]

So, I initially wrote up a long-winded reply explaining what my observations, areas for code improvement, etc. If you still want me to give that, I can, but after writing it all out, I figured it might be better to just get straight to the point.

The root of the problem here is that the roll customization dialogue is created rather late in the skill check process, because it's tied to the D20Roll class. This then ends up complicating things because the D20Roll class's dialogue needs to know what an ability modifier actually is.

So why not extract this logic out? From what I can see, the primary purpose of the D20Roll class is to contain data about a d20 dice formula and evaluate it. It shouldn't be showing the user prompts, nor (arguably) should it be manipulating or reading any other parts of the UI. The only data it should contain is data common to all d20 rolls*.

Splitting this dialogue logic out would simplify both the D20Roll class and the d20roll function, while allowing customization of the prompt.

Just to be clear, I'm not against a default ability selector in a skill's configuration dialogue (which would impact fast-forwarded rolls); I'm against placing them directly on the character sheet, then indicating to users that the selectors are suitable for one-off rolls.

*with some exceptions

[Fyorl]

I'm trying to understand how factoring out the dialog code would solve the core issue of having to re-prepare the roll data if the ability used changes halfway through the roll process. Are you suggesting that we move the dialog to the start of the process in some way? So we immediately check the click event for whether the roll is fast-forwarded or not. If not, we display the dialog first, then do roll preparation after, or if the roll was fast-forwarded we skip the dialog and go straight to roll preparation?

I think it would work but it would mean that we cannot show a preview of the actual bonus values in the dialog. I think it's also quite a disruptive change to the roll workflow that I suspect will break a lot of modules. I won't discount it for that but I think it would mean that we don't do this in 2.1.

[Varriount]

Are you suggesting that we move the dialog to the start of the process in some way? So we immediately check the click event for whether the roll is fast-forwarded or not. If not, we display the dialog first, then do roll preparation after, or if the roll was fast-forwarded we skip the dialog and go straight to roll preparation?

Yes. As I see it, the Actor5e.rollSkill method should be the one creating the dialogue. The existing Alternatively, if you really want to keep the dialogue logic in the D20Roll class, a SkillCheckRoll (and AbilityCheckRoll`) class could be created which overrides the relevant methods.

[T]he core issue of having to re-prepare the roll data if the ability used changes halfway through the roll process

Just to make sure I'm understanding things correctly, where is the re-preparation is being done? Do you mean the logic in Actor5e.rollSkill, d20roll, and D20Roll._onDialogSubmit for dealing with the @abilityCheckBonus term?

[Fyorl]

Just to make sure I'm understanding things correctly, where is the re-preparation is being done? Do you mean the logic in Actor5e.rollSkill, d20roll, and D20Roll._onDialogSubmit for dealing with the @abilityCheckBonus term?

Yes, @abilityCheckBonus is one such example. You can see that we resolve all bonuses in Actor5e#rollSkill, allowing for the roll to be made immediately in the case of a fast-forward roll. But then we have to un-set some of them (like @mod and @abilityCheckBonus) because the user might change the ability modifier being used. This is why, for all other rolls, the intermediary dialog is able to show the exact resolved bonus, but for skill rolls, you see the raw @abilityCheckBonus string in the preview.

We then have to splice the bonus back into the partially-resolved Roll in _onDialogSubmit, and we need to make sure we have enough information to do so. The ability check bonus is relatively simple, but consider Remarkable Athlete which has its own little package of logic that can potentially change the proficiency bonus. We don't really want to carry that logic around with us through the entire roll process, see this comment here. Ideally we want to resolve all the bonuses once up-front, and then the intermediary roll dialog can be concerned only with modifications to the d20 itself.

[Varriount]

Yeah, I definitely feel like this is due to misplaced responsibilities. The Roll class and its descendants should really only be concerned with evaluating dice rolls. If dialogue functionality really needs to be attached to them, it should probably be done through dependency injection.

The broader idea here is to invert the control flow. Instead of having the D20Roll object create the dialogue, have the dialogue create the D20Roll object. This would allow the dialogue to update the formula it displays as the user modifies the UI.

In pseudo-code:

function rollSkill(...) {
    if (fastforward) {
        let roll = new SkillRoll(data, options)
    } else {
        // Could also be a regular function, `skillRollfromDialog(...)`
        let roll = await SkillRoll.fromDialog(data, options)
    }
    return roll.evaluate()
}


class SkillRoll extends D20Roll {
    dialogueFunction = (data, options) => {
        // Create the dialog, then when it closes, return the roll it constructed.
        const dialog = new SkillRollDialog(data, options)
        const roll = await dialog.render(true)
        return roll
    }

    constructor(data, options) {
        // Assemble skill roll formula and terms.
        ...
    }

    static async fromDialog(data, options) {
        await this.dialogueFunction(data, options)
    }
}


class SkillRollDialogue extends FormApplication {
    async _onChangeInput(event) {
        // Or whatever method is called when a from field is changed.
        // Update the roll, then re-render the dialogue to display the new roll.
        this.roll = new SkillRoll(this.data, this.options)
        await this.render(true)
    }
}

As a side benefit, this greatly reduces the the complexity of the current rolling logic, since features like "Elven Accuracy", etc. aren't being handled in multiple places by multiple functions.

[Fyorl]

Yeah, bringing the dialog forward in the pipeline would work. Even if we do that though, the experience for fast-forward rolls isn't as good: You have to open the skill dialog, change the ability score, close the dialog, then make the roll. It erodes any of the time-saving you might have wanted from fast-forwarding the roll in the first place.

So while I understand the concern that the proposal in this PR might make things worse for non-fast-forward rolls, I'm not sure that overall it's a strictly worse design. Ideally we'd have a solution that has the best of both worlds.

Either way, I think this has to be parked for now as it has been taking too long and we have other priorities to get to in 2.1.

[Varriount]

I guess I don't quite see why this needs to be implemented for fast-forward rolls. To me, the whole point of the fast-forward process is to say, "I'm ok with the defaults, and have no need to customize them for this roll".

[p4535992]

I guess I don't quite see why this needs to be implemented for fast-forward rolls. To me, the whole point of the fast-forward process is to say, "I'm ok with the defaults, and have no need to customize them for this roll".

100% agree and I think it should be the logic in general when it comes to fast forward roll to use the default preset settings.

Possibly external modules such as https://github.com/misthero/dnd5e-custom-skills can allow you to prepare multiple versions of the same skill and "customize" the fast forward rolls.