Devilutionx: 'quick' and 'haste' speed modifiers do not work as intended

[julealgon]

Because of the way the game handles frame skipping to increase attack speeds, some modifiers in the game end up having wrong effects on the final attack speeed.

For reference, here is the logic for each attack type that is affected by speed modifiers:
| Function | Logic |
| --- | --- |
| PM_DoAttack | https://github.com/diasurgical/devilutionX/blob/4a4486d6161028e69702bedec6cb071e6a5354c3/Source/player.cpp#L2617-L2629 |
| PM_DoRangedAttack | https://github.com/diasurgical/devilutionX/blob/4a4486d6161028e69702bedec6cb071e6a5354c3/Source/player.cpp#L2703-L2709 |

Notice how all frame skipping for both attacks always check frame 1 to decide how many frames to skip. However, these 2 methods are most of the time not fired for frame 1, and instead only begin to execute on frame 2 onwards. This has the effect that all frame skipping logic that is based on frame 1 is actually discarded. Effectively:

| Suffix | Modifier | Intended frame skips | Effective frame skips |
| --- | --- | --- | --- |
| "of readiness" | quick attack | 1 frame | 0 frames |
| "of swiftness" | fast attack | 2 frames | 1 frame |
| "of speed" | faster attack | 3 frames | 2 frames |
| "of haste" | fastest attack | 4 frames | 2 frames |

This behavior is caused by the way the game handles animation increments (see #838) as well as how attacks work when starting from a standstill. When performing player updates, animation transitions from several player modes cause the logic to repeat during the same frame depending on the return value of each processing function:
https://github.com/diasurgical/devilutionX/blob/4a4486d6161028e69702bedec6cb071e6a5354c3/Source/player.cpp#L3416-L3454

Notice how each processing function can decide whether or not the loop should continue running, or if it should be aborted (variable tplayer). However, the CheckNewPath call does not provide this same functionality, as it does not affect tplayer which will always be false when it returns from the standing mode, as can be seen here:

https://github.com/diasurgical/devilutionX/blob/4a4486d6161028e69702bedec6cb071e6a5354c3/Source/player.cpp#L2075-L2078

CheckNewPath is where all requests to attack come from, meaning, whenever an attack animation is triggered from a standstill, the loop finishes and animation is incremented to frame 2 before PM_DoAttack has a chance to execute. Thus all logic that depends on frame 1 inside the attack functions will never execute.

There are a few ways to have frame 1 dependent logic to execute today, for example, attacking after walking. However, this is not how most normal attacks take place and thus why this problem is so impactful to weapon speed.

Here are a couple of saves to test the weapon speed behavior:

• Warrior (5 swords, one for each speed): single_0.zip
• Rogue (3 bows, one for each speed): single_1.zip

[galaxyhaxz]

Already a well known bug blizzard had a B's claim to fix it in 1.07 but didn't. The theory is that 4 frame increase would make melee attacks overpowered, so they didn't fix it.

[julealgon]

The theory is that 4 frame increase would make melee attacks overpowered, so they didn't fix it.

@PrisonOfMirrors final effect compared to vanilla is actually 3 frames, not 4. You have to count the first frame which was being skipped before.

[galaxyhaxz]

I'm still all for fixing this bug, imo it's not op warrior is super underpowered compared to the sorceror anyway.

[FitzRoyX]

I've come to agree with fixing this, mostly because modding needs to make sense. You just can't have enchantment values doing nothing or having their own hidden logic. Any changes this may cause can be modded "back to how it was" by doing away with two affixes and lowering the base attack duration for all characters by 1.

I think it's very likely that Blizzard themselves were stumped by this. After all, the same team also claimed to have been surprised by the Hellfire team's ability to increase walkspeed in town.