diff --git a/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp b/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp index 4027950b5..ed0607c7e 100644 --- a/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp +++ b/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp @@ -3,16 +3,14 @@ #include #include #include -#include -#include -void AlchemyItem::Capture(TESForm* apForm, Inventory::Entry& aEntry) noexcept +bool AlchemyItem::CaptureRecipe(TESForm* apForm, Inventory::PotionData& aData) noexcept { + aData = {}; auto* potion = apForm && apForm->IsTemporary() ? Cast(apForm) : nullptr; if (!potion || potion->IsFood()) - return; - aEntry.Potion = {}; - aEntry.Potion.IsPoison = potion->IsPoison(); + return false; + aData.IsPoison = potion->IsPoison(); auto& mods = World::Get().GetModSystem(); for (auto* effect : potion->listOfEffects) { @@ -21,24 +19,26 @@ void AlchemyItem::Capture(TESForm* apForm, Inventory::Entry& aEntry) noexcept if (!effect || !effect->pEffectSetting || effect->Condition.pHead || effect->pEffectSetting->IsTemporary() || !mods.GetServerModId(effect->pEffectSetting->formID, result.EffectId)) { - aEntry.Potion.Effects.clear(); - return; + aData.Effects.clear(); + return false; } result.Magnitude = effect->data.fMagnitude; result.Area = effect->data.iArea; result.Duration = effect->data.iDuration; result.RawCost = effect->fRawCost; - aEntry.Potion.Effects.push_back(result); + aData.Effects.push_back(result); } - if (!aEntry.Potion.IsValid()) - { - aEntry.Potion.Effects.clear(); + if (!aData.IsValid()) + aData.Effects.clear(); + return !aData.Effects.empty(); +} + +void AlchemyItem::Capture(TESForm* apForm, Inventory::Entry& aEntry) noexcept +{ + auto* potion = apForm && apForm->IsTemporary() ? Cast(apForm) : nullptr; + if (!potion || potion->IsFood()) return; - } - std::sort(aEntry.Potion.Effects.begin(), aEntry.Potion.Effects.end(), [](const auto& a, const auto& b) { - return std::tie(a.EffectId.ModId, a.EffectId.BaseId, a.Magnitude, a.Area, a.Duration, a.RawCost) < - std::tie(b.EffectId.ModId, b.EffectId.BaseId, b.Magnitude, b.Area, b.Duration, b.RawCost); - }); + CaptureRecipe(apForm, aEntry.Potion); } AlchemyItem* AlchemyItem::Find(TESObjectREFR* apOwner, const Inventory::PotionData& aData) noexcept @@ -82,18 +82,20 @@ AlchemyItem* AlchemyItem::Create(const Inventory::PotionData& aData) noexcept } effects[i] = effect; } - // CommonLibSSE AddPotion: output is a one-pointer CreatedObjPtr, not a raw return value. - // AE address-library ID 36167; no AlchemyMenu, recipe, ingredients or skill calculation. + // CommonLibSSE AddPotion/AddPoison: output is a one-pointer created-object smart pointer. + // AE address-library IDs 36167 (potion) and 36168 (poison), the same calls the alchemy menu ends with. + // No AlchemyMenu, recipe, ingredient or skill calculation is involved. TP_THIS_FUNCTION(TAddPotion, void, BGSCreatedObjectManager, AlchemyItem**, GameArray*); POINTER_SKYRIMSE(TAddPotion, addPotion, 36167); + POINTER_SKYRIMSE(TAddPotion, addPoison, 36168); AlchemyItem* created = nullptr; - TiltedPhoques::ThisCall(addPotion, manager, &created, &effects); + TiltedPhoques::ThisCall(aData.IsPoison ? addPoison : addPotion, manager, &created, &effects); Memory::Free(effects.data); if (created) { - Inventory::Entry check; - Capture(created, check); - if (check.Potion == aData) + Inventory::PotionData check; + CaptureRecipe(created, check); + if (check == aData) return created; spdlog::error("[TradeService]: AddPotion returned effects/type differing from the offered potion"); Release(created); diff --git a/Code/client/Games/Skyrim/Forms/AlchemyItem.h b/Code/client/Games/Skyrim/Forms/AlchemyItem.h index 69c6597cd..4877f302c 100644 --- a/Code/client/Games/Skyrim/Forms/AlchemyItem.h +++ b/Code/client/Games/Skyrim/Forms/AlchemyItem.h @@ -30,6 +30,8 @@ struct AlchemyItem : MagicItem bool IsPoison() const noexcept { return (data.flags & Poison) != 0; } bool IsFood() const noexcept { return (data.flags & Food) != 0 && !IsPoison(); } + // Records a player-crafted potion or poison's effects; false for other forms or unsupported effects. + static bool CaptureRecipe(TESForm* apForm, Inventory::PotionData& aData) noexcept; static void Capture(TESForm* apForm, Inventory::Entry& aEntry) noexcept; static AlchemyItem* Find(TESObjectREFR* apOwner, const Inventory::PotionData& aData) noexcept; // The returned pointer owns one created-object-manager reference. Release it after use. diff --git a/Code/client/Games/Skyrim/TESObjectREFR.cpp b/Code/client/Games/Skyrim/TESObjectREFR.cpp index 5ed88268f..7369586e1 100644 --- a/Code/client/Games/Skyrim/TESObjectREFR.cpp +++ b/Code/client/Games/Skyrim/TESObjectREFR.cpp @@ -558,6 +558,7 @@ void TESObjectREFR::GetItemFromExtraData(Inventory::Entry& arEntry, ExtraDataLis { modSystem.GetServerModId(pExtraPoison->pPoison->formID, arEntry.ExtraPoisonId); arEntry.ExtraPoisonCount = pExtraPoison->uiCount; + AlchemyItem::CaptureRecipe(pExtraPoison->pPoison, arEntry.PoisonData); } } @@ -618,12 +619,18 @@ ExtraDataList* TESObjectREFR::GetExtraDataFromItem(const Inventory::Entry& arEnt if (arEntry.ExtraPoisonId != 0) { - // TODO: does poison have the same temp problem as enchants? - // doesn't seem to be the case, there are only like 3 poisons, and no custom ones - TP_ASSERT(arEntry.ExtraPoisonId.ModId != 0xFFFFFFFF, "Poison is sent as temp!"); - - uint32_t poisonId = modSystem.GetGameId(arEntry.ExtraPoisonId); - if (AlchemyItem* pPoison = Cast(TESForm::GetById(poisonId))) + AlchemyItem* pPoison = nullptr; + if (arEntry.ExtraPoisonId.ModId == 0xFFFFFFFF) + { + // Player-crafted poison: recreate it from the owner's recipe. The created-object reference + // is kept so the form outlives this extra data list being attached to the item. + pPoison = AlchemyItem::Create(arEntry.PoisonData); + if (!pPoison) + spdlog::warn("{}: Unable to recreate applied crafted poison", __FUNCTION__); + } + else + pPoison = Cast(TESForm::GetById(modSystem.GetGameId(arEntry.ExtraPoisonId))); + if (pPoison) { pExtraDataList->SetPoison(pPoison, arEntry.ExtraPoisonCount); } diff --git a/Code/client/Services/Generic/TradeService.cpp b/Code/client/Services/Generic/TradeService.cpp index b84d7527a..f71172fea 100644 --- a/Code/client/Services/Generic/TradeService.cpp +++ b/Code/client/Services/Generic/TradeService.cpp @@ -73,6 +73,52 @@ const char* GetItemCategory(TESForm* apForm) noexcept } } +bool EffectsResolve(ModSystem& aModSystem, const Vector& acEffects) noexcept +{ + for (const auto& effect : acEffects) + if (!Cast(TESForm::GetById(aModSystem.GetGameId(effect.EffectId)))) + return false; + return true; +} + +// Whether this game can rebuild a partner's item: every plugin form it references must be loaded, +// and temporary forms must carry data to recreate them here. +bool CanReceiveTradeItem(ModSystem& aModSystem, const Inventory::Entry& acItem) noexcept +{ + if (!acItem.Potion.Effects.empty()) + { + if (!acItem.Potion.IsValid() || !EffectsResolve(aModSystem, acItem.Potion.Effects)) + return false; + } + else if (acItem.BaseId.ModId == 0xFFFFFFFF || !Cast(TESForm::GetById(aModSystem.GetGameId(acItem.BaseId)))) + return false; + + if (acItem.ExtraEnchantId != 0) + { + if (acItem.ExtraEnchantId.ModId == 0xFFFFFFFF) + { + // Player-crafted enchantment: recreated from its effects on the recipient. + if (acItem.EnchantData.Effects.empty() || !EffectsResolve(aModSystem, acItem.EnchantData.Effects)) + return false; + } + else if (!TESForm::GetById(aModSystem.GetGameId(acItem.ExtraEnchantId))) + return false; + } + + if (acItem.ExtraPoisonId != 0) + { + if (acItem.ExtraPoisonId.ModId == 0xFFFFFFFF) + { + // Applied player-crafted poison: recreated from its effects on the recipient. + if (!acItem.PoisonData.IsValid() || !EffectsResolve(aModSystem, acItem.PoisonData.Effects)) + return false; + } + else if (!Cast(TESForm::GetById(aModSystem.GetGameId(acItem.ExtraPoisonId)))) + return false; + } + return true; +} + std::string MakeDisplayName(ModSystem& aModSystem, const Inventory::Entry& aEntry) { if (!aEntry.Potion.Effects.empty()) @@ -193,23 +239,27 @@ void TradeService::SetReady(bool aReady) noexcept CancelTrade(); return; } - // Keep existing prepared forms alive through cancellation races until the session ends. + // 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(); for (const auto& item : m_session.PartnerItems) { - if (item.Potion.Effects.empty()) + if (!CanReceiveTradeItem(modSystem, item)) continue; - auto* created = AlchemyItem::Create(item.Potion); - if (!created) + if (!item.Potion.Effects.empty()) { - spdlog::error("[TradeService]: Unable to prepare incoming crafted potion; cancelling before inventory changes"); - CancelTrade(); - return; + auto* created = AlchemyItem::Create(item.Potion); + if (!created) + { + spdlog::error("[TradeService]: Unable to prepare incoming crafted potion; leaving it with the partner"); + continue; + } + if (std::find(m_preparedPotions.begin(), m_preparedPotions.end(), created) == m_preparedPotions.end()) + m_preparedPotions.push_back(created); + else + AlchemyItem::Release(created); } - if (std::find(m_preparedPotions.begin(), m_preparedPotions.end(), created) == m_preparedPotions.end()) - m_preparedPotions.push_back(created); - else - AlchemyItem::Release(created); - request.PreparedPotions.push_back(item); + request.AcceptedItems.push_back(item); } } m_transport.Send(request); @@ -314,6 +364,7 @@ void TradeService::OnTradeStarted(const NotifyTradeStarted& acMessage) noexcept m_session.SelfItems.clear(); m_session.PartnerItems.clear(); m_session.SelfInventory.clear(); + m_session.SelfAcceptedItems.clear(); m_session.CountdownMs = 0; m_session.CountdownTotalMs = 0; @@ -336,6 +387,7 @@ void TradeService::OnTradeState(const NotifyTradeState& acMessage) noexcept m_session.SelfItems = acMessage.SelfItems; m_session.PartnerItems = acMessage.PartnerItems; m_session.SelfInventory = acMessage.SelfInventory; + m_session.SelfAcceptedItems = acMessage.SelfAcceptedItems; m_session.CountdownMs = acMessage.CountdownMs; m_session.CountdownTotalMs = acMessage.CountdownTotalMs; @@ -386,7 +438,17 @@ void TradeService::EmitStateToUI() const noexcept auto& modSystem = m_world.GetModSystem(); - auto makeDict = [&](const Inventory::Entry& entry) { + auto formatEffect = [&](const Inventory::EffectItem& effect) { + auto* form = TESForm::GetById(modSystem.GetGameId(effect.EffectId)); + std::string text = fmt::format("{}: {:.0f}", form && form->GetName() ? form->GetName() : "Unknown effect", effect.Magnitude); + if (effect.Area > 0) + text += fmt::format(", {} ft", effect.Area); + if (effect.Duration > 0) + text += fmt::format(" for {}s", effect.Duration); + return text; + }; + + auto makeDict = [&](const Inventory::Entry& entry, const char* apNote = nullptr) { auto dict = CefDictionaryValue::Create(); dict->SetInt("modId", entry.BaseId.ModId); dict->SetInt("baseId", entry.BaseId.BaseId); @@ -400,30 +462,42 @@ void TradeService::EmitStateToUI() const noexcept auto details = CefListValue::Create(); int detailIndex = 0; - for (const auto& effect : entry.Potion.Effects) + if (!entry.Potion.Effects.empty()) { - auto* form = TESForm::GetById(modSystem.GetGameId(effect.EffectId)); - details->SetString(detailIndex++, fmt::format("{}: {} / {}s", form && form->GetName() ? form->GetName() : "Unknown effect", effect.Magnitude, effect.Duration)); + // Temporary IDs belong to the owner's game session; the recipient gets a fresh one from AddPotion. + details->SetString(detailIndex++, fmt::format("Temporary ID: {:08X}", 0xFF000000u | (entry.BaseId.BaseId & 0x00FFFFFFu))); + for (const auto& effect : entry.Potion.Effects) + details->SetString(detailIndex++, formatEffect(effect)); } if (entry.ExtraHealth > 1.0f) details->SetString(detailIndex++, fmt::format("Improvement: {:.0f}%", entry.ExtraHealth * 100.0f)); if (entry.ExtraEnchantId.BaseId || !entry.EnchantData.Effects.empty()) details->SetString(detailIndex++, "Enchanted"); - if (entry.ExtraPoisonId.BaseId) + if (!entry.PoisonData.Effects.empty()) + { + details->SetString(detailIndex++, fmt::format("Poisoned ({} uses left):", entry.ExtraPoisonCount)); + for (const auto& effect : entry.PoisonData.Effects) + details->SetString(detailIndex++, " " + formatEffect(effect)); + } + else if (entry.ExtraPoisonId.BaseId) details->SetString(detailIndex++, "Poisoned"); if (entry.ExtraSoulLevel > 0) details->SetString(detailIndex++, fmt::format("Soul level: {}", entry.ExtraSoulLevel)); + if (apNote) + details->SetString(detailIndex++, apNote); dict->SetList("details", details); return dict; }; std::vector usedCounts(m_session.SelfInventory.size(), 0); + const auto selfAccepted = MatchAcceptedItems(m_session.SelfItems, m_session.SelfAcceptedItems); auto selfList = CefListValue::Create(); for (size_t i = 0; i < m_session.SelfItems.size(); ++i) { const auto& item = m_session.SelfItems[i]; - auto dict = makeDict(item); + const bool notReceivable = m_session.PartnerReady && !selfAccepted[i]; + auto dict = makeDict(item, notReceivable ? "Partner can't receive this item. It stays with you." : nullptr); dict->SetInt("count", std::abs(item.Count)); std::optional match; @@ -451,13 +525,16 @@ void TradeService::EmitStateToUI() const noexcept } pArgs->SetList(5, selfList); + // Partner items this game cannot rebuild are hidden; they are never accepted and stay with the partner. auto partnerList = CefListValue::Create(); - for (size_t i = 0; i < m_session.PartnerItems.size(); ++i) + int partnerIndex = 0; + for (const auto& item : m_session.PartnerItems) { - const auto& item = m_session.PartnerItems[i]; + if (!CanReceiveTradeItem(modSystem, item)) + continue; auto dict = makeDict(item); dict->SetInt("count", std::abs(item.Count)); - partnerList->SetDictionary(static_cast(i), dict); + partnerList->SetDictionary(partnerIndex++, dict); } pArgs->SetList(6, partnerList); diff --git a/Code/client/Services/TradeService.h b/Code/client/Services/TradeService.h index 816fff621..913c7bdd9 100644 --- a/Code/client/Services/TradeService.h +++ b/Code/client/Services/TradeService.h @@ -69,6 +69,7 @@ struct TradeService TiltedPhoques::Vector SelfItems; TiltedPhoques::Vector PartnerItems; TiltedPhoques::Vector SelfInventory; + TiltedPhoques::Vector SelfAcceptedItems; uint32_t CountdownMs{0}; uint32_t CountdownTotalMs{0}; }; diff --git a/Code/encoding/Messages/NotifyTradeState.cpp b/Code/encoding/Messages/NotifyTradeState.cpp index 34ed8dda2..8ce0fc35b 100644 --- a/Code/encoding/Messages/NotifyTradeState.cpp +++ b/Code/encoding/Messages/NotifyTradeState.cpp @@ -19,6 +19,10 @@ void NotifyTradeState::SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) cons Serialization::WriteVarInt(aWriter, SelfInventory.size()); for (const auto& item : SelfInventory) item.Serialize(aWriter); + + Serialization::WriteVarInt(aWriter, SelfAcceptedItems.size()); + for (const auto& item : SelfAcceptedItems) + item.Serialize(aWriter); } void NotifyTradeState::DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept @@ -57,4 +61,13 @@ void NotifyTradeState::DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) no entry.Deserialize(aReader); SelfInventory.push_back(entry); } + + auto acceptedCount = Serialization::ReadVarInt(aReader); + SelfAcceptedItems.reserve(acceptedCount); + for (size_t i = 0; i < acceptedCount; ++i) + { + Inventory::Entry entry; + entry.Deserialize(aReader); + SelfAcceptedItems.push_back(entry); + } } diff --git a/Code/encoding/Messages/NotifyTradeState.h b/Code/encoding/Messages/NotifyTradeState.h index e1fc6a989..5ade628b5 100644 --- a/Code/encoding/Messages/NotifyTradeState.h +++ b/Code/encoding/Messages/NotifyTradeState.h @@ -24,4 +24,6 @@ struct NotifyTradeState final : ServerMessage TiltedPhoques::Vector SelfItems; TiltedPhoques::Vector PartnerItems; TiltedPhoques::Vector SelfInventory; + // Own offered items the ready partner can receive; empty while the partner is not ready. + TiltedPhoques::Vector SelfAcceptedItems; }; diff --git a/Code/encoding/Messages/TradeSetReadyRequest.cpp b/Code/encoding/Messages/TradeSetReadyRequest.cpp index 93b12f948..bf12caa6c 100644 --- a/Code/encoding/Messages/TradeSetReadyRequest.cpp +++ b/Code/encoding/Messages/TradeSetReadyRequest.cpp @@ -3,8 +3,8 @@ void TradeSetReadyRequest::SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept { Serialization::WriteBool(aWriter, Ready); - Serialization::WriteVarInt(aWriter, PreparedPotions.size()); - for (const auto& item : PreparedPotions) + Serialization::WriteVarInt(aWriter, AcceptedItems.size()); + for (const auto& item : AcceptedItems) item.Serialize(aWriter); } @@ -13,12 +13,12 @@ void TradeSetReadyRequest::DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader ClientMessage::DeserializeRaw(aReader); Ready = Serialization::ReadBool(aReader); - PreparedPotions.clear(); + AcceptedItems.clear(); const auto count = Serialization::ReadVarInt(aReader); for (uint64_t i = 0; i < count; ++i) { Inventory::Entry item; item.Deserialize(aReader); - PreparedPotions.push_back(std::move(item)); + AcceptedItems.push_back(std::move(item)); } } diff --git a/Code/encoding/Messages/TradeSetReadyRequest.h b/Code/encoding/Messages/TradeSetReadyRequest.h index d757b3c4f..129bf0c17 100644 --- a/Code/encoding/Messages/TradeSetReadyRequest.h +++ b/Code/encoding/Messages/TradeSetReadyRequest.h @@ -16,5 +16,5 @@ struct TradeSetReadyRequest final : ClientMessage void DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept override; bool Ready{false}; - Vector PreparedPotions; + Vector AcceptedItems; }; diff --git a/Code/encoding/Structs/Inventory.cpp b/Code/encoding/Structs/Inventory.cpp index f373cbac6..5f853e1ec 100644 --- a/Code/encoding/Structs/Inventory.cpp +++ b/Code/encoding/Structs/Inventory.cpp @@ -1,9 +1,16 @@ #include #include +#include #include using TiltedPhoques::Serialization; +bool Inventory::PotionData::operator==(const PotionData& acRhs) const noexcept +{ + return IsPoison == acRhs.IsPoison && Effects.size() == acRhs.Effects.size() && + std::is_permutation(Effects.begin(), Effects.end(), acRhs.Effects.begin()); +} + bool Inventory::PotionData::IsValid() const noexcept { if (Effects.empty() || Effects.size() > 32) @@ -64,6 +71,10 @@ void Inventory::Entry::Serialize(TiltedPhoques::Buffer::Writer& aWriter) const n Serialization::WriteVarInt(aWriter, Potion.Effects.size()); for (const auto& effect : Potion.Effects) effect.Serialize(aWriter); + Serialization::WriteBool(aWriter, PoisonData.IsPoison); + Serialization::WriteVarInt(aWriter, PoisonData.Effects.size()); + for (const auto& effect : PoisonData.Effects) + effect.Serialize(aWriter); } void Inventory::Entry::Deserialize(TiltedPhoques::Buffer::Reader& aReader) noexcept @@ -104,6 +115,15 @@ void Inventory::Entry::Deserialize(TiltedPhoques::Buffer::Reader& aReader) noexc effect.Deserialize(aReader); Potion.Effects.push_back(effect); } + PoisonData.IsPoison = Serialization::ReadBool(aReader); + PoisonData.Effects.clear(); + const auto poisonEffectCount = Serialization::ReadVarInt(aReader); + for (uint64_t i = 0; i < poisonEffectCount; ++i) + { + EffectItem effect; + effect.Deserialize(aReader); + PoisonData.Effects.push_back(effect); + } } bool Inventory::operator==(const Inventory& acRhs) const noexcept diff --git a/Code/encoding/Structs/Inventory.h b/Code/encoding/Structs/Inventory.h index e1f42ccbc..a0e7cc2e5 100644 --- a/Code/encoding/Structs/Inventory.h +++ b/Code/encoding/Structs/Inventory.h @@ -30,8 +30,9 @@ struct Inventory struct PotionData { bool IsPoison{}; + // Engine order is kept so recreation matches the original potion; comparison ignores order. Vector Effects{}; - bool operator==(const PotionData&) const noexcept = default; + bool operator==(const PotionData& acRhs) const noexcept; bool IsValid() const noexcept; }; @@ -51,6 +52,8 @@ struct Inventory GameId ExtraPoisonId{}; uint32_t ExtraPoisonCount{}; + // Recipe of an applied player-crafted poison, whose temporary ID is only valid for its owner. + PotionData PoisonData{}; int32_t ExtraSoulLevel{}; @@ -81,10 +84,17 @@ struct Inventory // TODO: the whole server side state thing is very flawed // since many of these things can and will change, like poison id or charge // or the fact that the enchant id can be temp - return ExtraCharge == acRhs.ExtraCharge && ExtraEnchantId == acRhs.ExtraEnchantId && ExtraEnchantCharge == acRhs.ExtraEnchantCharge && ExtraEnchantRemoveUnequip == acRhs.ExtraEnchantRemoveUnequip && ExtraHealth == acRhs.ExtraHealth && ExtraPoisonId == acRhs.ExtraPoisonId && + return ExtraCharge == acRhs.ExtraCharge && ExtraEnchantId == acRhs.ExtraEnchantId && ExtraEnchantCharge == acRhs.ExtraEnchantCharge && ExtraEnchantRemoveUnequip == acRhs.ExtraEnchantRemoveUnequip && ExtraHealth == acRhs.ExtraHealth && IsSamePoison(acRhs) && ExtraPoisonCount == acRhs.ExtraPoisonCount && ExtraSoulLevel == acRhs.ExtraSoulLevel && ExtraWorn == acRhs.ExtraWorn && ExtraWornLeft == acRhs.ExtraWornLeft && IsQuestItem == acRhs.IsQuestItem; } + bool IsSamePoison(const Entry& acRhs) const noexcept + { + if (!PoisonData.Effects.empty() || !acRhs.PoisonData.Effects.empty()) + return ExtraPoisonId.ModId == 0xFFFFFFFF && acRhs.ExtraPoisonId.ModId == 0xFFFFFFFF && PoisonData == acRhs.PoisonData; + return ExtraPoisonId == acRhs.ExtraPoisonId; + } + bool IsWorn() const noexcept { return ExtraWorn || ExtraWornLeft; } }; diff --git a/Code/encoding/Structs/TradeValidation.h b/Code/encoding/Structs/TradeValidation.h index 0b6b68aea..54b0afe2d 100644 --- a/Code/encoding/Structs/TradeValidation.h +++ b/Code/encoding/Structs/TradeValidation.h @@ -27,19 +27,33 @@ inline bool SameTradeItem(const Inventory::Entry& left, const Inventory::Entry& return true; } -// A ready client must have recreated exactly the incoming crafted potions for this offer. -inline bool ValidatePreparedPotions(const Vector& acIncoming, const Vector& acPrepared) noexcept +// A ready client lists the partner items it can resolve and receive, in offer order. +// Items it leaves out stay with their owner. +inline bool ValidateAcceptedItems(const Vector& acIncoming, const Vector& acAccepted) noexcept { size_t index = 0; for (const auto& item : acIncoming) { - if (item.Potion.Effects.empty()) - continue; - if (index >= acPrepared.size() || acPrepared[index].Count != item.Count || !SameTradeItem(item, acPrepared[index])) - return false; - ++index; + if (index < acAccepted.size() && acAccepted[index].Count == item.Count && SameTradeItem(item, acAccepted[index])) + ++index; + } + return index == acAccepted.size(); +} + +// Marks which offered items appear in the partner's accepted list, matching in offer order. +inline Vector MatchAcceptedItems(const Vector& acOffer, const Vector& acAccepted) noexcept +{ + Vector result(acOffer.size(), false); + size_t index = 0; + for (size_t i = 0; i < acOffer.size(); ++i) + { + if (index < acAccepted.size() && acAccepted[index].Count == acOffer[i].Count && SameTradeItem(acOffer[i], acAccepted[index])) + { + result[i] = true; + ++index; + } } - return index == acPrepared.size(); + return result; } // Reserve quantities across the entire offer so repeated entries cannot reuse the same items. diff --git a/Code/server/Services/TradeService.cpp b/Code/server/Services/TradeService.cpp index f36593c20..af72c9708 100644 --- a/Code/server/Services/TradeService.cpp +++ b/Code/server/Services/TradeService.cpp @@ -266,6 +266,8 @@ void TradeService::OnTradeOfferUpdate(const PacketEvent pSession->Offers[cIndex].Items = std::move(sanitized); pSession->Offers[cIndex].Ready = false; pSession->Offers[1 - cIndex].Ready = false; + pSession->Offers[cIndex].AcceptedItems.clear(); + pSession->Offers[1 - cIndex].AcceptedItems.clear(); ResetCountdown(*pSession); spdlog::debug("[TradeService]: Updated offer for player {} in session {}", pPlayer->GetId(), pSession->Id); @@ -292,6 +294,7 @@ void TradeService::OnTradeSetReady(const PacketEvent& acPa if (!message.Ready) { pSession->Offers[cIndex].Ready = false; + pSession->Offers[cIndex].AcceptedItems.clear(); ResetCountdown(*pSession); SendStateUpdate(*pSession); return; @@ -304,12 +307,18 @@ void TradeService::OnTradeSetReady(const PacketEvent& acPa return; } - if (!ValidatePreparedPotions(pSession->Offers[1 - cIndex].Items, message.PreparedPotions)) + if (!ValidateAcceptedItems(pSession->Offers[1 - cIndex].Items, message.AcceptedItems)) { CancelSession(*pSession, TradeCancelReason::FailedValidation, pPlayer); return; } - pSession->Offers[cIndex].PreparedPotions = message.PreparedPotions; + // Keep the server's canonical copies of the accepted partner items, not the client's echo. + const auto& incoming = pSession->Offers[1 - cIndex].Items; + const auto accepted = MatchAcceptedItems(incoming, message.AcceptedItems); + pSession->Offers[cIndex].AcceptedItems.clear(); + for (size_t i = 0; i < incoming.size(); ++i) + if (accepted[i]) + pSession->Offers[cIndex].AcceptedItems.push_back(incoming[i]); pSession->Offers[cIndex].Ready = true; const bool cBothReady = pSession->Offers[0].Ready && pSession->Offers[1].Ready; @@ -454,6 +463,8 @@ void TradeService::SendStateUpdate(const TradeSession& aSession) const noexcept notify.PartnerItems = aSession.Offers[1 - i].Items; notify.CountdownTotalMs = total; notify.CountdownMs = remaining; + if (aSession.Offers[1 - i].Ready) + notify.SelfAcceptedItems = aSession.Offers[1 - i].AcceptedItems; notify.SelfInventory.clear(); if (auto* pInventory = GetInventoryFor(pPlayer)) notify.SelfInventory = pInventory->Entries; @@ -500,11 +511,13 @@ void TradeService::FinalizeTrade(TradeSession& aSession) noexcept auto* rightInventory = GetInventoryFor(sessionCopy.Players[1]); Inventory leftResult; Inventory rightResult; + // Only items the recipient could resolve change hands; the rest stay with their owner. + const auto& leftTransfer = sessionCopy.Offers[1].AcceptedItems; + const auto& rightTransfer = sessionCopy.Offers[0].AcceptedItems; if (!leftInventory || !rightInventory || - !ValidatePreparedPotions(sessionCopy.Offers[1].Items, sessionCopy.Offers[0].PreparedPotions) || - !ValidatePreparedPotions(sessionCopy.Offers[0].Items, sessionCopy.Offers[1].PreparedPotions) || - !PrepareTradeExchange(*leftInventory, *rightInventory, - sessionCopy.Offers[0].Items, sessionCopy.Offers[1].Items, leftResult, rightResult)) + !ValidateAcceptedItems(sessionCopy.Offers[0].Items, leftTransfer) || + !ValidateAcceptedItems(sessionCopy.Offers[1].Items, rightTransfer) || + !PrepareTradeExchange(*leftInventory, *rightInventory, leftTransfer, rightTransfer, leftResult, rightResult)) { CancelSession(aSession, TradeCancelReason::FailedValidation); return; @@ -519,7 +532,7 @@ void TradeService::FinalizeTrade(TradeSession& aSession) noexcept Player* recipient = sessionCopy.Players[1 - from]; const auto senderEntity = *sender->GetCharacter(); const auto recipientEntity = *recipient->GetCharacter(); - for (const auto& item : sessionCopy.Offers[from].Items) + for (const auto& item : sessionCopy.Offers[1 - from].AcceptedItems) { NotifyInventoryChanges removal; removal.ServerId = World::ToInteger(senderEntity); diff --git a/Code/server/Services/TradeService.h b/Code/server/Services/TradeService.h index f689e39f6..35ffd1e8b 100644 --- a/Code/server/Services/TradeService.h +++ b/Code/server/Services/TradeService.h @@ -46,7 +46,8 @@ class TradeService } TiltedPhoques::Vector Items; - TiltedPhoques::Vector PreparedPotions; + // Items of the partner's offer this player can resolve and receive. + TiltedPhoques::Vector AcceptedItems; bool Ready; }; diff --git a/Code/tests/trading.cpp b/Code/tests/trading.cpp index 73c2e5c49..1cef9b47a 100644 --- a/Code/tests/trading.cpp +++ b/Code/tests/trading.cpp @@ -81,20 +81,108 @@ TEST_CASE("Crafted potion trades identify recipes independently of temporary for } } -TEST_CASE("Potion readiness proofs must match the complete incoming offer", "[trading][alchemy]") +TEST_CASE("Crafted potion recipes compare independently of effect order", "[trading][alchemy]") +{ + auto first = CraftedPotion(); + first.Potion.Effects.push_back(first.Potion.Effects[0]); + first.Potion.Effects[1].EffectId.BaseId += 1; + first.Potion.Effects[1].Magnitude = 5.0f; + auto second = first; + std::swap(second.Potion.Effects[0], second.Potion.Effects[1]); + REQUIRE(first.Potion == second.Potion); + REQUIRE(SameTradeItem(first, second)); + + second.Potion.Effects[0] = second.Potion.Effects[1]; + REQUIRE_FALSE(first.Potion == second.Potion); +} + +TEST_CASE("Applied crafted poisons identify by recipe, not temporary ID", "[trading][alchemy]") +{ + Inventory::Entry sword; + sword.BaseId.BaseId = 0x12EB7; + sword.Count = 1; + sword.ExtraPoisonId.ModId = 0xFFFFFFFF; + sword.ExtraPoisonId.BaseId = 0x900; + sword.ExtraPoisonCount = 3; + sword.PoisonData = CraftedPotion().Potion; + sword.PoisonData.IsPoison = true; + REQUIRE(sword.PoisonData.IsValid()); + + auto recreated = sword; + recreated.ExtraPoisonId.BaseId = 0xABC; + REQUIRE(SameTradeItem(sword, recreated)); + recreated.PoisonData.Effects[0].Magnitude += 1; + REQUIRE_FALSE(SameTradeItem(sword, recreated)); + + TiltedPhoques::Buffer buffer(1024); + TiltedPhoques::Buffer::Writer writer(&buffer); + sword.Serialize(writer); + TiltedPhoques::Buffer::Reader reader(&buffer); + Inventory::Entry decoded; + decoded.Deserialize(reader); + REQUIRE(decoded == sword); + REQUIRE(decoded.PoisonData == sword.PoisonData); +} + +TEST_CASE("Accepted items must be an in-order subset of the incoming offer", "[trading][alchemy]") { auto potion = CraftedPotion(); - REQUIRE_FALSE(ValidatePreparedPotions({potion}, {})); - REQUIRE(ValidatePreparedPotions({potion}, {potion})); + REQUIRE(ValidateAcceptedItems({potion}, {})); + REQUIRE(ValidateAcceptedItems({potion}, {potion})); auto stale = potion; stale.Count += 1; - REQUIRE_FALSE(ValidatePreparedPotions({potion}, {stale})); + REQUIRE_FALSE(ValidateAcceptedItems({potion}, {stale})); stale = potion; stale.Potion.Effects[0].Magnitude += 1; - REQUIRE_FALSE(ValidatePreparedPotions({potion}, {stale})); - REQUIRE_FALSE(ValidatePreparedPotions({}, {potion})); - REQUIRE_FALSE(ValidatePreparedPotions({potion}, {potion, potion})); - REQUIRE(ValidatePreparedPotions({}, {})); + REQUIRE_FALSE(ValidateAcceptedItems({potion}, {stale})); + REQUIRE_FALSE(ValidateAcceptedItems({}, {potion})); + REQUIRE_FALSE(ValidateAcceptedItems({potion}, {potion, potion})); + REQUIRE(ValidateAcceptedItems({}, {})); +} + +TEST_CASE("Only items each recipient accepts change hands", "[trading][alchemy]") +{ + // A offers a modded-effect potion and a vanilla-effect poison; B offers a vanilla weapon and modded armor. + auto moddedPotion = CraftedPotion(1); + moddedPotion.Count = 1; + moddedPotion.Potion.Effects[0].EffectId.ModId = 7; + auto poison = CraftedPotion(2); + poison.Count = 1; + poison.Potion.IsPoison = true; + Inventory::Entry weapon; + weapon.BaseId.BaseId = 0x12EB7; + weapon.Count = 1; + weapon.ExtraEnchantId.ModId = 0xFFFFFFFF; + weapon.ExtraEnchantId.BaseId = 0x800; + weapon.EnchantData.IsWeapon = true; + weapon.EnchantData.Effects.push_back(poison.Potion.Effects[0]); + Inventory::Entry armor; + armor.BaseId.ModId = 9; + armor.BaseId.BaseId = 0x801; + armor.Count = 1; + + Inventory left; + left.Entries = {moddedPotion, poison}; + Inventory right; + right.Entries = {weapon, armor}; + const Vector leftOffer{moddedPotion, poison}; + const Vector rightOffer{weapon, armor}; + const Vector acceptedByB{poison}; + const Vector acceptedByA{weapon}; + REQUIRE(ValidateAcceptedItems(leftOffer, acceptedByB)); + REQUIRE(ValidateAcceptedItems(rightOffer, acceptedByA)); + REQUIRE(MatchAcceptedItems(leftOffer, acceptedByB) == Vector{false, true}); + REQUIRE(MatchAcceptedItems(rightOffer, acceptedByA) == Vector{true, false}); + + Inventory leftResult; + Inventory rightResult; + REQUIRE(PrepareTradeExchange(left, right, acceptedByB, acceptedByA, leftResult, rightResult)); + REQUIRE(leftResult.Entries.size() == 2); + REQUIRE(SameTradeItem(leftResult.Entries[0], moddedPotion)); + REQUIRE(SameTradeItem(leftResult.Entries[1], weapon)); + REQUIRE(rightResult.Entries.size() == 2); + REQUIRE(SameTradeItem(rightResult.Entries[0], armor)); + REQUIRE(SameTradeItem(rightResult.Entries[1], poison)); } TEST_CASE("Potion recipes and readiness proofs round trip through packet factories", "[trading][encoding][alchemy]") @@ -109,7 +197,7 @@ TEST_CASE("Potion recipes and readiness proofs round trip through packet factori { TradeSetReadyRequest request; request.Ready = true; - request.PreparedPotions.push_back(potion); + request.AcceptedItems.push_back(potion); TiltedPhoques::Buffer::Writer writer(&buffer); request.Serialize(writer); TiltedPhoques::Buffer::Reader reader(&buffer); @@ -117,14 +205,15 @@ TEST_CASE("Potion recipes and readiness proofs round trip through packet factori REQUIRE(message); const auto* decoded = static_cast(message.get()); REQUIRE(decoded->Ready); - REQUIRE(decoded->PreparedPotions == request.PreparedPotions); - REQUIRE(decoded->PreparedPotions[0].Potion == potion.Potion); + REQUIRE(decoded->AcceptedItems == request.AcceptedItems); + REQUIRE(decoded->AcceptedItems[0].Potion == potion.Potion); } SECTION("Full inventory state") { NotifyTradeState state; state.PartnerItems.push_back(potion); state.SelfInventory.push_back(potion); + state.SelfAcceptedItems.push_back(potion); TiltedPhoques::Buffer::Writer writer(&buffer); state.Serialize(writer); TiltedPhoques::Buffer::Reader reader(&buffer); @@ -133,6 +222,8 @@ TEST_CASE("Potion recipes and readiness proofs round trip through packet factori const auto* decoded = static_cast(message.get()); REQUIRE(decoded->PartnerItems[0].Potion == potion.Potion); REQUIRE(decoded->SelfInventory[0].Potion == potion.Potion); + REQUIRE(decoded->SelfAcceptedItems.size() == 1); + REQUIRE(decoded->SelfAcceptedItems[0].Potion == potion.Potion); } } diff --git a/PLAYER_TRADING_POTIONS.md b/PLAYER_TRADING_POTIONS.md index 4f6dbbea2..ef083e9fc 100644 --- a/PLAYER_TRADING_POTIONS.md +++ b/PLAYER_TRADING_POTIONS.md @@ -4,13 +4,20 @@ Build both clients and the server from the same commit on `codex/player-trading- 1. Craft a single-effect potion and a potion with multiple effects. Record each effect's magnitude and duration and the stack counts. 2. Give the recipient different Alchemy skills/perks and record their ingredients and Alchemy experience. -3. Offer part of each stack. Verify the offer lists show the actual potion name and effects, and complete the trade. +3. Offer part of each stack. Verify the offer lists show the actual potion name, the owner's temporary ID (FFxxxxxx) and each effect's magnitude and duration, and complete the trade. 4. Check that exactly the offered quantity left the sender and arrived at the recipient. Effects must match; ingredients and Alchemy experience must stay unchanged. 5. Drink one received potion, trade another back, and verify subsequent stack counts. Save/reload and verify the remaining potion still works. 6. Repeat with a crafted poison, with both players offering potions, and with an existing matching potion stack on the recipient. 7. Cancel Ready, change quantities, cancel the trade and disconnect during the countdown. Preparation must not add potions to inventory or consume anything. 8. Trade a normal cooked meal and a normal purchased potion to verify the existing static-form path. -Recreation uses Skyrim's `BGSCreatedObjectManager::AddPotion` with the captured effects directly. It does not invoke the crafting menu or calculate strength from the recipient's skills. The Ready request confirms successful reconstruction; changing an offer invalidates readiness. Unsupported temporary effects and effect conditions are excluded from trading. Custom per-instance names remain local inventory UI metadata. +Recreation uses Skyrim's `BGSCreatedObjectManager::AddPotion` (or `AddPoison` for poisons) with the captured effects directly, in the sender's effect order. It does not invoke the crafting menu or calculate strength from the recipient's skills. The Ready request confirms successful reconstruction; changing an offer invalidates readiness. Unsupported temporary effects and effect conditions are excluded from trading. Custom per-instance names remain local inventory UI metadata. Native engine behavior, including the AddPotion address and created-object reference lifetime, still requires this in-game test. Passing compilation and packet tests does not establish in-game correctness. + +## Items the recipient cannot load + +Each client shows only the partner items it can rebuild: every plugin form the item references must be loaded locally, and crafted (temporary) potions and enchantments must have effects that resolve locally. Weapons coated with a crafted poison carry the poison's recipe and are receivable when its effects resolve; the recipient's coating is recreated with `AddPoison`, keeping the remaining uses. When a player readies, the Ready request lists exactly the partner items that client accepts; the server transfers only those, and everything else stays with its owner. Once the partner is ready, the sender's own offer marks any item the partner cannot receive. + +9. A offers a crafted potion with an effect from a plugin B lacks, plus a crafted poison with vanilla effects. B offers a weapon with a crafted vanilla enchantment, plus armor from a plugin A lacks. Verify A sees only the weapon, B sees only the poison, each sender's offer marks the hidden item after the partner readies, and after the countdown only the weapon and poison change hands. +10. Coat a weapon with a crafted vanilla-effect poison, trade it, and verify the recipient's weapon shows the same poison effects and remaining uses, then hits apply the poison. Repeat with a poison using an effect from a plugin the recipient lacks: the weapon must stay hidden and remain with the sender.