Devilutionx: Fix double-casting bug

[julealgon]

When casting a spell or using a scroll through the spellbook, players are allowed to cast 2 times in rapid succession, resulting in 2 distinct bugs:

1. Scrolls can be used twice (even though they are consumable, single-use items)
2. Spells can be cast without sufficient mana

At first, I thought this was related to animation skipping (#779), in particular this section on player.cpp which interrupts the spell casting animation and starts it again if the user is quickly casting:

https://github.com/diasurgical/devilutionX/blob/9e1428dd0b7738c0c305ffaa0b7e7882736c94f0/Source/player.cpp#L3256-L3272

However, even without this skipping in place, it is still possible to perform double casting and get to negative mana (or use a scroll multiple times):

| Spells | Scrolls |
| --- | --- |
| | |

I'm sure this is a single-frame type of exploit, but wasn't able to find what exactly is causing it yet.

[AJenbo]

My understanding is that this happens as casting is an event that gets qued, mana check is done when generating the event, but mana isn't subtracted until the event fires. So you have the time between the event being started and then being processed to fire another spell.

It's really nice with the gifs in the issues to better understand what is happening ingame.

[julealgon]

My understanding is that this happens as casting is an event that gets qued, mana check is done when generating the event, but mana isn't subtracted until the event fires. So you have the time between the event being started and then being processed to fire another spell.

Interesting. I followed the code from the right mouse button event into the mana subtraction logic and it appeared to be happening in the same frame, so technically there would not be any time to allow for the second casting.

I was wondering if this bug was related to a potential second input reading that happens later in the frame that would skip the mana check. I'll go back to this at some point see if I can make it work without too many changes.

It's really nice with the gifs in the issues to better understand what is happening ingame.

I figured I'd be opening a few graphics/animation-related issues so I searched for a gif program that could record a portion of the screen. Ended up installing this one:

• https://www.screentogif.com/

Works well enough for me :)

[julealgon]

I was debugging this a little bit more yesterday and I stand corrected: you are right @AJenbo , this is because the check to see "if the player can cast" is done very early, on the frame of the action, however the mana spending logic only executes on the hit-frame, usually 10 or more frames later.

If the user then attempts to cast a second time before the hit-frame comes, the game passes the checks and "queues" the action.

I think there is a variety of ways this could be fixed. Looking for feedback here:

1. Disallow queueing the second cast if the hit-frame has not happened yet.
   We could add some sort of "flag" indicating whether the actual cast has happened or not (if the hit-frame for the spell/mana deductions/etc happened. Then, if the current player mode is "casting", we could check this flag and avoid accepting the new action.
2. Check mana/cursor again on spell start
3. Change the "followup action" logic to actually send a new event to the beginning of the pipeline, running all checks again

3 is probably the nicest way to do it but the most complex. I'll look into this problem at a later time.

[AJenbo]

1 sounds like it would affect game play since the game wouldn't react to the input for that time.

2 sounds like a simple and good solution

3 sounds complex and I'm not deep enough in to the details atm to understand the potential consequences, or what this solves compared to 2, but you probably didn't suggest it if there wheren't any benifit to it.

[NiteKat]

Checking the cursor again on 2 could impact game play. As a player, I'm expecting to be able to move my mouse after I right click; however, checking mana again on spell start may be a good idea. Could probably do something similar to stop double casting of scrolls and staves, and actually, there's a whole other bug with staves where you can cast without using up charges that's related to this (is there an open issue for that?)

[julealgon]

@AJenbo

1 sounds like it would affect game play since the game wouldn't react to the input for that time.

Yes. While testing this there is for sure an impact on playability. It works better when #779 is implemented, since you have all the "recovery frames" to act again, but is still not optimal. Player actions that happen before the hit-frame are ignored.

2 sounds like a simple and good solution

I tend to agree, but only considering the current codebase. See explanation for 3 below.

3 sounds complex and I'm not deep enough in to the details atm to understand the potential consequences, or what this solves compared to 2, but you probably didn't suggest it if there wheren't any benifit to it.

The main reason why I think something like this would be the optimal strategy in the long run, is that it is architecturally simpler. What I mean by that is that it avoids the need to perform the same checks in distinct places of the code base, leading to an overall simpler approach (ironically). The problem with the game now is that some of the logic is done close to the input reading logic, instead of being decoupled. Ideally IMHO we'd have the concept of commands (game seems to have some of it, but it's flawed), and they would be queued and executed. The logic to check pre-requisites (like available mana) would be there, and as long as "command execution" actually happened in sequence, checks would just work.

Current engine is very messy in this regard and that's what leads to so many frame-related bugs. Instead of decoupling input from commands, it actually "executes" the command even if the player is already executing another command. Then it sets that destAction flag as a sort of input buffer. That's incredibly messy in my view. The input buffer should just buffer commands to be executed, and then they should e executed one after the other (for commands that cannot be parallelized).

