Lexer747 commented 4 months ago

The 0-ticking Patch (1.1)

New support for getting the correct weapon stats when switching weapons on the same tick that a player attacks.
Adds Salamander Support 🎉.
Adds Harmonized 4t Support (only triggers when using standards).
- Further improvements to powered staff detection in general required to enable harm.
Basic Red Keris special attack support.

I do understand this is a big patch, therefore from the get go, please just say if you want this split into smaller more reviewable PRs. Essentially it could be reasonably broken down into one PR per top-level bullet point above, I am more than happy to do this work if you would like. Please just say!

Implementation details:

The quick swapping support (known as 0-ticking in the PvP community), is a helpful feature I wanted for rooms like Nylos and Baba-puzzle where many frequent and fast swaps happen. I noticed that the underlying cause of the problem was that we are essentially caching the results of getWeaponStats() and related functions when the onGameTick() subscription was triggered. Unfortunately this subscription happens at the start of a tick so you could very easily create a bug like so: (Youtube link: https://youtu.be/t_2h5uJM5kk or uploaded to github as an .mp4). As you can see if you attack and swap on the same tick the wrong cool down is triggered, e.g. blowpiping with a 5t timer above my head.

This change tackles the root of the problem by using a new a subscriber to listen a client int 384 which from testing will always monotonically go up as the client switches any worn item. This means we can use a fresh non-stale weapon id for the equipped weapons stats. This required some more finicky details, including a new state for the first tick of attack delay, this is so the plugin can only do a "weapon" swap. If it's early enough to not have already been rendered.

Next up is Salamander support, which turned out to just need a spot anim. As it turns out the ranged and magic attacks have no animation at all and simply trip up the isPlayerAttacking() code to think no attack was happening.

Finally the even more improved staff support, as I was fixing the 0-ticking I was noticing quite a few problems with d97172936b4e8f82ecdbb1666e9cd2634aa50e30 when staffs were involved. Especially when quickly swapping and manual casting. So I tackled it from a bunch of angles trying to reduce the false positives and false negatives that come from the current solution.

This starts with the new PoweredStaves.java file (which renamed from WeaponIds.java but git has lost the rename), the first step of this is too explicitly link every staff to it's corresponding animation and projectile. With this new information we can then already be more accurate by only switching to 4t when the animation and weapon Id match, whereas the old version just looked at id and animation separately.

Next there's some new data Spellbook.java and CastingSoundData.java which allows the plugin to correlate which spell book the player is on and if that animation can be manually cast. Giving a us a really big win in 99% of cases. This is because most things people do they're on thralls/ancients/lunars and all the powered staves tend to use the standard spellbook animations. We also use the sound to help detect manual casts, but a lot is still left on the table here as we should also be detecting the sound of the staff being used too, but I haven't got around to implementing that.

The other reason I haven't worked on the sound side of it too much is that it doesn't work when the client is muted so it feels half-baked in premise.

The last piece of the staff puzzle is the most controversial because it uses the deprecated API getProjectiles(), now it's not documented why this API is deprecated or the replacement to use instead, so my gut says this patch may need this part being ripped out because this is breaking some jagex rules. And for the record I'm more than ok ripping it out, due to how to ends up working. Which I'll explain now:

This last piece is that using the projectiles we can check on the next tick what projectile the player fired, if it doesn't match the staff then they must have been casting a spell instead. Note that this whole section is what enabled Harm as we can tie to specific animations and projectiles to be sure the player is casting something which will be upgraded to only 4 ticks instead of 5, like curse or bind.

Misc details:

Version bump the whole plugin to 1.1 since this a big change. New subscribers, different state management, etc.
Fixed a small bug with blowpipe tailing animation being picked up as a second attack (see inline comment in isPlayerAttacking().
Added @ngraves95 to more copyright headers. (I've just assumed it's all copyright circa 2022 because checking the history was a touch too tedious for me).
Added some very basic sanity check unit tests which while adding I discovered that I couldn't run them locally. This is because Runelite has moved to lombok:1.18.30 instead of lombok:1.18.20, this is a breaking change with the V7.4 of gradle (Fails with: java.lang.IllegalArgumentException: Unsupported class file major version 65). Therefore to match the lombok version we just have to bump the gradle version to 8.X.

Testing

I test a bunch of random weapons here https://www.youtube.com/watch?v=TC6O1SSEwTw if you would like me to test any more senarios or ideas you have just let me know! More than happy to verify things are good.

Lexer747 commented 4 months ago

@ngraves95 please could you have a look at your convenience, thanks in advance :)

ngraves95 commented 4 months ago

Nice improvements - please also squash before committing. I think a blocklist of common non-attack animations is the right call here. mostly looks good but a few things:

Let's make sure that checking instanceof NPC doesn't break PvP/LMS.
I'd prefer not to use the projectile tracking sections if possible because the API is deprecated.

Lexer747 commented 4 months ago

Good spot on the PvP thing (and thanks for the review in general), it not only doesn't work, but needs a separate branch because we can't check a player object for all the things we use on NPCs (Attack option, etc). However the fix is relatively easy, see the latest version of the code.

Before	After

The projectile code is now also removed (just the deprecated code and calling functions), I've left the data in just in case we ever add it back in.

This PR now squashed into one commit.

ngraves95 commented 4 months ago

I was just testing Trident of the Swamp and its showing up as 5t. Let's double check there are no regressions on powered staves after removing the projectile tracking code.

I think it might be because the tridents use a standard spellboook wave animation.

Lexer747 commented 4 months ago

I was just testing Trident of the Swamp and its showing up as 5t. Let's double check there are no regressions on powered staves after removing the projectile tracking code.

I think it might be because the tridents use a standard spellboook wave animation.

Which spell book where you on when it was 5t? (standards?)

I think I've realised the regression regardless which is that if the spell book does match the stave built-in spell it now defaults to a manual cast instead of the inbuilt spell 🤦‍♂️ That should be reversed to match the old method. We could do better here by matching the sound of the staff built in spell but that still isn't 100% reliable because it doesn't work when sound is off.

The fix is probably just this, because this check only made sense when we had the projectile code in place. Let me test it though.

@@ -309,7 +309,7 @@ public class AttackTimerMetronomePlugin extends Plugin

     private int getWeaponSpeed(int weaponId, PoweredStaves stave, AnimationData curAnimation, boolean matchesSpellbook)
     {
-        if (stave != null && !matchesSpellbook)
+        if (stave != null)
         {
             // We are currently dealing with a staves in which case we can make decisions based on the
             // spellbook flag. We can only improve this by using a deprecated API to check the projectile

ngraves95 commented 4 months ago

I was just testing Trident of the Swamp and its showing up as 5t. Let's double check there are no regressions on powered staves after removing the projectile tracking code.

I think it might be because the tridents use a standard spellboook wave animation.

Ok, I managed to fix this by adding a check for stave.getAnimations().contains(curAnimation). Also validated that manual casting still shows up as 5 tick (earth bolt manual cast-> 5t, trident of the swamp autocast -> 4t).

I'll merge this and then add the fix locally.

I'm still seeing some issues where pickpocketing while in an alch animation triggers the timer but that feels like a low priority. I'll get around to adding alch and other utility spells to the blocklist

Lexer747 commented 4 months ago

Oh yeah that's actually a better fix than my small diff, slightly more strict on which animations can trigger this false positive - I like it.

Ah - Alching while pick-pocketing! I didn't think of that 😆

Lexer747 commented 4 months ago

092130b (the stave.getAnimations().contains(curAnimation) fix) testing:

Builtin ✅	Wind Blast ✅	Fire Wave ❌

We do expect the wave to fail since the animations are the same, good enough without sounds or projectiles.

ngraves95 commented 4 months ago

092130b (the stave.getAnimations().contains(curAnimation) fix) testing:

Builtin ✅ Wind Blast ✅ Fire Wave ❌

I dont think there's anything we can do about wave animations colliding with powered staves right now until we get an non-deprecated projectile tracking API. The overlap of people casting wave spells while using a powered staff should hopefully be almost 0, though.

ngraves95 / attacktimer