-
Notifications
You must be signed in to change notification settings - Fork 75
raidboss: Additional M/R2S triggers #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I will not have time to fully review this tonight, but on a quick read through it all looks fine. Nothing jumped out at me as being obviously incorrect or poorly-handled. I'll need to have a think about this vs 349's handling on the Defamation sequence. |
valarnin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have time tonight to do a deep dive on this either, but a quick skim through showed nothing glaring.
Adds callouts for "curtain call" (aka Beeloved Venom). On this, I added a 'Marge w/ X` alert for the active player, as well as info alerts for other merges that should happen not involving the active player. On the latter, I could see some disagreement about whether that info is useful or not, so happy to adjust/remove that if folks don't like.
I think that the "other" triggers are useful both as a healer and as the person doing callouts for my group. Making the "self" trigger alertText and the "other" trigger infoText should be sufficient enough to distinguish them and players always have the option of disabling the trigger in raidboss config if it's too noisy for them personally?
|
(Rebased after the other r2s PRs were merged, and added a suppress to the beat2 spread/towers trigger that was missing from #343.) This should be ready for merge. I've tested in game a couple times, and everything looks good. On the timing of the curtain call stuff, I opted for 10s as suggested. 10s definitely felt comfy for the first merge -- for the 2nd-4th merge, a 10s call comes out right as the prior raidwide damage is being applied, so a player with itchy trigger fingers could see that and merge before being healed up. But delaying it by 1 second doesn't really add much healing margin, and any later than that feels rushed (plus using different delays based on merge order feels like overkill), so I think a flat 10s, plus using "Merge Soon" instead of just "Merge", is the best of the options. |
JLGarber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay, Life In General has been a thing this week. This all looks good. I'll let you push the button on the merge in case any last-minute changes jump out at you.
|
I'm going to pull this and manually test it in my raid tonight! |
|
I have a couple of thoughts:
I think this is overall positive, but I do think I would like to keep the info venom callouts. |
I don't think we usually do "stored" or "later"-type calls when the mechanic happens immediately after, but if you think it's helpful to have these for the first two, I don't have a major issue with keeping the info callouts there.
Not seeing that in the handful of pulls from this week or in prior logs running through RE. Could you provide a split/anonymized log with the duplicated callout? |
I would like to keep the info callouts. Maybe without saying "stored"? It helps me because I like to call out "it will be partners/spreads" and its easier to do that with an info callout. Otherwise you eaither delay saying its partners or spread until after the sides or donut appear. Its not a huge deal and I would just port it back in on my instance.
I think this is a merge conflict with some other changes I had actually. You can ignore that. |
Finally getting around to PRing some additional triggers: * Revises the 'Stored: ' partner/spread call to an `infoText` and suppresses it for the first two occurrences (since Tempting Twist/Honey Beeline happen immediately after). * Adds an `infoText` with the subsequent positioning + partner/spread at the beginning of Tempting Twist and Honey Beeline, followed by an explicit `alertText` with that info when the first part of the mechanic resolves. * Lists some possible TODOs for beat1 & beat2. * Adds defamation/in/towers calls during beat3. (NB: I had this mostly worked up before #349, but if people prefer that approach + some added handling for defamations, I can remove it from this PR before merging.) * Adds callouts for "curtain call" (aka Beeloved Venom). On this, I added a 'Marge w/ X` alert for the active player, as well as info alerts for other merges that should happen not involving the active player. On the latter, I could see some disagreement about whether that info is useful or not, so happy to adjust/remove that if folks don't like. (NB2: This PR doesn't address/replace anything in #339 or #343 -- I had in my notes to do things similarly to those, but since there are already PRs, I think it makes sense to work with those.) This looks fine in vod/RE, but probably could use a proofread and some in-game testing. I may be able to test a little later tonight, but would be great if others can poke at it too. 2262a36
Finally getting around to PRing some additional triggers: * Revises the 'Stored: ' partner/spread call to an `infoText` and suppresses it for the first two occurrences (since Tempting Twist/Honey Beeline happen immediately after). * Adds an `infoText` with the subsequent positioning + partner/spread at the beginning of Tempting Twist and Honey Beeline, followed by an explicit `alertText` with that info when the first part of the mechanic resolves. * Lists some possible TODOs for beat1 & beat2. * Adds defamation/in/towers calls during beat3. (NB: I had this mostly worked up before #349, but if people prefer that approach + some added handling for defamations, I can remove it from this PR before merging.) * Adds callouts for "curtain call" (aka Beeloved Venom). On this, I added a 'Marge w/ X` alert for the active player, as well as info alerts for other merges that should happen not involving the active player. On the latter, I could see some disagreement about whether that info is useful or not, so happy to adjust/remove that if folks don't like. (NB2: This PR doesn't address/replace anything in #339 or #343 -- I had in my notes to do things similarly to those, but since there are already PRs, I think it makes sense to work with those.) This looks fine in vod/RE, but probably could use a proofread and some in-game testing. I may be able to test a little later tonight, but would be great if others can poke at it too. 2262a36
Finally getting around to PRing some additional triggers:
infoTextand suppresses it for the first two occurrences (since Tempting Twist/Honey Beeline happen immediately after).infoTextwith the subsequent positioning + partner/spread at the beginning of Tempting Twist and Honey Beeline, followed by an explicitalertTextwith that info when the first part of the mechanic resolves.(NB2: This PR doesn't address/replace anything in #339 or #343 -- I had in my notes to do things similarly to those, but since there are already PRs, I think it makes sense to work with those.)
This looks fine in vod/RE, but probably could use a proofread and some in-game testing. I may be able to test a little later tonight, but would be great if others can poke at it too.