New "Boss Mod" triggers working with both DBM & BigWigs #4537

mrbuds · 2023-07-18T11:42:00Z

Description

This PR adds new triggers working with both DBM & BigWigs, which fixes #4502

Boss Mod Announce
Boss Mod Stage
Boss Mod Timer

All code for DBM & BW triggers were moved from Prototypes.lua & GenericTrigger.lua to BossMods.lua

New public function WeakAuras.GetBossStage

It has a few deprecated functions kept for compatibility with custom code

WeakAuras.GetAllDBMTimers
WeakAuras.GetDBMTimerById
WeakAuras.GetDBMTimer
WeakAuras.GetBigWigsTimerById
WeakAuras.GetAllBigWigsTimers
WeakAuras.GetBigWigsStage
WeakAuras.RegisterBigWigsTimer
WeakAuras.RegisterDBMCallback

Boss Mod Timer doesn't carry a "type" option, as both bossmod don't have common type options, and will always ignore matches for a cast bar.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested

These triggers will use BigWigs if installed If not installed they will use DBM BossMod Timer will always ignore "cast" timers for bigwigs also.. renamed all "id" fields into "timerId" to avoid confusion re-inverted timerId & triggerId for DBM TimerMatches

…according to Mitalie WeakAuras#4502 (comment)

QartemisT

Looks good from a DBM compatibility point of view. Tagging @MysticalOS for cross-review.

MysticalOS · 2023-07-18T17:10:54Z

don't existing bw triggers have type. or at least a checkbox for cast/not cast? i feel we still have to match this or auraa that rely on it won't move over to unified. i know bw doesn't support advanced typing so it can't really go beyond cd and cast but that's probably fine for now. if it helps i can also just add a simp type arg that just returns true/false in our handler for caatbar or not but looking at this code you basically have that anyways by string finding case "cast" already.

if i'm mistaken this on this i'll correct post when i get home. family keeps forgetting my "stop scheduling shit on tuesday's" rule :/

mrbuds · 2023-07-19T16:25:00Z

i feel we still have to match this or auraa that rely on it won't move over to unified

Plan changed from

migrate to new trigger
kill old triggers

to

no migration planned
keep old triggers
new bossmod triggers won't have any type option, casts are ignored

MysticalOS · 2023-07-19T18:22:15Z

I feel that if there isn't parity with feature of old trigger, no one will use new one at all. The type option is used extensively in many of weak aura examples i gave that targeted BW triggers, cause they need to know whether it's a CD or cast bar and they do trigger off cast bars sometimes.

I can understand no migration since that's extra work, but to ensure people even use the new trigger in future tiers, it has to do everything old one does, or they'd just always use old one. Parity to old BW trigger should be at base a minimum. I can understand why DBMs more thorough advanced typing isn't supported (cause BW obviously doesn't support it). but outs does allow very easily mapping to existing BW cast/nocast trigger mapping.

mrbuds · 2023-07-19T18:28:25Z

It's not a problem of "extra work", we won't do anything that break previously made auras in the middle of an expansion.

MysticalOS · 2023-07-19T18:29:37Z

no i get that, that's why no migration. existing WAs shouldn't be changed now I agree. But i'm saying if 10.2 hits ptr, and someone is creating NEW auras, they're not gonna use the new trigger if it lacks features old one has.

mrbuds · 2023-07-19T18:35:33Z

no i get that, that's why no migration. existing WAs shouldn't be changed now I agree. But i'm saying if 10.2 hits ptr, and someone is creating NEW auras, they're not gonna use the new trigger if it lacks features old one has.

We can ask Ora to check on wago, but i think 99.99% of auras using these triggers doesn't use type options

MysticalOS · 2023-07-19T19:06:08Z

if that's case, we can table the "type" stuff for now until it's further simplified and streamlined.

I just pushed DeadlyBossMods/DBM-Unified@491d8a5 to address the over complexity of DBMs type system. Whether or not BW does parity to it, at very least, it still makes it easier to parse ours.

This summerizes down to simply

"cd" = any timer that's a cooldown. straight forward
"stage" = any timer that's for a stage such as stage ending, next stage, Rp timers, or combat begins/warmup timers.
"target" = any timer that is for an event or debuff affecting a specific target such as "debuff on playername" timer or "debuff fades" timer which is basically same thing but for self.
"cast" = any timer that's for either an incoming cast, or an active cast channel such as a cast bar for incoming frostbolt or a cast bar for a channeled whirlwind
"pull", "break", and "berserk" have own classifications now and is pretty straight forward.

