Skip to content
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

raidboss: tweak sidewise spark callouts #471

Merged

Conversation

jacob-keller
Copy link

This is another attempt at improving the sidewise spark callouts that
hopefully is acceptable to everyone.

I dropped the sequence strategy, and just focused on the collapsing of
intercardinals during the two callouts, and on avoiding repeat callout of
already called or hit spots.

  • raidboss: introduce helper function for stored cleaves
  • raidboss: pass final sidewise spark hit through getCleaveDirs
  • raidboss: avoid re-calling previous clone cleave safe spots
  • raidboss: collapse multiple sidewise spark hits to intercards

@jacob-keller
Copy link
Author

This is a rework of #354

I made it a separate PR because while some of the code is re-used, this is a different solution, and I think it is too dissimilar to the old approach.

@jacob-keller jacob-keller force-pushed the jk-r4n-clone-callout-improvement-v2 branch 2 times, most recently from b1c19ec to 059e293 Compare October 11, 2024 23:28
@jacob-keller
Copy link
Author

This changes the callout from something like:

image

to the following:

image

@jacob-keller
Copy link
Author

I'm also open to making this a strategy option if you don't want to risk changing the existing callouts at all.

@wexxlee
Copy link
Collaborator

wexxlee commented Oct 12, 2024

Thanks for the re-work on this -- on a quick look, this seems like a clean approach and output. Will give this a run tonight/tomorrow!

Copy link
Collaborator

@wexxlee wexxlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a chance to do some quick testing, and looks pretty good. I think this is close enough to what already exists that it makes sense to implement this as a change rather than a strat option, so your approach seems good to me.

If I'm understanding the implementation correctly, the example you provided above is what will appear if the final 3 cleaves can be collapsed to a single intercard. If the final 3 cleaves cannot be collapsed to a single intercard, R4N Sidewise Spark will just return the full sequence of the three remaining cleaves like so:
image

My comments below are based on that assumed behavior, so if I've misunderstood or if it's not working as intended, disregard!

