(On Hold) [melody] non-casting state between songs

[cord-b]

What

• This makes the player in a non-casting/singing state during the small gaps between songs in a Melody, by making the stop_current_cast happen sooner.
• Previously, the Melody would do a frame-perfect stop_cast/cast combination right as the next song was ready to cast.
• This does not alter the timing or speed of Melody in any way, because the Cast event still happens on the regular timing.

Supporting changes

• Adjusting the StopCast and StopCast callback logic slightly to tolerate non-song events and handle/ignore them properly, if they were to occur.

Visualization / Example

This visualizes the effect of the change on the player state:

(Song 1)                   (Song 2)
!cccccccccccccccccccccc!sss!ccccccccccc...  // before
!cccccccccccccccccccccc!   !ccccccccccc...  // after

Legend

• 'c' is time spent casting
• 's' is time spent singing
• '!' denote a spell cast beginning or end

Why

Previously, stopcast was not invoked until the startcast of the next song. This caused the melody to be completely airtight in terms of the player always being in a casting state. This differs from standard twisting mechanics where there is a small window of non-casting between songs (the gap between /stopsong and /cast #). This window between songs was often utlized by bards to use clicklies like Breath of Harmony by timing their clicky right in this window, without interrupting their twist rotation.

This is not an attempt to add any smart-clicky support, features, automation, or scope creep for Melody. This is just to allow the client to be in a non-casting state in the small gap between songs, which more accurately mirrors how song twisting worked, allowing players the same access to techniques that they used to be able to use while twisting back in the day. I see this more as a feature-parity style change, but would make people fairly happy with being able to use their clickies during a Melody (but still requires fairly precise user control to pull off).

Test

I did initial testing on this and it seems to not break melody behavior after I added the supporting changes. Without the supporting changes, Melody would sometimes repeat a song because some clickies emit a "reason 3" stopcast event for their spell ID, which we should simply ignore by checking that it wasn't the melody song that we got the event for.

Tested/working with:

• Breath of Harmony (doesn't cause a "reason 3" stopcast event)
• Jboots (causes a "reason 3" stopcast event)
• Casted spells like Pegasus Cloak / Illusions (usually has a "reason 3" stopcast event)

[CoastalRedwood]

The changes look reasonable (and allowing manual insta-click in the 150 ms timing window until the next song would be hard to claim is automating things). I don't entirely understand the timing updates of the ActorInfo->CastingSpellId and whether stop_current_cast() is going to get repetitively called until that transitions (maybe harmless). Executing the stop_current_cast() immediately after the start of cast didn't appear to cause any issues (not sure if there could be a race condition, but if we see one, we could just push that stop cast at some fixed few 100 msec after the start_of_cast_timestamp).

I went through the test cases at the top of the melody.cpp, and those all worked fine along with some limited weaving / kiting to confirm general functionality. I didn't see any new issues. The timing vulnerability of clicking on a spell gem at the end of a melody cast is still there.

[cord-b]

Tonight I can play around with some debug logging around stop_current_cast() to confirm. I believe it was not being repetitively called, which makes me believe the client immediately updates ActorInfo->CastingSpellId locally. But it's worth being 100% sure and defensive against making the client not do unecessary/unusual behavior, even if it seems harmless.

[CoastalRedwood]

One item I did notice after my comments above is that if I was early with a macro set to "/useitem 16" to trigger a SS BP invigorate, it would abort the current melody cast (current_index) and then skip it without performing the Invigorate cast. The melody would just advance to the next song.

If I timed it well and did /useitem in that gap, invigorate did cast and the melody resumed after w/out obviously skipping anything.

I didn't test an insta-click like journeyman's boots.

[cord-b]

Ah I didn't test anything with /useitem, just via direct hotkeys or clicks. I'll see if maybe /useitem needs some defensive logic to do nothing if you are already casting something, maybe it's something strange on the /useitem side that is acting oddly with melody. I assume the behavior we'd want is that nothing happens if /useitem is pressed at the wrong time.

[neyacinu]

What would be the impact of allowing useitem in the melody itself (if even possible)? Can anyone see any negatives or possible exploits if this was allowed? Is it too much of an automation? The positives are obvious, I’d think.

…

On Mon, Sep 2, 2024 at 7:52 PM Cord B. ***@***.***> wrote: Ah I didn't test anything with /useitem, just via direct hotkeys or clicks. I'll see if maybe /useitem needs some defensive logic to do nothing if you are already casting something, maybe it's something strange on the /useitem side that is acting oddly with melody. I assume the behavior we'd want is that nothing happens if /useitem is pressed at the wrong time. — Reply to this email directly, view it on GitHub <#81 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6UYBWCTDGDJJKMCLVUB2JDZUUB3RAVCNFSM6AAAAABNPCNETKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRVGQZDIMZUHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ImperiumTakp]

i was going to suggest extending melody so you could do /melody 129 1 2 3 5

where 1** is the /useitem

[CoastalRedwood]

This was raised as a suggestion by Moliere in zeal-suggestions on Discord and received a negative reaction. If you want to revive that thread and get it approved by the powers that be, it could be a future enhancement PR, but I don't think it is needed for this PR (which is clearly not automating anything).

If this PR can make /useitem simply not abort/skip if used too early, then it will at least allow some precise timing use of /useitem without having to stop/start melody.

[neyacinu]

Agreed, Sherra. My query was more a thought experiment for those who are developing this to see if pursuing an approval would even be worth it. I tend to be very cautious about anything that could be used to automate any class outside of nice to have QOL issues, and am extremely cautious about bards even though I main one. If this PR goes in, it would be welcome of course, but the question then is what is the fallout if we just allowed melody to useitem?

…

On Wed, Sep 4, 2024 at 9:56 AM SherraBard ***@***.***> wrote: This was raised as a suggestion by Moliere in zeal-suggestions on Discord and received a negative reaction. If you want to revive that thread and get it approved by the powers that be, it could be a future enhancement PR, but I don't think it is needed for this PR (which is clearly not automating anything). If this PR can make /useitem simply not abort/skip if used too early, then it will at least allow some precise timing use of /useitem without having to stop/start melody. — Reply to this email directly, view it on GitHub <#81 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6UYBWFIFWZHQOH5BB6TYY3ZU4NSTAVCNFSM6AAAAABNPCNETKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRZGI4TGMRWHE> . You are receiving this because you commented.Message ID: ***@***.***>

[cord-b]

Added the /useitem fixes in a separate PR, since those are a standalone improvement:

• Add defensive checks for useitem. #83

[cord-b]

So the only potential behavior change I noticed is that in a laggy zone (just EC basically), sometimes the earlier invocation of stop_current_cast() will cause the spell to not fire upon completion about 10% of the time.

The only workaround would be to delay the stop_current_cast(), and introduce an extra forced ~100ms gap between stop_current_cast() and cast(), but that might end up slowing down the overall speed of Melody if the time between songs gets longer.

A middle ground would be to put this behavior behind an option/flag, and players can choose to have the song-gap included, with either the small risk of misfires in EC or with a slightly longer pause between songs. Or maybe they can just specify the song gap length via config, etc.

I think this is the main thing that needs to be figured out before deciding on these changes.

[CoastalRedwood]

There is the existing forced delay of 150 ms from when the casting window drops until the next song is started. I think Salty picked that empirically.

What if you delayed the stop casting call until the casting visible window drops?

Just add a (current_timestamp > casting_visible_timestamp) check along with the "is casting" ones before calling stop_current_cast(). That might eliminate 99% of the 150 msec casting dead time you want to open up for a manual clicky while ensuring completion of melody step.

[cord-b]

What if you delayed the stop casting call until the casting visible window drops?

The code section calling stop_current_cast() is already after Zeal::EqGame::Windows->Casting->IsVisible is false. So it turns out it's already waiting for that.
It seems that there's a latency factor where if the zone is laggy enough, it can throw off the server's timing between start/stop cast timestamps and it can prevent the song from firing if it disagrees on the timing. So even though the client is always waiting at minimum ~3016ms between the start/stop cast, it can fail on the server side under high load thinking the stop was too soon.

[CoastalRedwood]

Yeah, you are right that it is after the visible check. It's been a while since I touched that.

I agree that it is likely a client / server sync issue. I don't know if there is another server -> client message that confirms the song fired (completed) server side. Or alternatively if there was a confirmation from the server that the cast had started, which could then be used for the 3 sec offset to do the stop. These are Salty questions.

[cord-b]

AFAIK the self->ActorInfo->CastingSpellGemNumber == 255 is basically the server confirmation that the song fired, which was in the original logic. This happens around 100-125ms after the window disappears, at least with my 90ms connection.

The safe way is to wait for the confirmation, but we end up back in the original boat of stopcast/startcast basically happening at the same time and the gap being effectively too small.

Would defer to Salty if this is still worth pursuing. If we want to allow users to add an optional/additional gap via config, I could try that, or something.

[CoastalRedwood]

I chatted with Salty, and there wasn't a clear way to sync things better to know when to execute the stop cast. The current 150 ms delay works 99.9% of the time. Increasing the delay between songs to add some additional non-singing gap would make a lot of Bards grumpy, as even now a 4 song rotation can drop a buff with unlucky timing.

cord-b, can you try out this and see if it is an acceptable alternative to weave in clickies? It does require dedicated macros for each clicky inventory slot.
(1) Create a macro (and optional hotkey) with stopsong and useitem
/stopsong
/useitem 16 (BP or other clicky)
(2) Create a macro hotkey with
/stopsong (try with and without)
/mel # # # #

And then you should be able to hit (1) whenever you want to use the clicky and (2) after to restart the melody.

Another alternative might be adding melody pause / resume commands and a macro like below, but I'm not sure it's worth it:
/melody pause (set flag, stop current cast)
/useitem 16
/melody resume (clear flag, melody resumes when the casting box isn't visible)

or could add a /melody extra_delay value, but that's also making things complicated.

At the very least I think this PR has some cleanups of the stop_current_cast that could be worthwhile even if the stop_cast timing is reverted.

[cord-b]

Yeah I could split the PRs into

(1) Improving the helper functions to be more spell/song tolerant against skipping ahead/backwards, especially if additional changes are to be added.

(2) It sounds fine by Salty/Secrets, as an acceptable alternative due to the technical limitations, we let the /useitem, if used during a melody, would pause the melody, use the item, and resume the melody at the current song. Then players could just press that near the start of the song for the same effect as if there was a manual gap.

An alternative to (2) would be a dedicated /pausemelody 20 that is similar to stopsong, but stops current melody/song for 200ms (or whatever duration is given, eg 20 = 200ms), giving enough time for a /useitem to execute within the same social macro, and then the melody would automatically start back up after that 200ms gap.

Effectivelly I think it's the same as (2) but with more steps, so maybe (2) would just be simpler.

[CoastalRedwood]

I agree on splitting it and getting the (1) fix in.

For (2), I can't speak for Secrets, soI think it's worth making a Zeal Suggestion on Discord making it clear that it simply "interrupts" the current melody cast and pauses restarting melody for XX seconds and is entirely a manual trigger. It would be a /melody pause command probably.

[cord-b]

I think what I'll do is actually override /stopsong to optionally accept an argument. That way one macro will work both if you're using a melody or if you were currently manually singing a song:

• /stopsong #
  • If a melody is active, pauses the melody for # duration.
  • If a non-melody song is active, cancels the song like normal.
• /stopsong fallsback to the original /stopsong implementation.

[cord-b]

So while working on the extracting defensive logic and testing every timing possible, I came across that same timing vulnerability in the client and realized what is happening. This probably needs to be fixed if we're working on allowing players to trigger even more song events between songs, or pausing melodies right at those moments, etc. Because all of that relies on not triggering this glitch all the time, otherwise it's really annoying.

That being said, that fix only matters if we're putting in the time for things like Melody Pause feature, which I have working, barring this bug from being so frequent.

For a while I was thinking this was a bug with Melody when I noticed that sometimes it immediately re-casts certain songs when you try to stop them, but it's actually just a glitch that's part of the client caused by packet race conditions. The Melody is actually trying to stop, as it should, but the server keeps sending the wrong state to the client and tricking the client into "casting" a phantom spell that was already actually interrupted.

Here's a reproduction:

1. Make your client Cast a spell.

• The client begins casting.
• The spell cast bar appears.

2. Make your client send a StopCast (eg click a /stopsong macro) or similar event almost immediately (like <30ms).

• The client stops casting.
• The spell cast bar disappears.

3. Very shortly after, the first server response arrives, acknowledging the "Cast" started.

Current (Bugged) Behavior in the EQ Client:

• The client is informed/acknolwedged by the server that it is casting. The server hasn't seen the StopCast packet yet, so it's still thinking the Client should be casting and telling the client to be in a casting state.
• The client, trusting the server, puts itself back into the casting state for the song that it just stopped.
• The cast bar starts again and the song or Melody continues now that it looks like a spell is in progress.

Ideal Behavior (Instead of the above behavior, we should fix it to):

• The client should know it isn't casting spell X because it just recently sent a StopCast(X) for that spell.
• With this knowledge, the client should ignore the server from putting it back into a casting state.

4. The server gets the StopCast packet.

• The server doesn't inform the client that the Cast was aborted.
• The client is still showing the cast bar, thinking the cast is still in progress.
• The cast bar completes.
• Nothing happens / the song effect doesn't fire.
• If this bug happened without Melody, the client will just sit in the "singing" state. Even in the "singing" state, the song will not pulse.

---

So basically, when there is fast succession of start/stop casting packets like trying to cancel a Melody song that just started, the Client and Server get desycned very often, making the client continue casting the "phantom" song even though it was correctly stopped on the server.

We'll probably want a 3rd branch to address the timing vulnerability issue. It could maybe be done by adding a Packet callback that checks to ignore this specific packet under these very specific conditions, assuming that doesn't disrupt anything else in the Client, which I'm unsure of. It could be a simple fix if that works though, at least if it's limited to the scope of handling Melody related Casts/StopCasts.

Caveats: I think we need to very careful if addressing this behavior, however, because if we happen to address the desync incorrectly and cause a reverse desync (Client thinks it isn't casting, but actually is), and it happens to be a spell (not a bard song), then we might allow the Client to do illegal operations, like moving items around while casting spells, which could look like hacks. So if we are to patch this, I would say we only address this specifically for bard songs, as those do not cause the client to be blocked from doing stuff like moving items, attacking, etc).

---

Alternatives

Because doing a "Melody Pause" requires this fix, an alternative short(or long-term) fix would be:

• If the client sends a /useitem # command within about ~300ms of the end of a song (or during the intended gap), the Melody will treat this as if it arrived during the gap. It will carefully stop the casting state at an appropriate time to not trigger the vulnerability before clicking the item, and then continue with the Melody. The main difference here is we don't need to pro-actively force the client into the non-singing state. We just operate normally, but if the /useitem arrives in a timely window prior to the next "Cast()", we can let the Melody safely work around the timing vulnerability by giving it minor discretion on the exact timing.

To me this seems like maybe the best route. Instead of trying to wrestle with the client's casting state and these non-casting windows, we just allow the user to click a /useitem in an appropriate time window where it would more or less be allowed, and let Melody do the actual manipulation to make it work properly and not bug out, without having to force the client into a bunch of states.

[cord-b]

Gonna wrap this thread up shortly and move over to the new PR(s).

1st new PR is up with the core logic improvements, without any of the behavioral changes. Should be pretty safe.
2nd PR with the clicky-related support will come after the 1st is done. I'm pleased with how it turned out. Hopefully it gets the blessings for use.