Dawn-of-Light / DOLSharp

Dawn of Light (DOL) - Dark Age of Camelot (DAOC) Server Emulator
http://www.dolserver.net
GNU General Public License v2.0
114 stars 102 forks source link

Create a GameInventoryItem when interacting with a GameInventoryObject #459

Closed Lotendan closed 1 year ago

Lotendan commented 1 year ago

Every item added to the player's inventory needs to be a GameInventoryItem type. There exists a condition in GameInventoryObjects which allow to exchange two items from a vault, during which the item received loses its type and uses InventoryItem instead. A player could exploit this behaviour to prevent items from losing condition (for example). Indeed once turned into bare InventoryItems this code is no longer called. Note there are probably other places in the code where we are pushing an InventoryItem instead of a GameInventoryItem and we forget to GameInventoryItem.Create.

This begs some questions:

  1. Why do the interface IGameInventory and all derivatives not accept an IGameInventoryItem instead of a generic InventoryItem object. This would effectively add a compile-time check that all items added to a player's inventory (and even a living's inventory - that would not hurt actually) are of the right type. I had a quick look in the code and actually this change would be large but wouldn't be too difficult because most of the code is articulated around GameInventoryItem anyway already.

  2. What was the initial reason we splitted InventoryItem into GameInventoryItem historically? I imagine the first is the database record, and the second is the logical record and it makes sense to have a separation of their concerns.

  3. Why does the GameInventoryItem inherit from InventoryItem? Most of the time this forces a copy of the InventoryItem object into the GameInventoryItem, while it would probably (?) be simpler to keep a private reference to an InventoryItem inside the GameInventoryItem and not inherit from it (composition over inheritance).

NetDwarf commented 1 year ago

Nice find!

Inventory is indeed quite messy. For 1. I am not even sure this interface is really needed actually, but given that it exists I am sure it has different implementations by now. For 2. you are probably right and regarding 3. using inheritance for this is indeed a code smell. I would however not care about object creation or memory usage just yet (but it is definitely a potential memory hog).