ZDoom

Is it about this commit? Anyway, this isn't fixed in the current master, 95995e4.

Cooke is right, the problem is in rewritten loops.

To fix this particular issue it's enough to replace Actor invp = self; with Actor invp = item.Owner; here.
Item's owner in needed because we are trying to remove it from normal player pawn, not morphed one, and this breaks inventory linked list.
Without this, for loop does nothing, and the inventory list is cut after the removed item.

So in other words, the function gets called from the wrong actor. This means the call needs to get fixed, not the function. All the function needs is to do nothing if the item doesn't belong to the calling actor.

Seriously though, the unmorphing process needs some serious rethinking. It cannot be that it fucks up the self pointer of the calling functions.
Actual unmorphing needs to be handled OUTSIDE PlayerThink or these problems will always come back haunting us - even if it breaks a small number of hacky mods that try to abuse the system.

OK but we should fix this bug for upcoming release I think without significant and risky changes.

Here is the problem. toucher is equal to players[consoleplayer].mo for AddInventory() call.
This is not the case for RemoveInventory() because Use() does morphing changing player pawn actor.
For this reason item's owner was important for that RemoveInventory() call.
The value of toucher remains the same while player pawn is a different actor.

Graf Zahl wrote:Seriously though, the unmorphing process needs some serious rethinking.

Indeed. Although I'm not au fait with the code itself, the mere fact that it breaks so often (I can't count the number of times an unmorph bug has appeared after an update) indicates that there is something fundamentally unstable about the setup.

_mental_ wrote:OK but we should fix this bug for upcoming release I think without significant and risky changes.

But that also makes a lot of sense.

For a point release, the fix should obviously be kept as simple as possible.

But the fix should still not be in RemoveInventory but in the calling code. This actually needs to set toucher to the new owner before calling RemoveInventory so that the code calling CallTryPickup doesn't continue to operate on the invalid player. The weird setup of this function was only done to accomodate morphing, but missed this path because it wasn't an issue when the code was written.

To fix the provided sample it's enough to add toucher = self.owner; after Inventory.Use() call.
I guess this should be done conditionally, i.e. it must depend on value of usegood at least. Probably the owner can be null too.

Yes, that's what I would have done, too. And surely be cautious about the owner being null.

Fixed in fc5420b.

Thank you for your time and attention. Much appreciated.