CleverRaven/Cataclysm-DDA — pull request #8301 by BevapDin (2014-07-21 16:54:33+00:00)
Title: Remove the last remaining functions that use the invlet instead of the item position.

Some of them are already unused, the only remaining functions are `invlet_to_position`/`allocated_invlets` in player and inventory, and the restack-elated functions in inventory. I added a debug statement (directly to the log file), when `invlet_to_position` is called on a npc, npcs should not use the invlets at all.

Invlets are not assigned before the item is added to the inventory. This was done on very few places anyway. Items without invlet, or with an already used invlet, are automatically assigned a new invlet by the inventory class. Only exception is the weapon slot in pickup.cpp because it forces a valid invlet, instead of keeping the item without invlet.

I made `iventory::get_invlet_for_item` private, and removed its usage in pickup.cpp. It is already used in inventory when a new item is added to assign a new invlet.

----

All npc functions now use the item position, or (in case of trading which removes several items at once) they use the item pointer instead of the invlet.

----

Some questions about further improvements:

- `inventory::push_back` is currently a synonym for `inventory::add_item` or `inventory::add_stack` (depending on the argument type). Having two functions with no meaningful difference seems pointless and can be confusing. Should all code use `push_back` or should all code use `add_item`/`add_stack`? Or is there a reason for keeping all three functions?

- Some code uses `player.inv.add_item`, some uses `player.i_add` and there is also `player::i_add_or_drop`, shouldn't this be unified to a single function?
`i_add_or_drop` seems to be the best, the others would overburden the player, `player.inv.add_item` bypasses the artifact message and it bypasses setting `player::last_item`.