WeakAuras/BossMods.lua

Mitalie · 2023-07-20T05:07:50Z

It doesn't necessarily have to be in this PR, especially if announce triggers are rarely used, but DBM Announce and Boss Mod Announce triggers could be improved by adding support for spellID / key and count, reducing or removing the need to use locale-sensitive messages as trigger options. DBM_Announce has spellID as 4th and count as 7th arg, BigWigs_Message has key as 2nd arg and count would need to be parsed from message similarly to BigWigs_StartBar.

Also, not a new issue but BigWigs Message trigger has BigWigs Addon option to test against the first arg of BigWigs_Message, however the first arg is a table value (https://github.com/search?q=repo%3ABigWigsMods%2FBigWigs%20BigWigs_Message&type=code) which will never match the string-valued option. It should probably be removed, or test against <table>.moduleName instead.

MysticalOS · 2023-07-20T05:19:28Z

announce triggers are heavily used on echo of neltharian for bomb assignments, specifically for hearts/bombs. But the interesting note of them is they used string matching even when spellid and count was available in the BW announce object. I intentionally changed DBMs strings to match perfectly so they'd work in a unified. but I also didn't realize that weak auras never actually added support for spellid or count in the dbm announce object (and thus not in the boss mod one either). I'd argue it's definitely used enough to warrant addition before merge, or at least a new PR put in immediately after (we can do that if it helps).

Mitalie · 2023-07-22T06:05:46Z

Boss Mod Timer trigger passes extraArg1/"noCastBar" to Generic:TimerMatches as the 6th argument, but DBM:TimerMatches expects it in the 7th position. The unexpected 6th arg causes TimerMatches to always return false.

The above bug would cause Boss Mod Timer to never activate when backed by DBM, if not for the call to Generic:GetTimer omitting extraArg1. While this allows the trigger to function at least partially, the discrepancy doesn't seem intentional.

mrbuds · 2023-07-29T14:16:06Z

@Mitalie thanks for your review! I pushed a commit that should fix the issues you found

Mitalie

Found a few more things.

You can go ahead and mark my first review's comments as resolved - apparently I can't do that myself, it's restricted to the PR submitter and people with write access to the repository.

WeakAuras/BossMods.lua

MysticalOS · 2023-08-07T23:52:05Z

local barOptions = DBT.Options or DBM.Bars.options
Pretty sure we eliminated DBM.Bars last expansion. No one should be running that, the force disable in DBM would make sure DBM for 9.x versions wasn't loading

Anytime I see DBM.Bars come up in a lua error, it's cause they have a weak aura from like 2018 they haven't updated in years or something, usually the old timeline lol.

Shouldn't exist in any sane setup

MysticalOS · 2023-08-08T00:01:54Z

I'd also still love to see Count arg added to "BossMod_Announce" object since it's a single line of parsing it from text (similar to what you already do for BW object) and using the count arg DBM provides (no need to even parse it) Then it's a knock out of the park in full usability next tier

mrbuds · 2023-08-08T01:06:52Z

I'd also still love to see Count arg added to "BossMod_Announce" object since it's a single line of parsing it from text (similar to what you already do for BW object) and using the count arg DBM provides (no need to even parse it) Then it's a knock out of the park in full usability next tier

Afaik BigWigs_Message event doesn't have a count arg, neither of the 3 message/announce triggers have a count option

MysticalOS · 2023-08-08T01:08:52Z

right, that's why all the weak aura packs in aberus were using string scanning on echo of nelth.

if you look at any of popular packs they were scanning for msg match = "bomb (1)" "bomb (2)" etc. since there was no means to go "if spellid = 12345 and count = 4" on announce.

but yeah if original objects don't even have it then maybe worth another separate PR/issue then and not this one. :), to add feature to both legacy AND this object.

mrbuds · 2023-08-08T01:10:39Z

i have not seen a single aura for neltharion using announce/message triggers, only bigwigs/dbm timers, but i didn't look at all of them

MysticalOS · 2023-08-08T01:21:59Z

https://wago.io/Y2peTP20U
This one

MysticalOS · 2023-08-08T01:25:55Z

Fortunately it had very few installs. So I'd call it a niche case, but it still exposes the design flaw of that currently this actually is the ONLY way to cherry pick specific counts (in announce object at least, timers support count).

InfusOnWoW · 2023-08-08T09:09:15Z

We can certainly add that, though it doesn't need to be part of this PR. So let's merge this first.

mrbuds added 14 commits July 18, 2023 12:33

Move and Rename BW & DBM functions to Private.ExecEnv.BossMods

0785fb5

Move BossMods trigger system to BossMods.lua

017ee58

remove unused variables

cbd862b

refactor to make both bossmod more symetrical

72f2e9b

move arguments specific to an addon at the end

a9b4669

move bossmods's event_protypes to BossMods.lua

21c142e

Rename new triggers, and add information message

8c151dc

add WeakAuras.GetBossStage

782d0a3

fix BigWigs stage reset event

636883a

add deprecated functions

e8f42c2

Improve compatibility with jods aura

4e365b1

Remove "kill" and "wipe" callback for DBM as they are gone for years …

fc34434

…according to Mitalie WeakAuras#4502 (comment)

BossMod Timer trigger: don't match DBM cast bars

2e8ed2e

QartemisT reviewed Jul 18, 2023

View reviewed changes

Mitalie reviewed Jul 20, 2023

View reviewed changes

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

WeakAuras/BossMods.lua Show resolved Hide resolved

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

fixes issues reported by Mitalie

b53d1f2

Mitalie reviewed Aug 3, 2023

View reviewed changes

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

fixes more of Mitalie review

27613b5

mrbuds added 2 commits August 3, 2023 15:26

Timer triggers: do not update state for no change made

23e48e8

add a "Boss Mod Stage (Event)" trigger

26f66f3

InfusOnWoW reviewed Aug 6, 2023

View reviewed changes

InfusOnWoW reviewed Aug 7, 2023

View reviewed changes

WeakAuras/BossMods.lua Outdated Show resolved Hide resolved

Fix a few bugs, polish a few things

5e0206f

Remove DBM.Bars compability

c681510

Shouldn't exist in any sane setup

InfusOnWoW approved these changes Aug 8, 2023

View reviewed changes

InfusOnWoW merged commit 1611294 into WeakAuras:main Aug 8, 2023
1 check passed

MysticalOS mentioned this pull request Aug 8, 2023

[Feature] Improve Announce object to include Count #4562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New "Boss Mod" triggers working with both DBM & BigWigs #4537

New "Boss Mod" triggers working with both DBM & BigWigs #4537

mrbuds commented Jul 18, 2023 •

edited

Loading

QartemisT left a comment

MysticalOS commented Jul 18, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023 •

edited

Loading

Mitalie commented Jul 20, 2023

MysticalOS commented Jul 20, 2023

Mitalie commented Jul 22, 2023

mrbuds commented Jul 29, 2023

Mitalie left a comment

MysticalOS commented Aug 7, 2023 •

edited

Loading

MysticalOS commented Aug 8, 2023

mrbuds commented Aug 8, 2023

MysticalOS commented Aug 8, 2023 •

edited

Loading

mrbuds commented Aug 8, 2023

MysticalOS commented Aug 8, 2023

MysticalOS commented Aug 8, 2023

InfusOnWoW commented Aug 8, 2023

New "Boss Mod" triggers working with both DBM & BigWigs #4537

New "Boss Mod" triggers working with both DBM & BigWigs #4537

Conversation

mrbuds commented Jul 18, 2023 • edited Loading

Description

Type of change

How Has This Been Tested

QartemisT left a comment

Choose a reason for hiding this comment

MysticalOS commented Jul 18, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023

mrbuds commented Jul 19, 2023

MysticalOS commented Jul 19, 2023 • edited Loading

Mitalie commented Jul 20, 2023

MysticalOS commented Jul 20, 2023

Mitalie commented Jul 22, 2023

mrbuds commented Jul 29, 2023

Mitalie left a comment

Choose a reason for hiding this comment

MysticalOS commented Aug 7, 2023 • edited Loading

MysticalOS commented Aug 8, 2023

mrbuds commented Aug 8, 2023

MysticalOS commented Aug 8, 2023 • edited Loading

mrbuds commented Aug 8, 2023

MysticalOS commented Aug 8, 2023

MysticalOS commented Aug 8, 2023

InfusOnWoW commented Aug 8, 2023

mrbuds commented Jul 18, 2023 •

edited

Loading

MysticalOS commented Jul 19, 2023 •

edited

Loading

MysticalOS commented Aug 7, 2023 •

edited

Loading

MysticalOS commented Aug 8, 2023 •

edited

Loading