OverlayPlugin / cactbot

FFXIV TypeScript Raiding Overlay
Apache License 2.0
97 stars 34 forks source link

raidboss: add sidewise spark configuration option #354

Closed jacob-keller closed 2 weeks ago

jacob-keller commented 2 months ago

This PR is an attempt to help improve the m4n sidewise spark callouts. In particular, this PR does 2 major things:

  1. Introduce a config for m4n
  2. collapse sequence of cardinals directions

The configuration option adds a way to select how sidewise spark should be called. It includes the 'collected' mode which is the current default, and the 'sequence' mode which is the new behavior.

The 'collected' mode makes one initial callout after 2 cleaves are detected, and then a single callout for all the cleave safe spots as the boss starts casting the final cleave.

The 'sequence' mode makes an initial callout after 2 cleaves as before. But instead of a single cleave with all the options, it makes one callout with the remaining safe spots after every cleave finishes.

In addition to the new configuration option, I added logic to collapse a sequence of safe spots into a single intercardinal. This works by checking if all the spots point to the same intercard.

This greatly simplifies the callouts in many cases, such as when most of the cleaves are in the same directions.

There is one nit, which is that the sequence mode may not stop calling early in certain cases, and may make a callout sequence like:

NE NW SW SW W

This is because I worried about clearing the sidewise spark cleaves array in the hit trigger, due to the potential to accidentally clear the array when the final hit has not yet been recorded.

Finally, this change has an added advantage that a sidewise spark from the boss later in the fight does not get ignored anymore. That occured because the callout for the boss sidewise spark was checking if the sidewiseSparkCounter was 0. It assumed the fight only has one sidewise spark in this manner, but it can infact happen again later in the fight.

jacob-keller commented 2 months ago

This worked ok in my practice, but I'm not entirely sure about it. I really struggled with seeing a callout that was 5 directions, since it is extremely easy to lose track of where to go. This worked ok for me, but I wasn't sure what others would think, since the actual callouts for when to go are relatively delayed, because you don't get told to go to the next spot until the moment the current clone expires. This could lead to being to slow to react and move to a good spot.

eathdemon commented 2 months ago

This worked ok in my practice, but I'm not entirely sure about it. I really struggled with seeing a callout that was 5 directions, since it is extremely easy to lose track of where to go. This worked ok for me, but I wasn't sure what others would think, since the actual callouts for when to go are relatively delayed, because you don't get told to go to the next spot until the moment the current clone expires. This could lead to being to slow to react and move to a good spot.

as a option in the settings, would it in thoary be possable to let people select group 1 or group 2 for the callouts?

jacob-keller commented 2 months ago

It could probably be an option. I also considered making it call out all remaining directions so it would sort of count down the callout.

I'm also perfectly fine carrying this on my own fork if no one else likes it.

wexxlee commented 2 months ago

as a option in the settings, would it in thoary be possable to let people select group 1 or group 2 for the callouts?

I might be completely forgetting, so apologies in advance - but is there a G1/G2 component to R/M4 normal? IIRC, Sidewise Spark is just 5 directional cleaves, so I'm not immediately recalling what the light party/group aspect here would be.

More generally about the PR: Full disclosure that I haven't tested this in game or looked at the timing, so this is based on recollections from stale memory. I do think the combined 5-direction infoText call out has some utility (perhaps not for everyone, but at least some), since it both gives a continual on-screen reminder of the movement and allows potential intercard optimization and reduced movement -- for example, if I see "E -> N -> E -> N -> something else", I personally will be going NE to post up and cast for a while. So while I can definitely see value in something additive here, I don't necessarily think it makes sense to remove what's already there.

As far as what could be additive: In a lot of older content, cactbot sort of follows the unwritten (maybe written? I should re-read the docs) rule of an infoText alert that displays a full mechanic chain, with periodic alertText warnings for each step of the mechanic. (SoSEx Quintuple Cast, and the left/right cleaves in P12Sp1 immediately come to mind). I think that's the approach you were going for here (assuming you make this a separate trigger, rather than replacing the existing one). The downside for this fight, though, is that the cleaves happen so quickly in sequence that alertTexts have to have short durations (I think you've got it at 2s right now) to avoid stacking or displaying stale info, and that doesn't give a lot of reaction time or ability to process the info.

TLDR: I think there's good initial direction here. I'd suggest separating this into a new trigger as opposed to replacing the existing one, and if it would be useful, I'm happy to give this a run in-game and see how it feels or if I can come up with any other ideas for potentially consolidating the alerts.

JLGarber commented 2 months ago

There is no party component on normal mode, correct. Each player resolves it individually.

