diff --git a/dGame/dComponents/InventoryComponent.cpp b/dGame/dComponents/InventoryComponent.cpp index c40394c04..4ac5a7bde 100644 --- a/dGame/dComponents/InventoryComponent.cpp +++ b/dGame/dComponents/InventoryComponent.cpp @@ -964,6 +964,11 @@ void InventoryComponent::Update(float deltaTime) { for (auto* set : m_Itemsets) { set->Update(deltaTime); } + + // Items removed during the last frame are no longer in use + for (auto* const inventory : m_Inventories | std::views::values) { + inventory->FreeRetiredItems(); + } } void InventoryComponent::UpdateSlot(const std::string& location, const EquippedItem& item, bool keepCurrent) { diff --git a/dGame/dInventory/Inventory.cpp b/dGame/dInventory/Inventory.cpp index 363458e53..00851e3c2 100644 --- a/dGame/dInventory/Inventory.cpp +++ b/dGame/dInventory/Inventory.cpp @@ -236,6 +236,14 @@ void Inventory::RemoveManagedItem(Item* item) { free++; } +void Inventory::RetireItem(Item* item) { + m_RetiredItems.emplace_back(item); +} + +void Inventory::FreeRetiredItems() { + m_RetiredItems.clear(); +} + eInventoryType Inventory::FindInventoryTypeForLot(const LOT lot) { auto itemComponent = FindItemComponent(lot); diff --git a/dGame/dInventory/Inventory.h b/dGame/dInventory/Inventory.h index f07ec68e4..295e417cd 100644 --- a/dGame/dInventory/Inventory.h +++ b/dGame/dInventory/Inventory.h @@ -4,6 +4,7 @@ #define INVENTORY_H #include +#include #include #include "CDItemComponentTable.h" @@ -126,6 +127,23 @@ public: */ void RemoveManagedItem(Item* item); + /** + * Takes ownership of an item that was taken out of the inventory. Code still holding the item can use it until + * FreeRetiredItems runs (the inventory component's next update), so an item never frees itself. + * @param item the removed item + */ + void RetireItem(Item* item); + + /** + * Frees the items retired since the last call + */ + void FreeRetiredItems(); + + /** + * @return whether removed items are waiting to be freed + */ + bool HasRetiredItems() const { return !m_RetiredItems.empty(); } + /** * Returns the inventory type an item of the specified lot should be placed in * @param lot the lot to find the inventory type for @@ -183,6 +201,11 @@ private: */ std::map items; + /** + * Items taken out of this inventory, freed by FreeRetiredItems + */ + std::vector> m_RetiredItems; + /** * The inventory component this inventory belongs to */ diff --git a/dGame/dInventory/Item.cpp b/dGame/dInventory/Item.cpp index ec0a67de4..caff8ee22 100644 --- a/dGame/dInventory/Item.cpp +++ b/dGame/dInventory/Item.cpp @@ -580,7 +580,9 @@ void Item::RemoveFromInventory() { inventory->RemoveManagedItem(this); - delete this; + // The inventory frees the item at its next update, so callers still holding it (SetCount(0) and the loops that + // remove several items) are not left with a freed item + inventory->RetireItem(this); } Item::~Item() { diff --git a/tests/dGameTests/dComponentsTests/CMakeLists.txt b/tests/dGameTests/dComponentsTests/CMakeLists.txt index 97ccf063b..6950ce482 100644 --- a/tests/dGameTests/dComponentsTests/CMakeLists.txt +++ b/tests/dGameTests/dComponentsTests/CMakeLists.txt @@ -16,6 +16,7 @@ set(DCOMPONENTS_TESTS "QuickBuildCompleteTests.cpp" "RocketLaunchTests.cpp" "ReplicaConstructionTests.cpp" + "ItemRemovalTests.cpp" ) # Get the folder name and prepend it to the files above diff --git a/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp b/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp new file mode 100644 index 000000000..a946f7471 --- /dev/null +++ b/tests/dGameTests/dComponentsTests/ItemRemovalTests.cpp @@ -0,0 +1,87 @@ +#include "GameDependencies.h" + +#include "CDClientManager.h" +#include "CDComponentsRegistryTable.h" +#include "CDItemComponentTable.h" +#include "Entity.h" +#include "InventoryComponent.h" +#include "Item.h" +#include "eReplicaComponentType.h" + +#include + +// An item taken out of its inventory is not freed on the spot: the inventory keeps it until its component's next +// update, so code still holding the item can finish with it. +class ItemRemovalTests : public GameDependenciesTest { +protected: + std::unique_ptr entity; + InventoryComponent* inventoryComponent{}; + Inventory* items{}; + LWOOBJID nextId = 0x7000; + + void SetUp() override { + SetUpDependencies(); + RegisterLot(1000); + CDClientManager::GetEntriesMutable().insert_or_assign(static_cast(info.lot), 0); + entity = std::make_unique(1, info); + inventoryComponent = entity->AddComponent(-1); + items = inventoryComponent->GetInventory(eInventoryType::ITEMS); + } + + void TearDown() override { + entity.reset(); + TearDownDependencies(); + } + + static void RegisterLot(const LOT lot) { + const auto componentID = static_cast(96000 + lot); + auto& registry = CDClientManager::GetEntriesMutable(); + registry.insert_or_assign(static_cast(lot), componentID); + registry.insert_or_assign(static_cast(eReplicaComponentType::ITEM) << 32 | static_cast(lot), componentID); + CDItemComponent component{}; + component.id = componentID; + CDClientManager::GetEntriesMutable().insert_or_assign(componentID, component); + } + + Item* Give(const uint32_t count) { + return new Item(nextId++, 1000, items, static_cast(items->GetItems().size()), count, false, {}, LWOOBJID_EMPTY, LWOOBJID_EMPTY, eLootSourceType::NONE); + } +}; + +TEST_F(ItemRemovalTests, ARemovedItemIsUsableUntilTheNextUpdate) { + auto* const item = Give(1); + const auto id = item->GetId(); + + item->RemoveFromInventory(); + + EXPECT_EQ(items->GetItems().count(id), 0u); + EXPECT_TRUE(items->HasRetiredItems()); + // Still a valid object + EXPECT_EQ(item->GetId(), id); + EXPECT_EQ(item->GetCount(), 0u); + + inventoryComponent->Update(0.0f); + EXPECT_FALSE(items->HasRetiredItems()); +} + +TEST_F(ItemRemovalTests, RemovingSeveralItemsInALoop) { + Give(1); + Give(2); + Give(3); + + // Every item removed while the list is walked; each stays valid until the update + auto held = items->GetItems(); + for (auto* const item : held | std::views::values) item->RemoveFromInventory(); + for (auto* const item : held | std::views::values) EXPECT_EQ(item->GetCount(), 0u); + + EXPECT_TRUE(items->GetItems().empty()); + inventoryComponent->Update(0.0f); + EXPECT_FALSE(items->HasRetiredItems()); +} + +TEST_F(ItemRemovalTests, RetiredItemsAreFreedWithTheInventory) { + Give(1)->RemoveFromInventory(); + EXPECT_TRUE(items->HasRetiredItems()); + // Freed by the inventory's destructor (checked by sanitizer builds) + entity.reset(); +}