Fix trade picker missing enchanted/brewed items and Ready cancelling trades

Code/client/Games/Skyrim/TESObjectREFR.cpp

@@ -901,10 +901,9 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet
         return;
     }
 
-    ExtraDataList* pExtraDataList = GetExtraDataFromItem(arEntry);
-
     if (arEntry.Count > 0)
     {
+        ExtraDataList* pExtraDataList = GetExtraDataFromItem(arEntry);
         bool isWorn = false;
         bool isWornLeft = false;
         if (pExtraDataList)
@@ -925,8 +924,18 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet
     else if (arEntry.Count < 0)
     {
         spdlog::debug("Removing item {:X}, count {}", pObject->formID, -arEntry.Count);
+        // Removal must name the inventory's own extra data list. A freshly built list matches none of the
+        // stored ones, so an enchanted, renamed or poisoned item would stay while a plain copy was taken.
+        auto removalList = [this, &arEntry](TESBoundObject* apItem) -> ExtraDataList* {
+            if (!arEntry.ContainsExtraData())
+                return nullptr;
+            if (ExtraDataList* pExisting = FindExtraDataForItem(apItem, arEntry))
+                return pExisting;
+            spdlog::warn("{}: No stored extra data matches item {:X}; removing with rebuilt data", __FUNCTION__, apItem->formID);
+            return GetExtraDataFromItem(arEntry);
+        };
         if (arEntry.Potion.Effects.empty())
-            RemoveItem(pObject, -arEntry.Count, ITEM_REMOVE_REASON::kRemove, pExtraDataList, nullptr);
+            RemoveItem(pObject, -arEntry.Count, ITEM_REMOVE_REASON::kRemove, removalList(pObject), nullptr);
         else
         {
             // Equivalent recipes can have different local IDs. Remove across their actual stacks.
@@ -940,7 +949,7 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet
                 const auto count = static_cast<int32_t>(std::min(remaining, before));
                 if (count <= 0)
                     break;
-                RemoveItem(potion, count, ITEM_REMOVE_REASON::kRemove, pExtraDataList, nullptr);
+                RemoveItem(potion, count, ITEM_REMOVE_REASON::kRemove, removalList(potion), nullptr);
                 const auto removed = before - GetItemCountInInventory(potion);
                 if (removed <= 0)
                     break;
@@ -969,6 +978,34 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet
     UpdateItemList(nullptr);
 }
 
+ExtraDataList* TESObjectREFR::FindExtraDataForItem(TESBoundObject* apObject, const Inventory::Entry& arEntry) const noexcept
+{
+    auto* pChanges = GetContainerChanges();
+    if (!apObject || !pChanges || !pChanges->entries)
+        return nullptr;
+
+    for (auto* pEntry : *pChanges->entries)
+    {
+        if (!pEntry || pEntry->form != apObject || !pEntry->dataList)
+            continue;
+        for (ExtraDataList* pExtraDataList : *pEntry->dataList)
+        {
+            if (!pExtraDataList)
+                continue;
+            Inventory::Entry local{};
+            local.BaseId = arEntry.BaseId;
+            local.Potion = arEntry.Potion;
+            {
+                ScopedExtraDataOverride _;
+                GetItemFromExtraData(local, pExtraDataList);
+            }
+            if (local.CanBeMerged(arEntry))
+                return pExtraDataList;
+        }
+    }
+    return nullptr;
+}
+
 void TESObjectREFR::UpdateItemList(TESForm* pUnkForm) noexcept
 {
     TP_THIS_FUNCTION(TUpdateItemList, void, TESObjectREFR, TESForm*);

Code/client/Games/Skyrim/TESObjectREFR.h

@@ -63,6 +63,8 @@ struct TESObjectREFR : TESForm
 
     static void GetItemFromExtraData(Inventory::Entry& arEntry, ExtraDataList* apExtraDataList) noexcept;
     static ExtraDataList* GetExtraDataFromItem(const Inventory::Entry& arEntry) noexcept;
+    // The inventory's own extra data list describing this entry, so removal takes that exact item.
+    ExtraDataList* FindExtraDataForItem(TESBoundObject* apObject, const Inventory::Entry& arEntry) const noexcept;
 
     virtual void sub_3B();
     virtual void sub_3C();

Code/client/Services/Generic/TradeService.cpp

@@ -11,6 +11,7 @@
 #include <Messages/TradeOfferUpdateRequest.h>
 #include <Messages/TradeSetReadyRequest.h>
 #include <Messages/TradeCancelRequest.h>
+#include <Messages/TradeInventorySyncRequest.h>
 #include <Messages/NotifyTradeInvite.h>
 #include <Messages/NotifyTradeStarted.h>
 #include <Messages/NotifyTradeState.h>
@@ -233,12 +234,8 @@ void TradeService::SetReady(bool aReady) noexcept
     request.Ready = aReady;
     if (aReady)
     {
-        auto* player = PlayerCharacter::Get();
-        if (!player || !ValidateTradeOffer(player->GetInventory(), m_session.SelfItems))
-        {
-            CancelTrade();
-            return;
-        }
+        // The server validates the offer against this snapshot; an outdated offer unreadies instead of cancelling.
+        SyncInventory();
         // Accept only partner items this game can rebuild; the rest stay with the partner.
         // Keep prepared potion forms alive through cancellation races until the session ends.
         auto& modSystem = m_world.GetModSystem();
@@ -265,6 +262,16 @@ void TradeService::SetReady(bool aReady) noexcept
     m_transport.Send(request);
 }
 
+void TradeService::SyncInventory() const noexcept
+{
+    auto* player = PlayerCharacter::Get();
+    if (!player)
+        return;
+    TradeInventorySyncRequest request{};
+    request.Entries = player->GetInventory().Entries;
+    m_transport.Send(request);
+}
+
 void TradeService::UpdateOffer(const TiltedPhoques::Vector<OfferSelection>& aSelections) noexcept
 {
     ApplyOfferSelection(aSelections);
@@ -371,6 +378,9 @@ void TradeService::OnTradeStarted(const NotifyTradeStarted& acMessage) noexcept
     m_pendingInvites.erase(acMessage.PartnerPlayerId);
     EmitInviteUpdate(acMessage.PartnerPlayerId, false);
 
+    // Offer from the live inventory: enchanted, renamed and brewed items may be missing from the server copy.
+    SyncInventory();
+
     EmitStateToUI();
 }
 

Code/client/Services/TradeService.h

@@ -50,6 +50,7 @@ struct TradeService
 
     void ClearSession() noexcept;
     void ReleasePreparedPotions() noexcept;
+    void SyncInventory() const noexcept;
     void EmitStateToUI() const noexcept;
     void EmitInviteUpdate(uint32_t aInviterId, bool aAdded, uint64_t aExpiryTick = 0) const noexcept;
     void EmitCancellation(uint32_t aPartnerId, TradeCancelReason aReason, bool aWasInitiator) const noexcept;

Code/encoding/Messages/ClientMessageFactory.h

@@ -20,6 +20,7 @@
 #include <Messages/TradeInviteResponseRequest.h>
 #include <Messages/TradeOfferUpdateRequest.h>
 #include <Messages/TradeSetReadyRequest.h>
+#include <Messages/TradeInventorySyncRequest.h>
 #include <Messages/TradeCancelRequest.h>
 #include <Messages/PartyInviteRequest.h>
 #include <Messages/RequestActorValueChanges.h>
@@ -71,7 +72,7 @@ struct ClientMessageFactory
     {
         auto s_visitor = CreateMessageVisitor<
             AuthenticationRequest, AssignCharacterRequest, ClientReferencesMoveRequest, EnterInteriorCellRequest, RequestInventoryChanges, RequestFactionsChanges, RequestQuestUpdate, PartyInviteRequest, PartyAcceptInviteRequest, PartyLeaveRequest, PartyCreateRequest,
-            PartyChangeLeaderRequest, PartyKickRequest, TradeInviteRequest, TradeInviteResponseRequest, TradeOfferUpdateRequest, TradeSetReadyRequest, TradeCancelRequest, RequestActorValueChanges, RequestActorMaxValueChanges, EnterExteriorCellRequest, RequestHealthChangeBroadcast, ActivateRequest, LockChangeRequest, AssignObjectsRequest, RequestDeathStateChange, ShiftGridCellRequest,
+            PartyChangeLeaderRequest, PartyKickRequest, TradeInviteRequest, TradeInviteResponseRequest, TradeOfferUpdateRequest, TradeSetReadyRequest, TradeCancelRequest, TradeInventorySyncRequest, RequestActorValueChanges, RequestActorMaxValueChanges, EnterExteriorCellRequest, RequestHealthChangeBroadcast, ActivateRequest, LockChangeRequest, AssignObjectsRequest, RequestDeathStateChange, ShiftGridCellRequest,
             RequestOwnershipTransfer, RequestOwnershipClaim, RequestObjectInventoryChanges, SpellCastRequest, ProjectileLaunchRequest, InterruptCastRequest, AddTargetRequest, ScriptAnimationRequest, DrawWeaponRequest, MountRequest, NewPackageRequest, RequestRespawn, SyncExperienceRequest,
             RequestEquipmentChanges, SendChatMessageRequest, TeleportCommandRequest, PlayerRespawnRequest, DialogueRequest, SubtitleRequest, PlayerDialogueRequest, PlayerLevelRequest, TeleportRequest, RequestPlayerHealthUpdate, RequestWeatherChange, RequestCurrentWeather, RequestSetWaypoint,
             RequestRemoveWaypoint, RemoveSpellRequest, SetTimeCommandRequest>;

Code/encoding/Messages/TradeInventorySyncRequest.cpp

@@ -0,0 +1,22 @@
+#include <Messages/TradeInventorySyncRequest.h>
+
+void TradeInventorySyncRequest::SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept
+{
+    Serialization::WriteVarInt(aWriter, Entries.size());
+    for (const auto& entry : Entries)
+        entry.Serialize(aWriter);
+}
+
+void TradeInventorySyncRequest::DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept
+{
+    ClientMessage::DeserializeRaw(aReader);
+
+    Entries.clear();
+    const auto count = Serialization::ReadVarInt(aReader);
+    for (uint64_t i = 0; i < count; ++i)
+    {
+        Inventory::Entry entry;
+        entry.Deserialize(aReader);
+        Entries.push_back(std::move(entry));
+    }
+}

Code/encoding/Messages/TradeInventorySyncRequest.h

@@ -0,0 +1,21 @@
+#pragma once
+
+#include "Message.h"
+#include <Structs/Inventory.h>
+
+// The trading player's live inventory. Incremental inventory events miss changes made in place,
+// such as enchanting, renaming or brewing, so trades work from this snapshot.
+struct TradeInventorySyncRequest final : ClientMessage
+{
+    static constexpr ClientOpcode Opcode = kTradeInventorySyncRequest;
+
+    TradeInventorySyncRequest()
+        : ClientMessage(Opcode)
+    {
+    }
+
+    void SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept override;
+    void DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept override;
+
+    Vector<Inventory::Entry> Entries;
+};

Code/encoding/Opcodes.h

@@ -58,6 +58,7 @@ enum ClientOpcode : unsigned char
     kTradeOfferUpdateRequest,
     kTradeSetReadyRequest,
     kTradeCancelRequest,
+    kTradeInventorySyncRequest,
     kClientOpcodeMax
 };
 

Code/encoding/Structs/TradeValidation.h

@@ -86,6 +86,37 @@ inline bool ValidateTradeOffer(const Inventory& acInventory, const Vector<Invent
     return true;
 }
 
+// Keeps the offered items an inventory can still cover, reserving quantities in offer order.
+inline Vector<Inventory::Entry> KeepCoveredTradeOffer(const Inventory& acInventory, const Vector<Inventory::Entry>& acItems) noexcept
+{
+    Vector<int32_t> remaining;
+    remaining.reserve(acInventory.Entries.size());
+    for (const auto& entry : acInventory.Entries)
+        remaining.push_back(std::max(0, entry.Count));
+
+    Vector<Inventory::Entry> kept;
+    for (const auto& item : acItems)
+    {
+        if (item.Count <= 0 || IsExcludedTradeItem(item) || item.IsWorn())
+            continue;
+        auto reserved = remaining;
+        int32_t needed = item.Count;
+        for (size_t i = 0; i < acInventory.Entries.size() && needed > 0; ++i)
+        {
+            if (!SameTradeItem(acInventory.Entries[i], item))
+                continue;
+            const int32_t count = std::min(needed, reserved[i]);
+            reserved[i] -= count;
+            needed -= count;
+        }
+        if (needed != 0)
+            continue;
+        remaining = std::move(reserved);
+        kept.push_back(item);
+    }
+    return kept;
+}
+
 // Publish neither inventory until both offers and resulting stack counts are valid.
 inline bool PrepareTradeExchange(const Inventory& acLeft, const Inventory& acRight,
     const Vector<Inventory::Entry>& acLeftOffer, const Vector<Inventory::Entry>& acRightOffer,

Code/server/Services/TradeService.cpp

@@ -18,6 +18,7 @@
 #include <Messages/TradeOfferUpdateRequest.h>
 #include <Messages/TradeSetReadyRequest.h>
 #include <Messages/TradeCancelRequest.h>
+#include <Messages/TradeInventorySyncRequest.h>
 
 #include <Game/Player.h>
 #include <Game/PlayerManager.h>
@@ -43,6 +44,7 @@ TradeService::TradeService(World& aWorld, entt::dispatcher& aDispatcher) noexcep
     m_tradeOfferUpdateConnection = aDispatcher.sink<PacketEvent<TradeOfferUpdateRequest>>().connect<&TradeService::OnTradeOfferUpdate>(this);
     m_tradeSetReadyConnection = aDispatcher.sink<PacketEvent<TradeSetReadyRequest>>().connect<&TradeService::OnTradeSetReady>(this);
     m_tradeCancelConnection = aDispatcher.sink<PacketEvent<TradeCancelRequest>>().connect<&TradeService::OnTradeCancel>(this);
+    m_tradeInventorySyncConnection = aDispatcher.sink<PacketEvent<TradeInventorySyncRequest>>().connect<&TradeService::OnTradeInventorySync>(this);
 }
 
 void TradeService::OnUpdate(const UpdateEvent&) noexcept
@@ -302,14 +304,22 @@ void TradeService::OnTradeSetReady(const PacketEvent<TradeSetReadyRequest>& acPa
 
     if (!ValidateOffer(pPlayer, pSession->Offers[cIndex].Items))
     {
+        // The offer no longer matches the player's inventory; keep the trade open so they can adjust it.
         spdlog::warn("[TradeService]: Player {} attempted to ready invalid offer", pPlayer->GetId());
-        CancelSession(*pSession, TradeCancelReason::FailedValidation, pPlayer);
+        pSession->Offers[cIndex].Ready = false;
+        pSession->Offers[cIndex].AcceptedItems.clear();
+        ResetCountdown(*pSession);
+        SendStateUpdate(*pSession);
         return;
     }
 
     if (!ValidateAcceptedItems(pSession->Offers[1 - cIndex].Items, message.AcceptedItems))
     {
-        CancelSession(*pSession, TradeCancelReason::FailedValidation, pPlayer);
+        // The partner's offer changed after this player last saw it; they can ready again on the new state.
+        pSession->Offers[cIndex].Ready = false;
+        pSession->Offers[cIndex].AcceptedItems.clear();
+        ResetCountdown(*pSession);
+        SendStateUpdate(*pSession);
         return;
     }
     // Keep the server's canonical copies of the accepted partner items, not the client's echo.
@@ -346,6 +356,39 @@ void TradeService::OnTradeCancel(const PacketEvent<TradeCancelRequest>& acPacket
     RemoveInviteFor(pPlayer, TradeCancelReason::Cancelled, pPlayer);
 }
 
+void TradeService::OnTradeInventorySync(const PacketEvent<TradeInventorySyncRequest>& acPacket) noexcept
+{
+    Player* pPlayer = acPacket.pPlayer;
+    TradeSession* pSession = GetSession(pPlayer);
+    if (!pSession)
+        return;
+
+    const int32_t cIndex = GetSessionIndex(*pSession, pPlayer);
+    auto* pInventory = GetInventoryFor(pPlayer);
+    if (cIndex < 0 || !pInventory)
+        return;
+
+    // Incremental inventory events miss in-place changes such as enchanting, renaming or brewing,
+    // so the trading player's live inventory replaces the server copy for the trade.
+    pInventory->Entries = acPacket.Packet.Entries;
+
+    auto& offer = pSession->Offers[cIndex];
+    auto covered = KeepCoveredTradeOffer(*pInventory, offer.Items);
+    if (covered.size() != offer.Items.size())
+    {
+        spdlog::debug("[TradeService]: Dropped {} offered items no longer in player {}'s inventory", offer.Items.size() - covered.size(), pPlayer->GetId());
+        offer.Items = std::move(covered);
+        for (auto& each : pSession->Offers)
+        {
+            each.Ready = false;
+            each.AcceptedItems.clear();
+        }
+        ResetCountdown(*pSession);
+    }
+
+    SendStateUpdate(*pSession);
+}
+
 TradeService::TradeSession* TradeService::GetSession(Player* apPlayer) noexcept
 {
     if (!apPlayer)

Code/server/Services/TradeService.h

@@ -13,6 +13,7 @@ struct TradeInviteResponseRequest;
 struct TradeOfferUpdateRequest;
 struct TradeSetReadyRequest;
 struct TradeCancelRequest;
+struct TradeInventorySyncRequest;
 
 struct Player;
 
@@ -35,6 +36,7 @@ class TradeService
     void OnTradeOfferUpdate(const PacketEvent<TradeOfferUpdateRequest>& acPacket) noexcept;
     void OnTradeSetReady(const PacketEvent<TradeSetReadyRequest>& acPacket) noexcept;
     void OnTradeCancel(const PacketEvent<TradeCancelRequest>& acPacket) noexcept;
+    void OnTradeInventorySync(const PacketEvent<TradeInventorySyncRequest>& acPacket) noexcept;
 
 private:
     struct TradeOffer
@@ -102,4 +104,5 @@ class TradeService
     entt::scoped_connection m_tradeOfferUpdateConnection;
     entt::scoped_connection m_tradeSetReadyConnection;
     entt::scoped_connection m_tradeCancelConnection;
+    entt::scoped_connection m_tradeInventorySyncConnection;
 };