I think I agree with "keep the current functionality, add this new trigger as additional functionality and not as replacement". I too prefer to just see the entire sequence, but I do agree that that doesn't work for everyone, so individually timed calls make sense to have alongside. The comparison with Seat of Sacrifice is apt.

eathdemon commented 2 months ago

there seem to be sevral mechanics in m4s where breacking it out by party, atleast as a option, would make the callouts cleaner. chain lighting in p2 is a clear example of that.

wexxlee commented 2 months ago

there seem to be sevral mechanics in m4s where breacking it out by party, atleast as a option, would make the callouts cleaner. chain lighting in p2 is a clear example of that.

This PR pertains to M4 Normal, not M4S.

Regarding Chain Lightning (assuming you are referring to the Raining Swords mechanic), the triggers already determine the side of the arena based on your light party and call the corresponding safe spots in order for just that side. If you think there's a mechanic that could use different callouts for G1/G2, could you open an new feature request here and give a little more detail on what mechanics you believe a G1/G2 option would be useful for, and what you would like the triggers to convey for each group?

jacob-keller commented 2 months ago

I will update this tomorrow. I have some ideas for how to integrate with the existing callout. I agree keeping both may be helpful but I'll try to figure out a good way.

One way I thought of was having the full set of what's left be updated every time one finishes? So rather than only showing you what to do next it goes down one at a time but shows the remaining set nicely.

I also noticed this is currently blocking a callout for a half room before she starts spawning clones, I think. I'll investigate that. I'll also try to refactor my other PRs to use multiple triggers as suggested instead of using all in the same one.

jacob-keller commented 2 months ago

I did some more refactoring, and now the callout for the clones goes something like this as the clones appear or hit:

W W => N W => N => E W => N =? E => S N => E => S E => S => W // another clone or the boss started here S => W W

These callouts are something like 2-3 seconds apart, with the actual hits starting when the list begins reducing.

This seems useful to me, but I haven't tested it live yet. I suspect the durations are wonky and need to be tuned. I also suspect not everyone will like this, so perhaps we can come to some compromise on what makes sense here.

jacob-keller commented 1 month ago

I've been using this for a few weeks, and I like it, but I'm not sure how others would feel about this.

wexxlee commented 1 month ago

I've been using this for a few weeks, and I like it, but I'm not sure how others would feel about this.

Sorry for the delay in reviewing - I had assumed since the PR is marked as draft that you were still working on it. I'll give this a run tomorrow and see how it feels.

wexxlee commented 1 month ago

I've been using this for a few weeks, and I like it, but I'm not sure how others would feel about this.

Sorry it took me so long to get back to this! I've run it a couple times to get a feel for the timing/noise level of these changes. I think there's probably a lot of room here for personal preference, but imho, I found this to run a little on the noisy side. The infoText alerts started stacking up on top of one another fairly quickly, and it took me a bit to figure out which I was supposed to pay attention to. And when running with tts enabled, it was a jumble of overlapping directional calls; without text alerts, I would have had no idea what cactbot was instructing me to do & when.

Compare, e.g.,: image

with:

image

This definitely seems like a prime use case for what valarnin proposed earlier with using a Generator to create and adjust dynamic text, but until we get around to implementing that, I think what we have now (an initial call with the first two, and then a long-duration with the full set) will probably be better for most players.

If you wanted to, I think you could add this as a config option people could enable if they want more sequenced directions -- but in that case, I'd suggest the output each time should be limited to the immediate direction and the one after (e.g. E => N), and it should disable the existing default triggers, so only 4 alerts would be generated (e.g. dir1 => dir2 ... dir4 =>dir5`). (And I'd definitely suggest testing this with TTS to make sure the timing won't overlap on the calls).

jacob-keller commented 1 month ago

I think what I may try to do is have it collapse directions, since the majority of pulls I've seen end up just having a single final safe corner.

I agree this is quite noisy as it is now.

jacob-keller commented 1 month ago

Compare, e.g.,: image

with:

image

My primary motivation for this PR is the fact that in these images, we call "E => S => W => N => W" at a point when east has already completed, and south is almost completed, making us do a callout that implies "you should go east and south" when infact, the directions really mean "you are about to need to go west".

jacob-keller commented 1 month ago

Would a version that only does two callouts, but which removes any that have already completed by the point we call be acceptable?

I'm also considering an option which can tell if the entire sequence can condense, such as if it was

"N => W => N => W => N" which could just be condensed to "NW and stay"

jacob-keller commented 1 month ago

I'm working on implementing this as an option at least to start.

jacob-keller commented 1 month ago

