tposney / midi-qol

Other
5 stars 0 forks source link

Use advantage reminder's new source feature #1052

Open tposney opened 1 year ago

tposney commented 1 year ago

In GitLab by @kaelad on Oct 30, 2022, 12:07

A while ago, back in v0.9.49, you started work integrating with Advantage Reminder to show the source of advantage for attacks in it's messages feature. I have since updated that module to improve on that feature. The way AR processes the advantage/disadvantage/critical flags is different than Midi. AR processes the active effect changes whereas Midi applies them to the actor data and then processes that. As such, AR can grab the label of the active effect and show it in a message. See the screenshot on the following page for an example:

https://github.com/kaelad02/adv-reminder/issues/39#issuecomment-1294843433

Right now, Midi is still creating a message to show advantage but because of it's label, it's only showing part of the flag (e.g. Advantage: grants.attack.all) in the message. While accurate, it doesn't help the user know where it came from. What I'm proposing is that we let AR handle the standard flags on active effects, since it can show the source. But there are things that Midi does, in addition, that AR does not: those optional rules like flanking and ranged attacks in melee. I looked at the source once before and saw those tracked in separate booleans. If that's the case, then Midi could easily add this to the message AR shows using labels like Flanking or Ranged attack in melee.

So instead of the code in itemhandling.ts around line 114, that creates the HTML directly, I propose adding those labels to an existing array that AR will show. I'm not 100% sure which code will run first, but AR is filling in properties like dialogOptions.adv-reminder.advantageLabels during the new dnd5e pre hooks (e.g. dnd5e.preRollAttack and dnd5e.preRollAbilitySave). If you add to the properties before those hooks, then AR will merge them. If you add them after, then make sure to merge the arrays if they're already set. Here are the four properties that AR uses to generate the message in the screenshot I shared:

dialogOptions.adv-reminder.advantageLabels     => for advantage
dialogOptions.adv-reminder.disadvantageLabels  => for disadvantage
dialogOptions.adv-reminder.criticalLabels      => for critical hits
dialogOptions.adv-reminder.normalLabels        => for normal hits

As an example, this would generate the message shown in the screenshot. You don't need the full string, just the label of what's granting advantage. AR will generate the rest of the text when preparing the message in it's renderDialog hook.

options.dialogOptions.adv-reminder.advantageLabels = ["Reckless Attack"];

I don't think I'll be able to make an MR for this myself, but I'll be more than happy to help out with code samples, testing, or whatever you might need.

tposney commented 1 year ago

Funnily enough AR was on my list of things to revisit. Thanks for the solution, I'll have a look at it.

tposney commented 1 year ago

I had a look and what you propose is fine. However, there's a subtlety that I don't know how to resolve.

Midi adjudicates advantage based on the value of the flags on the actor (flags.midi-qol.advantage.all for example), which usually come from active effects but don't have to.

When I do the advantage attribution I don't look at the effects, so don't know the reason for the flag being set. This means that advantage will be duplicated (by advantage reminder and by midi) for any active effect based flags. I don't have a great solution for the issue.

One option would be to only add advantage reminder for specific cases, like flanking (where you are not using the convenient effect), but that won't work in general.

I wonder if there is a way to manage combination of the settings, i.e. I pass something indicating the flag that was set as well as a label and if you find that the flag is set by an effect then don't include the midi data?

tposney commented 1 year ago

In GitLab by @kaelad on Nov 9, 2022, 19:09

I'm not that worried about something else setting the Midi flags directly on the actor data. I know it's technically possible to have a macro or something do that, but I'm not too worried about that. However, I did see that you've enhanced the AE processing so those flags are no longer just 1/0 for on/off. Now you can have an expression do a test so the flag can be conditional. That makes things more complicated. Are those expressions evaluated when the active effect is applied to the actor or do you save the expressions to the actor as-is and evaluate them each time separately?

tposney commented 1 year ago

The expression is kept in the active effect, but the target of the expression (i.e. the flag that is being set is always evaluated to true/false/0/1) and that is done each time prepareData is called, so might not be a problem after all if you look to the target flag to see what the value is.