From 6f7fd5f84e77ca10aa95f494844b722c61facce0 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 15:37:33 -0600 Subject: [PATCH] fix: a trade offer takes what the trade window takes The client's trade window has 8 slots for each player and takes neither a bound item nor an equipped one. An offer with more than 8 items, a bound item or an equipped item is now refused like the other offers the window can not make, when it arrives and again when the trade completes. Co-Authored-By: Claude Fable 5.2 --- dGame/TradingManager.cpp | 22 +++++++++------- dGame/TradingManager.h | 16 +++++++++++- dGame/dGameMessages/TradeMessages.cpp | 6 +---- .../dGameMessagesTests/TradeOfferTests.cpp | 26 ++++++++++++++++++- 4 files changed, 53 insertions(+), 17 deletions(-) diff --git a/dGame/TradingManager.cpp b/dGame/TradingManager.cpp index dcc822562..a8423d9a8 100644 --- a/dGame/TradingManager.cpp +++ b/dGame/TradingManager.cpp @@ -36,12 +36,14 @@ std::optional> Trade::ResolveOffer( resolved.reserve(offered.size()); std::set seen; + if (offered.size() > MaxOfferedItems) return std::nullopt; + for (const auto& tradeItem : offered) { // The client keeps its offer by item ID, so an item is only ever in it once if (tradeItem.itemCount == 0 || !seen.insert(tradeItem.itemId).second) return std::nullopt; const auto held = findHeld(tradeItem.itemId); - if (!held || held->count == 0) return std::nullopt; + if (!held || held->count == 0 || held->bound || held->equipped) return std::nullopt; resolved.push_back({ tradeItem.itemId, held->lot, std::min(tradeItem.itemCount, held->count) }); } @@ -49,6 +51,14 @@ std::optional> Trade::ResolveOffer( return resolved; } +std::function(LWOOBJID)> Trade::HeldIn(const InventoryComponent& inventory) { + return [&inventory](const LWOOBJID itemId) -> std::optional { + const auto* const item = inventory.FindItemById(itemId); + if (!item) return std::nullopt; + return HeldTradeItem{ item->GetLot(), item->GetCount(), item->GetBound(), item->IsEquipped() }; + }; +} + Trade::~Trade() { } @@ -172,15 +182,7 @@ void Trade::Complete() { } // The inventories may have changed since the offers were made, so check the offers against them again - const auto holds = [](const InventoryComponent* const inventory) { - return [inventory](const LWOOBJID itemId) -> std::optional { - const auto* const item = inventory->FindItemById(itemId); - if (!item) return std::nullopt; - return HeldTradeItem{ item->GetLot(), item->GetCount() }; - }; - }; - - if (ResolveOffer(m_ItemsA, holds(inventoryA)) != m_ItemsA || ResolveOffer(m_ItemsB, holds(inventoryB)) != m_ItemsB) { + if (ResolveOffer(m_ItemsA, HeldIn(*inventoryA)) != m_ItemsA || ResolveOffer(m_ItemsB, HeldIn(*inventoryB)) != m_ItemsB) { LOG("The items offered by %s and %s no longer match what they hold, aborting trade", characterA->GetName().c_str(), characterB->GetName().c_str()); return; } diff --git a/dGame/TradingManager.h b/dGame/TradingManager.h index 5924acd14..fa427e3ea 100644 --- a/dGame/TradingManager.h +++ b/dGame/TradingManager.h @@ -20,8 +20,13 @@ struct TradeItem { struct HeldTradeItem { LOT lot; uint32_t count; + // The trade window takes neither a bound item nor an equipped one + bool bound = false; + bool equipped = false; }; +class InventoryComponent; + class Trade { public: /** @@ -29,12 +34,21 @@ public: * @param offered the items as the player sent them * @param findHeld gives the held item with an ID, or nothing if the player has no such item * @return the offer with every LOT taken from the held item and every count limited to the held count, - * or nothing if the offer names an item the player does not hold, the same item twice or a count of 0 + * or nothing if the offer names an item the player does not hold, the same item twice, a count of 0, + * a bound or equipped item, or more items than the trade window has slots */ static std::optional> ResolveOffer( const std::vector& offered, const std::function(LWOOBJID)>& findHeld); + /** + * Gives the lookup ResolveOffer needs for the items in an inventory + */ + static std::function(LWOOBJID)> HeldIn(const InventoryComponent& inventory); + + // The slots one side of the trade window has + static constexpr uint32_t MaxOfferedItems = 8; + explicit Trade(LWOOBJID tradeId, LWOOBJID participantA, LWOOBJID participantB); ~Trade(); diff --git a/dGame/dGameMessages/TradeMessages.cpp b/dGame/dGameMessages/TradeMessages.cpp index 2d21720fd..046c4f2c9 100644 --- a/dGame/dGameMessages/TradeMessages.cpp +++ b/dGame/dGameMessages/TradeMessages.cpp @@ -247,11 +247,7 @@ namespace GameMessages { if (!inventoryComponent) return; // The LOT and the count the trade works with come from the items the player holds - const auto resolvedItems = Trade::ResolveOffer(items, [inventoryComponent](const LWOOBJID itemId) -> std::optional { - const auto* const item = inventoryComponent->FindItemById(itemId); - if (!item) return std::nullopt; - return HeldTradeItem{ item->GetLot(), item->GetCount() }; - }); + const auto resolvedItems = Trade::ResolveOffer(items, Trade::HeldIn(*inventoryComponent)); if (!resolvedItems) { LOG("Trade update from (%llu) does not match the items they hold, ignoring it", entity.GetObjectID()); diff --git a/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp index 696b93c8d..d6b53be3f 100644 --- a/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp +++ b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp @@ -8,6 +8,8 @@ namespace { const std::map g_Held = { { 100, { 1727, 5 } }, { 200, { 3040, 1 } }, + { 300, { 1727, 2, true, false } }, + { 400, { 1727, 2, false, true } }, }; std::optional FindHeld(const LWOOBJID itemId) { @@ -50,9 +52,31 @@ TEST(TradeOfferTest, AnItemOfferedTwiceIsRefused) { } TEST(TradeOfferTest, AnItemThePlayerDoesNotHoldIsRefused) { - EXPECT_FALSE(Trade::ResolveOffer({ { 300, 1727, 1 } }, FindHeld).has_value()); + EXPECT_FALSE(Trade::ResolveOffer({ { 900, 1727, 1 } }, FindHeld).has_value()); } TEST(TradeOfferTest, ACountOfZeroIsRefused) { EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 0 } }, FindHeld).has_value()); } + +TEST(TradeOfferTest, ABoundItemIsRefused) { + EXPECT_FALSE(Trade::ResolveOffer({ { 300, 1727, 1 } }, FindHeld).has_value()); + EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 1 }, { 300, 1727, 1 } }, FindHeld).has_value()); +} + +TEST(TradeOfferTest, AnEquippedItemIsRefused) { + EXPECT_FALSE(Trade::ResolveOffer({ { 400, 1727, 1 } }, FindHeld).has_value()); +} + +TEST(TradeOfferTest, AnOfferFillsTheSlotsOfTheTradeWindowAtMost) { + const auto findAny = [](const LWOOBJID) -> std::optional { + return HeldTradeItem{ 1727, 1 }; + }; + + std::vector offered; + for (LWOOBJID itemId = 1; itemId <= Trade::MaxOfferedItems; itemId++) offered.push_back({ itemId, 1727, 1 }); + EXPECT_TRUE(Trade::ResolveOffer(offered, findAny).has_value()); + + offered.push_back({ Trade::MaxOfferedItems + 1, 1727, 1 }); + EXPECT_FALSE(Trade::ResolveOffer(offered, findAny).has_value()); +}