However, this is by far the most difficult option (it's not even comparable). I have a strong feeling that we should first refactor some of this nasty codebase before attempting that, otherwise it will just end up creating even more bugs.

---

@NiteKat

Checking the cursor again on 2 could impact game play. As a player, I'm expecting to be able to move my mouse after I right click;

My bad for not being clear enough. When I meant "cursor check", I actually meant the cursor type, not it's position. The game currently has a few "early returns" when casting spells:

1. Do you have a skill selected?
2. Are you in town?
3. Do you have enough mana to cast it?
4. Do you have any active cursor that prevents you from casting it?

These checks are performed in the same method, CheckPlrSpell.

Cursor:
https://github.com/diasurgical/devilutionX/blob/9e1428dd0b7738c0c305ffaa0b7e7882736c94f0/Source/player.cpp#L3629-L3641

Mana (inside CheckSpell):
https://github.com/diasurgical/devilutionX/blob/9e1428dd0b7738c0c305ffaa0b7e7882736c94f0/Source/player.cpp#L3644-L3648

So you should be able to move the cursor just fine if we added the check in the hit-frame as well.

[julealgon]

@NiteKat

and actually, there's a whole other bug with staves where you can cast without using up charges that's related to this (is there an open issue for that?)

First time I'm hearing about this. Can you elaborate on what the bug is? I don't think there is an issue for it, so would suggest just documenting it as one and referencing it here.

[qndel]

If you cast a spell from staff and select a different spell before the spell finishes casting, it doesn't consume a charge

[julealgon]

Hey @AJenbo / @qndel , can we add the vanilla tag on this one?

[julealgon]

Ok folks... quick update here.

I've tried to go with approach 2: "re-check conditions on cast", however I just cannot make it work for "direct use" scrolls without massive changes in how items are used.

I proceeded to extract this logic from CheckPlrSpell into a reusable method, so that I could fire it from inside StartSpell as well:
https://github.com/diasurgical/devilutionX/blob/7108145a04c902a1638e93d937e48c47549257d0/Source/player.cpp#L3643-L3655

This works fine for spells and scrolls used like spells, but scrolls as items are a whole different matter. Turns out using a scroll through right-clicking/belt hotkey triggers different logic compared to using them as an actual spell, and I can't make a check that works for both from inside StartSpell.

First, for some really odd reason, the SpellType for scrolls in the inventory is not set to RSPLTYPE_SCROLL, but instead is set to RSPLTYPE_INVALID. This makes absolutely no sense to me but I have not yet figured out the possible behavior differences of setting it to RSPLTYPE_SCROLL. This is done inside UseItem:
https://github.com/diasurgical/devilutionX/blob/3227fea5c0926a661e38256600e37579605e4ff4/Source/items.cpp#L3231-L3264

Example:
https://github.com/diasurgical/devilutionX/blob/3227fea5c0926a661e38256600e37579605e4ff4/Source/items.cpp#L3234

After "fixing" this "problem", I then realized my check was still not working. The scroll check inside CheckPlrSpell is done through UseScroll, which checks to see if you have a scroll of that spell in your inventory or belt. The problem is that, when using scrolls directly, they are removed from the inventory in the same activation frame, not in the hit-frame... when StartSpell is called, it is already too late and the scroll doesn't exist in the inventory anymore, so there is no way to currently prevent the player from "casting again": even the first cast will be skipped using this logic.

The only "simple" way I see now for solving this would be to disallow scrolls in the belt. If that's acceptable, let me know.

Since I wasn't able to solve this for all similar cases, I'm inclined to not send any PR for this.

Let me know if you think it is a good idea to fix this problem only for spells. Personally, I don't like it. Feels hacky to me to solve for one case and not the other.

---

Things I'm thinking about after all this nonsense:

• I don't like that scrolls are removed right away from inventory when used
  This is inconsistent with how mana is consumed for example. Mana is actually consumed when the missile is fired, not when the player first uses the spell. Missiles are only fired on the hit-frame. Scrolls however, are "removed right away" even before the animation starts. Attempting to consume the scroll on the hit-frame today would not be possible in the case the scroll is used directly (inventory or belt), because there is no reference to the item that cased the spell to fire at that point. I'll open an issue about this.
• I still don't understand why spell type of scrolls is set to invalid
• UseMana calls RemoveScroll.... great design/naming Blizzard...
• This code is getting on my nerves and I'm seriously contemplating the idea of porting all of it to C#
  There is no "go to implementation", meaning all navigation has to happen using CTRL+F. Every file has multi-thousand lines of code, so seeing diffs and going through my own changes is a nightmare. Testing small changes takes a long time due to no edit-and-continue, so on and so on... I know mostly of this is standard C++, but still :(

[AJenbo]

Since the inventory is static memory, thing does not get removed, instead they get set to invalid.

There is almost no C++ in the code, it's all C except for a handful of lines. And it's not well structured C either. I have had the incline to refactor it on several occasions, but that is pretty much a nogo until we finish porting Hellfire as it would make merging a nightmare. Also, we need more tests befoer we can really start doing that.

Oh and C# is also a nogo, it's not at all portable enough :)

p.s. we can't really disallow scrolls in belt either, but good point about scrolls being consumed at an odd point in time.

[julealgon]

Since the inventory is static memory, thing does not get removed, instead they get set to invalid.

Yep I'm aware of that. The point I was making is that scrolls are created with a spell type set to invalid on purpose, for some reason.