[CR] Fix clothes dropping

[RadHazard]

Fix for #23524 & #23586. Unfortunately, as it stands this currently reverts the fix to #23242

Marking as CR because the underlying issue is a lot more complex than it looks at first, and I'd appreciate input from people who are more familiar with the codebase.

Explanation of the bugs

Prior to #23242, taking off an item was a synchronous action. The armor layer screen relied on this behavior and expects that once the function returns, the item will have been removed. However, if the item doesn't fit in the player's inventory, the action is changed to the "drop item" activity, which is processed asynchronously starting at the next turn. This caused the bug where the armor screen didn't update correctly, as the item hadn't technically been "removed" yet before the next redraw. This is harmless but annoying.

#23242 fixed the issue by changing the logic to always take off the item, regardless of whether a drop action had been queued. This fixed the armor screen's refresh, but introduced two related bugs, depending on whether it was the player or an NPC that is dropping the item.

If the player is the one dropping the item, they immediately take it off it, which causes said item to tumble to the ground. Then, the queued drop activity fires, but because the item tumbled to the ground it has an invalid index. The game attempts to drop whatever is currently at the index, which causes weird and inconsistent behavior (dropping a None, dropping random articles of clothing, etc.)

However, if it's an NPC that is dropping the item, there's a hack in place (apparently because NPCs don't normally process activities) that causes the drop to be executed synchronously as well. Because the takeoff function hasn't yet taken off the clothing when this happens, the drop activity performs a second takeoff, which completes first (the activity screen uses a separate code path that doesn't check for inventory size, which is why it doesn't recurse infinitely). This invalidates the iterator the original takeoff function was using, causing an immediate CTD when it tries to dereference it.

Potential fixes

At this point, I'm not sure what the best way to proceed is. The simplest fix would be to revert #23242. However, this reintroduces the bug that that PR fixed. This is the fix I've currently implemented, but it's not ideal.

Another option would be to change the drop behavior to just synchronously drop the item. However, that involves code duplication and may cause other issues.

A third option would be to rewrite the way dropping is handled in this scenario. This is probably the best long-term solution, but it requires significant code changes:

• NPCs would need to be able to handle activities to avoid the hack that causes a CTD
• The drop activity would need to be changed to accept item references rather than indexes so that it can handle items that move in between queuing the activity and executing it
• There would need to be some kind of ephemeral place to store the item in between taking it off and properly dropping it, or it will just end up tumbling to the ground.

[kevingranade]

A revert is definitely the best short term fix, this is definitely a worse problem than the menu not being totally consistent. Long term I don't know either, the underlying problem is that the menu mixes actions that can happen immediately with actions that require time to pass, and it doesn't have a consistent way of unifying them.

[RadHazard]

Yeah, makes sense. If you just want to revert for the short term, the branch should be ready to merge.

[Kuro-Maii]

is there a way of removing these "none" type items?
if not in game what would be the best way to find them in the save files?
or is there an alternative?
I ask because when a "none" item has been spawend trying to move it to a different tile causes the game to complain about the destination being full ( even if the tile is empty ) and then drops a copy in that tile anyway.

[Wishbringer]

@Kuro-Maii
You can put "none" items in a fire to get rid of them, that is how I do it. 😆

[budg3]

I got several crashes when the starting NPC burned to death in the burning building scenario and a crash when my player character burned to death, that I suspect are both related to these item dropping bugs. I can't get the source to compile properly on my system (tried both MinGW/MSYS2) so I can't test if this fixes that though.

[Wishbringer]

@budg3
Yeah, if you're trying to use the Makefile from the project I had to modify it to run on MinGW.
Remark out "CHECKS = astyle-check style-json", it stops the Makefile from completing (json_formatter.cgi fails).
Include these switches:
mingw32-make -j4 NATIVE=win32 RELEASE=1 DYNAMIC_LINKING=1 TILES=1 LOCALIZE=0 BACKTRACE=0

DYNAMIC_LINKING=1 reduces the number of dependencies dramatically (you'll only need SDL2 packages).
Use the posix dwarf i686 MinGW-w64 distro for best performance and compatibility (x64 probably wont work).

I'd suggest using GIT Bash (MSYS2) when compiling (CMD will cause more problems).

Hope this helps.

[budg3]

@Wishbringer
Thanks a lot. I was able to compile it with your tips and by getting pkg-config and sdl2-config. Will test to see if it gets rid of the crashing when burning to death.