I updated this PR to be a configuration option, and to add collapsing logic. I think this version should be less noisy, though I didn't yet work on de-noising the sequence mode fully.

wexxlee commented 1 month ago

Thanks again for putting this together! One general comment - I think combining two cleaves into an intercard direction is a good simplification, but I wonder whether we should add something like '(x2)' to the end of the callout to make clear that the direction applies for two hits, as opposed to just that intercard being safe for a single hit.

I did notice a couple things while testing too that I wanted to flag for you:

  1. In either configuration option, cactbot is outputting intercardinal directions for the first two clone cleaves (which are solo) -- previously it was just 'N' or 'S'. In either config option, I think we should probably continue to output 'N' or 'S' for these, since there's no east/west component.

  2. In collected mode for the 5-hit cleaves, the trigger is no longer outputting the first two directions as a collected string (e.g. E => S), but instead as a combined intercard (e.g. SE). I think the combined intercard makes sense for sequence mode, but for collected mode, I think we should retain the existing output, as folks have come to expect that -- and having a combined intercard makes it a little more work to parse the follow-up 5-hit sequence, especially as the intercard dir will always have 'N'/'S' as the first letter, but the first cleave is always E/W).

  3. In sequence mode, I think there may be an issue with the trigger firing multiple times or at the wrong times? From one pull, here's the different outputs from both modes:

Collected: image

Sequence: image

  1. In sequence mode when it's a stay pattern, I think there's an extraneous output. From one pull where all cleaves were N & W: image

I haven't looked at it closely, but I'm guessing that this is because the third & fourth cleave are combined into SE, and only when the fifth cleave is processed is the Stay output provided. We could wait for the 5th cleave to see if it's the same before providing an output for 3&4, but if that will cause a problem with reaction time, I think there's an alternate approach:

jacob-keller commented 1 month ago

I'll take a look. Its not supposed to output intercard spot unless the whole sequence is safe in the one intercard.

On Sat, Sep 28, 2024, 4:12 PM Wexx @.***> wrote:

@.**** commented on this pull request.

comments above

— Reply to this email directly, view it on GitHub https://github.com/OverlayPlugin/cactbot/pull/354#pullrequestreview-2335490537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGVRXZO4YGLWFIQJHYDLDTZY4ZWJAVCNFSM6AAAAABMRV3NHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZVGQ4TANJTG4 . You are receiving this because you authored the thread.Message ID: @.***>

jacob-keller commented 1 month ago

Thanks again for putting this together! One general comment - I think combining two cleaves into an intercard direction is a good simplification, but I wonder whether we should add something like '(x2)' to the end of the callout to make clear that the direction applies for two hits, as opposed to just that intercard being safe for a single hit.

It should only be combining when actually printing the whole chain, and only if everything so far is a single intercard. For sequence mode, this doesn't work quite right, as you saw above.

I did notice a couple things while testing too that I wanted to flag for you:

1. In either configuration option, cactbot is outputting intercardinal directions for the first two clone cleaves (which are solo) -- previously it was just 'N' or 'S'.  In either config option, I think we should probably continue to output 'N' or 'S' for these, since there's no east/west component.

2. In `collected` mode for the 5-hit cleaves, the trigger is no longer outputting the first two directions as a collected string (e.g. `E => S`), but instead as a combined intercard (e.g. `SE`).  I think the combined intercard makes sense for `sequence` mode, but for `collected` mode, I think we should retain the existing output, as folks have come to expect that -- and having a combined intercard makes it a little more work to parse the follow-up 5-hit sequence, especially as the intercard dir will always have 'N'/'S' as the first letter, but the first cleave is always E/W).

3. In `sequence` mode, I think there may be an issue with the trigger firing multiple times or at the wrong times?  From one pull, here's the different outputs from both modes:

Collected: image

Sequence: image

