tposney / midi-qol

Other
5 stars 0 forks source link

[10.0.35] - triggerTargetMacros - wasHit maybe not correct checks #1219

Closed tposney closed 1 year ago

tposney commented 1 year ago

In GitLab by @thatlonelybugbear on Apr 6, 2023, 12:30

This is how it's implemented now in async triggerTargetMacros(triggerList: string[], targets = this.targets) of workflow.ts

const wasHit = (this.item ? wasAttacked : true) && (this.hitTargets?.has(target) || this.hitTargetsEC?.has(target));

I wonder if this meant to be like this

const wasHit = (this.item?.hasAttack ? wasAttacked : true) && (this.hitTargets?.has(target) || this.hitTargetsEC?.has(target));

cause,as it now is, it will not pick up Items that don't do an actual Attack Roll, like save spells.

tposney commented 1 year ago

I think that for consistency wasHit should only fire if the item actually has an attack roll. Spells that don't roll an attack can be picked up via isDamaged/isSave etc.

tposney commented 1 year ago

In GitLab by @thatlonelybugbear on Apr 6, 2023, 23:16

I don't disagree, but some spells will not be picked up by isDamaged, cause this needs wasDamaged

if (wasDamaged && triggerList.includes("isDamaged"))

to qualify and wasDamaged needs wasHit, which needs wasAttacked :sweat_smile: So it effectively is cancelled.

And isSave will not always work as there are spells without a save like Magic Missile, which I was trying to automate when it all started (actually I was trying to automate the Brooch of Shielding that cancels damage of Magic Missiles).

I ended up creating a isTargeted trigger locally to circumvent that and keep the rest as is

tposney commented 1 year ago

I removed the wasHit requirement from wasDamaged so should be ok