diasurgical / devilutionX

Diablo build for modern operating systems
Other
8.01k stars 786 forks source link

[Issue Report]: Griswold charges money, but doesn't repair item #7350

Closed saviit closed 1 month ago

saviit commented 1 month ago

Operating System

Windows x64

DevilutionX version

1.5.2

Describe

Sometimes, repairing items with Griswold doen't actually repair the item, but the cost of the repair is still charged. Immediately trying to repair the item a second time seems to always work correctly (item is repaired and gold is charged), but now you've still spent twice the amount of gold that you should have. Re-loading the save results in the same behaviour, though I haven't tested whether picking up different/new items before going to Griswold changes this.

I have now observed this multiple times on the same save file. Does not appear to be related to unidentified status of item, I recall seeing this happen on an equipped, identified Arkaine's Valor as well.

griswold

To Reproduce

  1. Have items in inventory with durability < max.
  2. Repair said items with Griswold in the order they appear on the list, starting from the top.
  3. Observe characters gold amount and whether the items are repaired correctly.
  4. In the attached save file, the last item on the list should present the buggy behaviour.

Expected Behavior

Item should be repaired

Additional context

Game save file: savefile.zip

qndel commented 1 month ago

How about including the save? 😢

saviit commented 1 month ago

How about including the save? 😢

I managed to spoil that save while alt-tabbing back to the game, so I don't have the exact situation of that GIF in hand anymore :( If I manage to reproduce the bug, I'll attach a savegame file here.

saviit commented 1 month ago

I've now reproduced the bug.

A consistent way to reproduce the bug on this save file seems to be to repair all the items in the characters inventory, in the exact order they appear in the repair menu starting from the top.

Relevant clip and savegame files:

griswold2

Save files: savefile.zip

StephenCWills commented 1 month ago

Ah, looks like TakePlrsMoney() calls TakeGold() which may call player.RemoveInvItem() if a gold pile is removed. When that happens, it pulls the rug out from under storehidx, which no longer points to the correct index in the player's inventory.

ephphatha commented 1 month ago

Expanding on Stephen's debugging, the item being repaired needs to be the last item in the inventory list. I can consistently reproduce this by taking all but 1 gold from a stack of gold and putting it in a new inventory slot (this way the 1 gold pile is taken from first when going through TakePlrsMoney()), then unequipping a damaged item and placing it in the inventory (so it gets added to the end of the list).

RemoveInvItem() then copies the last item over the list entry where the 1 gold stack was, and the repair ends up setting the durability of the now-unused original instance of the item which is 1 past the end of the list.