4. In `sequence` mode when it's a stay pattern, I think there's an extraneous output.  From one pull where all cleaves were N & W:
   ![image](https://private-user-images.githubusercontent.com/86693821/371811276-f60336f5-bd01-4656-9070-4df68ff7a4b1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjc2NTQ5NDEsIm5iZiI6MTcyNzY1NDY0MSwicGF0aCI6Ii84NjY5MzgyMS8zNzE4MTEyNzYtZjYwMzM2ZjUtYmQwMS00NjU2LTkwNzAtNGRmNjhmZjdhNGIxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTMwVDAwMDQwMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJhNzAyNDUwOTVkMzUzYTE2N2IxNDFhYzQwMTAyOWFiYjc3MWJlZmVmN2Q5NmY5NDEwZWI3ZDQyNTFhNTZkMmUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.KS04pvD9Vkc3EIjFA9y58DwyVEALL8caUfI872EJSbw)

The "extra" input is because of this flow:

It sees S => W => N and logs this

The S cleave hits, so it sees W => N and collapses this to NW

The next collect happens and it sees W => N => W and collapses this to NW and logs it again.

The final hit appears and thats when we do the "NW => STAY" logic.

I haven't looked at it closely, but I'm guessing that this is because the third & fourth cleave are combined into SE, and only when the fifth cleave is processed is the Stay output provided. We could wait for the 5th cleave to see if it's the same before providing an output for 3&4, but if that will cause a problem with reaction time, I think there's an alternate approach:

* If the safe spot for 5 **is not** the same as the safe spot for 3 & 4, do an `alertText` with just the safe cardinal direction (no need for an intercard component for this one).

* If the safe spot for 5 **is** the same as the safe spot for 3 & 4, do an `infoText` that just says 'stay'.  I think that would accomplish the same thing but help avoid some of the directional clutter.  Thoughts?

I think something like this works, but I'll have to refactor how it actually decides this. I'll look into it.

I'm thinking something along the lines of checking at each hit, and if the safe spot changes, do an alert, and if it stays the same do an info with stay.

We can keep the collapsing in collect mode only for the final hit, and only collapse if the whole chain does.

jacob-keller commented 1 month ago

image

Ok this version seems to be ok with my simulated tests. For the pattern:

E => N => W => N => W, we first see the E =>N and collaps it to a "NE" callout. Then at the first hit we see N => W => N, collapse it to NW, and see that its different from the previous safe spot, so we call NW x3.

Then, when the final W hit from the boss shows up, we notice that its the same as the last NW hit, so we just avoid any more callouts.

I think this does most of what you want.

I opted to not call numbers for the first intercard collection, because this was more confusing than not. This would have shown you:

NE x2 NW x3

which is confusing because it might imply the wrong number of safe hits for the NW spot, as you might think "its going to hit NE twice, then NW 3x" but in reality, the north hit from the NE was also part of the NW hits from the NW x3.

wexxlee commented 4 weeks ago

I opted to not call numbers for the first intercard collection, because this was more confusing than not. This would have shown you:

NE x2 NW x3

which is confusing because it might imply the wrong number of safe hits for the NW spot, as you might think "its going to hit NE twice, then NW 3x" but in reality, the north hit from the NE was also part of the NW hits from the NW x3.

Sorry, I'm still a little confused. In that pattern, wouldn't E => N collapse to "NE x2" and then W => N => W collapse to "NW x3" Not sure I'm groking why the 2nd call (N) would be part of "NW x3". Maybe to help me understand, could you give a few examples of sequences and how the trigger will call them?

jacob-keller commented 4 weeks ago

I opted to not call numbers for the first intercard collection, because this was more confusing than not. This would have shown you:

NE x2 NW x3

which is confusing because it might imply the wrong number of safe hits for the NW spot, as you might think "its going to hit NE twice, then NW 3x" but in reality, the north hit from the NE was also part of the NW hits from the NW x3.

Sorry, I'm still a little confused. In that pattern, wouldn't E => N collapse to "NE x2" and then W => N => W collapse to "NW x3" Not sure I'm groking why the 2nd call (N) would be part of "NW x3". Maybe to help me understand, could you give a few examples of sequences and how the trigger will call them?

We save the full set of calls. We collapse only when reporting.

It first collects E > W, and logs this as NE x2

Then it collects a few more and we have E > N > w > N.

Then the first cleave hits and removes east so we have N > W > N which logs as NW x3, because we log after every hit in sequence mode.

After this log we get one final collect of the last hit, which is W, so we have N > W > N > W, but the new logic decects this is the same same spot so output is elided.

wexxlee commented 3 weeks ago

We save the full set of calls. We collapse only when reporting.

It first collects E > W, and logs this as NE x2

Then it collects a few more and we have E > N > w > N.

Then the first cleave hits and removes east so we have N > W > N which logs as NW x3, because we log after every hit in sequence mode.

After this log we get one final collect of the last hit, which is W, so we have N > W > N > W, but the new logic decects this is the same same spot so output is elided.

I think I understand, but the issue I'm seeing is that we're calling 'NE x2' (which implies that NE should be safe for two cleaves), but then consolidating the second safe spot (N in this example) into the 'NW x3' follow-up call -- which isn't correct, since the 'N' safe spot was already used as the 2nd part of the 'NE x2'. In other words, if we're doing a first 'x2' call, it should be the safe spot for the first two cleaves, and the followup 'x3' call (whether Stay or otherwise) should cover the final three cleaves. Does that make sense?

jacob-keller commented 3 weeks ago

We save the full set of calls. We collapse only when reporting.

It first collects E > W, and logs this as NE x2

Then it collects a few more and we have E > N > w > N.

Then the first cleave hits and removes east so we have N > W > N which logs as NW x3, because we log after every hit in sequence mode.

After this log we get one final collect of the last hit, which is W, so we have N > W > N > W, but the new logic decects this is the same same spot so output is elided.

I think I understand, but the issue I'm seeing is that we're calling 'NE x2' (which implies that NE should be safe for two cleaves), but then consolidating the second safe spot (N in this example) into the 'NW x3' follow-up call -- which isn't correct, since the 'N' safe spot was already used as the 2nd part of the 'NE x2'. In other words, if we're doing a first 'x2' call, it should be the safe spot for the first two cleaves, and the followup 'x3' call (whether Stay or otherwise) should cover the final three cleaves. Does that make sense?

I opted to just not collapse the first calls and only collapse when we are calling out the final one. Coding it to manage when it should or shouldn't collapse is a bit too difficult to figure out

xiashtra commented 3 weeks ago

I've been poking at this a bit in the emulator, sometimes it does a nice job of condensing the calls, and sometimes it just blows up into a confusing mess. For example, this set of cleaves image turns into image which is just an overload of information when they are coming every 2s.

jacob-keller commented 3 weeks ago

I've been poking at this a bit in the emulator, sometimes it does a nice job of condensing the calls, and sometimes it just blows up into a confusing mess. For example, this set of cleaves image turns into image which is just an overload of information when they are coming every 2s.

One thing I could do is just keep the default behavior of collecting but remove any already hit calls when making the final call.

I think that's the biggest thing that confused me with the combined callout: remembering where you are in the callout order.

wexxlee commented 3 weeks ago

One thing I could do is just keep the default behavior of collecting but remove any already hit calls when making the final call.

I think that's the biggest thing that confused me with the combined callout: remembering where you are in the callout order.

Do you need to remove at all? We know it's a fixed number of cleaves (5), and they get pushed into the array in order; if they aren't removed, you can access each cleave directly, combine certain ones, etc. I think then it would only becomes a matter of validating array length (and returning a call at the correct length), and using a combination of delays/durations to make the timing feel good in-game...

jacob-keller commented 3 weeks ago

One thing I could do is just keep the default behavior of collecting but remove any already hit calls when making the final call. I think that's the biggest thing that confused me with the combined callout: remembering where you are in the callout order.

Do you need to remove at all? We know it's a fixed number of cleaves (5), and they get pushed into the array in order; if they aren't removed, you can access each cleave directly, combine certain ones, etc. I think then it would only becomes a matter of validating array length (and returning a call at the correct length), and using a combination of delays/durations to make the timing feel good in-game...

My main point is that the order goes something like this:

  1. collect first clone hit
  2. collect second clone hit AND call the first two safe spots
  3. collect third clone hit
  4. collect 4th clone hit
  5. first clone hit is resolved
  6. collect boss hit AND call all 5 spots.

But at step 6, we are calling all 5 even though the first one has already hit.

My issue with this is that the final set of callouts may say something like "E => N => W => S => W"

but the east safe spot is already resolved, and in fact we need to be west soon. As far as I know they always alternate either N/S or E/W at each call. (it's never going to be N => S, for example, at least not in my experience)

At least for me, I then have to grasp and remember that something already hit, so calling out all 5 led to confusing issues where I'd end up on a bad spot thinking I still had time.

There's only something like a 1 second between the first hit and the boss collecting, so using the "sequence" mode is bad because it leads to the large number of calls.

Personally, I like the sound effect of when things hit, so that I know "ok I need to move to the next spot", but it ends up leaving a lot of clutter on the display and does not work well with TTS.

I don't think we should be making a callout for data that is already irrelevant. We could delay making the final 3 callouts until the first 2 have hit? The problem then is that you don't get a callout for the next (3rd) safe spot until its almost ready to be used.. But otherwise we are ultimately always going to repeat some information. Its only a delay of about 1 second... but you already only have ~2 seconds between each hit anyways.

jacob-keller commented 3 weeks ago

I'm leaning towards dropping the sequence strategy, and just keeping the collapsing bits, and making sure the final callout does not include any hits that already finished.

How does that sound?

jacob-keller commented 2 weeks ago

I attempted to extract just the collapsing and simplification into https://github.com/OverlayPlugin/cactbot/pull/471

jacob-keller commented 2 weeks ago

I think the version I posted without a sequence strategy is preferable to this.