Comment on lines 253 to 255
3: output.num3!(),
4: output.num4!(),
5: output.num5!(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the condition block only returns true if it's a single-cleave scenario or if data.storedCleaves.length === 2. So in the 5-cleave mechanic, this trigger will only fire after the second cleave has been collected, and will not fire again. (Which I think is what you intended, since R4N Sidewise Spark is handling the final 3 cleaves).

If that's the case, do we need outputs for 3, 4, and 5, since those will never be used? Wondering if we can simplify this by removing the lookup thru cleaveNums entirely, including just Output.num2 in the outputstrings, and then calling it explicity rather than doing a number-to-string lookup?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea. I started by copying the previous implementation, so I think we can simplify this a lot more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This makes the code a lot simpler too.


return output.combo!({ dirs: dirs.join(output.separator!()) });
const cleaves: number = data.storedCleaves.length;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const cleaves: number = data.storedCleaves.length;
const cleaves = data.storedCleaves.length;

Typescript should infer the number type automatically from .length.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At one point I had issues with that, but I think that was a different version of the code. Will check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was simplified so we don't even need to save the value.

@github-actions github-actions bot removed the needs-review Awaiting review label Oct 12, 2024
@jacob-keller
Copy link
Author

Had a chance to do some quick testing, and looks pretty good. I think this is close enough to what already exists that it makes sense to implement this as a change rather than a strat option, so your approach seems good to me.

If I'm understanding the implementation correctly, the example you provided above is what will appear if the final 3 cleaves can be collapsed to a single intercard. If the final 3 cleaves cannot be collapsed to a single intercard, R4N Sidewise Spark will just return the full sequence of the three remaining cleaves like so:
image

My comments below are based on that assumed behavior, so if I've misunderstood or if it's not working as intended, disregard!

Ya, right now it only collapses if all 3 can go the same spot. I think that's better than trying to refactor to do NW => E or what not.

@wexxlee
Copy link
Collaborator

wexxlee commented Oct 12, 2024

Ya, right now it only collapses if all 3 can go the same spot. I think that's better than trying to refactor to do NW => E or what not.

Yeah, I think that's a totally reasonable approach.

In m4n, we need to take a list of stored cleave directions and convert
them into cardinal safe spots. Similar code is used in two places. A
future change is going to refactor this logic.

Make StoredCleave an explicit type, and introduce a getCleaveDirs helper
function which takes the actors and stored cleave data, and returns an
array of DirectionOutputCardinal names.

This avoids duplicating the somewhat complex logic of looking up actor
data and finding the facing. It also makes it easier to extend this
logic in the near future.

Signed-off-by: Jacob Keller <[email protected]>
In the trigger for the final hit of sidewise spark from Wicked Thunder,
the cleave directions from stored cleaves are obtained, and then the
final hit is pushed onto the array.

This works for now, but will break when extending the logic of
getCleaveDirs to support collapsing the callout for intercardinal safe
spots.

Instead, push the boss's hit onto the stored cleave array first before
calling getCleaveDirs.

Signed-off-by: Jacob Keller <[email protected]>
Currently, during sidewise spark, the clone cleaves and boss cleave are
collected. Two callouts are made.

As soon as the first two cleaves are collected, a callout is made for
the first two safe spots. Then, once the boss cleave is collected, all
safe spots are called out.

This is problematic, because players will see callouts like:

E => S
E => S => W => N => W

The actual clone hits are about 2 seconds apart. By the time the final
callout is made, the first clone will have already hit, and the second
clone will be about half a second away from hitting.

This requires players to remember that the data for the first 2 spots is
effectively useless, and that they really need to pay attention to the
3 final hits.

Instead of calling all 5 hits, just call out the last 3.

Based on the raid simulator, we are making a call while the 2nd hit is
about 0.5 to 1 seconds from hitting. We *could* refactor to delay this
call until the second clone has hit. However, this gives the player only
~2 seconds of reaction time from when we make a call like "W => N => W"
where they need to be west.

I am unsure which of these is better, but at a minimum we should stop
repeating information which makes the callout more difficult to follow.

Signed-off-by: Jacob Keller <[email protected]>
The way clones and the boss hit for sidewise spark often results in an
intercardinal spot being safe for multiple cleaves. When making a call
for a safe spot, if an intercard is safe for the entire sequence, just
call that intercard.

For the collected callouts where there may be a future callout with a
new safe spot, include the number of hits. This should be two, though
the current code has support for anywhere from 2 to 5.

For the final hit after the boss cleave, just call the direction and
"stay", so its clear the player can just move to that intercard and stay
without needing to keep track of a count.

Signed-off-by: Jacob Keller <[email protected]>
@jacob-keller jacob-keller force-pushed the jk-r4n-clone-callout-improvement-v2 branch from 059e293 to 6929855 Compare October 13, 2024 02:52
@github-actions github-actions bot added the needs-review Awaiting review label Oct 13, 2024
@jacob-keller
Copy link
Author

Ya, right now it only collapses if all 3 can go the same spot. I think that's better than trying to refactor to do NW => E or what not.

Yeah, I think that's a totally reasonable approach.

Everything else you pointed out should be fixed now!

@xiashtra
Copy link
Collaborator

This looks good to me, if @wexxlee doesn't have any additional comments.

@xiashtra
Copy link
Collaborator

I'm going to go ahead and approve this one, it seems to be working fine.

@github-actions github-actions bot removed the needs-review Awaiting review label Oct 23, 2024
@jacob-keller
Copy link
Author

based on the recent work in m4s, I discovered that we can actually use ActorControlExtra for getting the clone cleaves. I think that is likely more efficient, but will wait for this to merge to do a follow up PR.

@xiashtra xiashtra merged commit f02a1bc into OverlayPlugin:main Oct 23, 2024
11 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 23, 2024
This is another attempt at improving the sidewise spark callouts that
hopefully is acceptable to everyone.

I dropped the sequence strategy, and just focused on the collapsing of
intercardinals during the two callouts, and on avoiding repeat callout
of
already called or hit spots.

- **raidboss: introduce helper function for stored cleaves**
- **raidboss: pass final sidewise spark hit through getCleaveDirs**
- **raidboss: avoid re-calling previous clone cleave safe spots**
- **raidboss: collapse multiple sidewise spark hits to intercards**

---------

Signed-off-by: Jacob Keller <[email protected]> f02a1bc
github-actions bot pushed a commit that referenced this pull request Oct 23, 2024
This is another attempt at improving the sidewise spark callouts that
hopefully is acceptable to everyone.

I dropped the sequence strategy, and just focused on the collapsing of
intercardinals during the two callouts, and on avoiding repeat callout
of
already called or hit spots.

- **raidboss: introduce helper function for stored cleaves**
- **raidboss: pass final sidewise spark hit through getCleaveDirs**
- **raidboss: avoid re-calling previous clone cleave safe spots**
- **raidboss: collapse multiple sidewise spark hits to intercards**

---------

Signed-off-by: Jacob Keller <[email protected]> f02a1bc
@jacob-keller jacob-keller deleted the jk-r4n-clone-callout-improvement-v2 branch October 24, 2024 01:46
github-actions bot pushed a commit to ShadyWhite/cactbot that referenced this pull request Oct 24, 2024
…n#471)

This is another attempt at improving the sidewise spark callouts that
hopefully is acceptable to everyone.

I dropped the sequence strategy, and just focused on the collapsing of
intercardinals during the two callouts, and on avoiding repeat callout
of
already called or hit spots.

- **raidboss: introduce helper function for stored cleaves**
- **raidboss: pass final sidewise spark hit through getCleaveDirs**
- **raidboss: avoid re-calling previous clone cleave safe spots**
- **raidboss: collapse multiple sidewise spark hits to intercards**

---------

Signed-off-by: Jacob Keller <[email protected]> f02a1bc
github-actions bot pushed a commit to ShadyWhite/cactbot that referenced this pull request Oct 24, 2024
…n#471)

This is another attempt at improving the sidewise spark callouts that
hopefully is acceptable to everyone.

I dropped the sequence strategy, and just focused on the collapsing of
intercardinals during the two callouts, and on avoiding repeat callout
of
already called or hit spots.

- **raidboss: introduce helper function for stored cleaves**
- **raidboss: pass final sidewise spark hit through getCleaveDirs**
- **raidboss: avoid re-calling previous clone cleave safe spots**
- **raidboss: collapse multiple sidewise spark hits to intercards**

---------

Signed-off-by: Jacob Keller <[email protected]> f02a1bc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants