From 3300784cd9d599d010cc13803a811e8777b09e58 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 29 Sep 2026 10:48:18 -0500 Subject: [PATCH] fix(combat): TacArc targets are read and written in the client's order (issue 1045) The client's TacArcBehavior::DoHit (0x00fb10c0) writes the closest max-targets ids from a set, so ascending and each once, then the action data per id in that same order; DoUnserializeBS (0x00fb26a0) reads them the same way and skips empty ids. The server handled the targets in list order and skipped ids whose object it could not find without reading their action data, so every later target read the wrong bits (several pirates under a Doom Slicer). Handle now reads the action for every listed id in ascending order, and the server's own casts write ids and actions in ascending order after picking the closest targets. TacArcBehavior::Cast (0x00fb2d10): a picked target that passes the filter gets the action with no TacArc data (the server's own cast now calculates that action instead of handling it); otherwise the target is dropped, and an arc measured from the target's position writes nothing. Check in game: Doom Slicer and multi-target katanas on groups of pirates/admirals damage each of them; apes still take damage during their stun; enemies with arc attacks (apes, Maelstrom horsemen) still hit players. Co-Authored-By: Claude Opus 5.5 --- dGame/dBehaviors/TacArcBehavior.cpp | 115 ++++++++++++++++------------ dGame/dBehaviors/TacArcBehavior.h | 10 +++ tests/dGameTests/CMakeLists.txt | 1 + tests/dGameTests/TacArcTests.cpp | 75 ++++++++++++++++++ 4 files changed, 153 insertions(+), 48 deletions(-) create mode 100644 tests/dGameTests/TacArcTests.cpp diff --git a/dGame/dBehaviors/TacArcBehavior.cpp b/dGame/dBehaviors/TacArcBehavior.cpp index 076fc5157..01a3b1df4 100644 --- a/dGame/dBehaviors/TacArcBehavior.cpp +++ b/dGame/dBehaviors/TacArcBehavior.cpp @@ -9,24 +9,25 @@ #include "QuickBuildComponent.h" #include "DestroyableComponent.h" +#include #include void TacArcBehavior::Handle(BehaviorContext* context, RakNet::BitStream& bitStream, BehaviorBranchContext branch) { - std::vector targets = {}; - + // TacArcBehavior::Cast (0x00fb2d10): a picked target that passes the filter gets the action and no TacArc data if (this->m_usePickedTarget && branch.target != LWOOBJID_EMPTY) { - auto target = Game::entityManager->GetEntity(branch.target); - if (!target) LOG("target %llu is null", branch.target); - else { - targets.push_back(target); - context->FilterTargets(targets, this->m_ignoreFactionList, this->m_includeFactionList, this->m_targetSelf, this->m_targetEnemy, this->m_targetFriend, this->m_targetTeam); - if (!targets.empty()) { - this->m_action->Handle(context, bitStream, branch); - return; - } + std::vector targets = { Game::entityManager->GetEntity(branch.target) }; + context->FilterTargets(targets, this->m_ignoreFactionList, this->m_includeFactionList, this->m_targetSelf, this->m_targetEnemy, this->m_targetFriend, this->m_targetTeam); + if (!targets.empty()) { + this->m_action->Handle(context, bitStream, branch); + return; } } + // Otherwise the client drops the target, so an arc around the target's position has nothing to measure from and + // writes nothing + branch.target = LWOOBJID_EMPTY; + if (this->m_useTargetPostion) return; + bool hasTargets = false; if (!bitStream.Read(hasTargets)) { LOG("Unable to read hasTargets from bitStream, aborting Handle! %i", bitStream.GetNumberOfUnreadBits()); @@ -48,40 +49,59 @@ void TacArcBehavior::Handle(BehaviorContext* context, RakNet::BitStream& bitStre } if (hasTargets) { - uint32_t count = 0; - if (!bitStream.Read(count)) { - LOG("Unable to read count from bitStream, aborting Handle! %i", bitStream.GetNumberOfUnreadBits()); - return; - }; + std::set targets; + if (!ReadTargets(bitStream, this->m_maxTargets, targets)) return; - if (count > m_maxTargets) { - LOG("Bitstream has too many targets Max:%i Recv:%i", this->m_maxTargets, count); - return; - } - - for (auto i = 0u; i < count; i++) { - LWOOBJID id{}; - - if (!bitStream.Read(id)) { - LOG("Unable to read id from bitStream, aborting Handle! %i", bitStream.GetNumberOfUnreadBits()); - return; - }; - - if (id != LWOOBJID_EMPTY) { - auto* canidate = Game::entityManager->GetEntity(id); - if (canidate) targets.push_back(canidate); - } else { - LOG("Bitstream has LWOOBJID_EMPTY as a target!"); - } - } - - for (auto target : targets) { - branch.target = target->GetObjectID(); + // The caster wrote the action for each of these targets, so it is read for each, even one that is gone here + for (const auto target : targets) { + branch.target = target; this->m_action->Handle(context, bitStream, branch); } } else this->m_missAction->Handle(context, bitStream, branch); } +bool TacArcBehavior::ReadTargets(RakNet::BitStream& bitStream, const uint32_t maxTargets, std::set& targets) { + uint32_t count = 0; + if (!bitStream.Read(count)) { + LOG("Unable to read count from bitStream, aborting Handle! %i", bitStream.GetNumberOfUnreadBits()); + return false; + } + + if (count > maxTargets) { + LOG("Bitstream has too many targets Max:%i Recv:%i", maxTargets, count); + return false; + } + + // TacArcBehavior::DoUnserializeBS (0x00fb26a0) puts the ids in a set: ascending, each once, no empty id + for (auto i = 0u; i < count; i++) { + LWOOBJID id{}; + if (!bitStream.Read(id)) { + LOG("Unable to read id from bitStream, aborting Handle! %i", bitStream.GetNumberOfUnreadBits()); + return false; + } + + if (id == LWOOBJID_EMPTY) { + LOG("Bitstream has LWOOBJID_EMPTY as a target!"); + continue; + } + targets.insert(id); + } + return true; +} + +std::set TacArcBehavior::WriteTargets(RakNet::BitStream& bitStream, const std::vector& closestFirst, const uint32_t maxTargets) { + // TacArcBehavior::DoHit (0x00fb10c0): the closest maxTargets targets, written and acted on in ascending id order + std::set targets; + for (const auto target : closestFirst) { + if (targets.size() >= maxTargets) break; + if (target != LWOOBJID_EMPTY) targets.insert(target); + } + + bitStream.Write(targets.size()); + for (const auto target : targets) bitStream.Write(target); + return targets; +} + void TacArcBehavior::Calculate(BehaviorContext* context, RakNet::BitStream& bitStream, BehaviorBranchContext branch) { auto* self = Game::entityManager->GetEntity(context->originator); if (self == nullptr) { @@ -95,11 +115,14 @@ void TacArcBehavior::Calculate(BehaviorContext* context, RakNet::BitStream& bitS targets.push_back(target); context->FilterTargets(targets, this->m_ignoreFactionList, this->m_includeFactionList, this->m_targetSelf, this->m_targetEnemy, this->m_targetFriend, this->m_targetTeam); if (!targets.empty()) { - this->m_action->Handle(context, bitStream, branch); + this->m_action->Calculate(context, bitStream, branch); return; } } + // As the client: past the picked target check there is no target + branch.target = LWOOBJID_EMPTY; + auto* combatAi = self->GetComponent(); const auto casterPosition = self->GetPosition(); @@ -179,15 +202,11 @@ void TacArcBehavior::Calculate(BehaviorContext* context, RakNet::BitStream& bitS if (combatAi) combatAi->LookAt(targets[0]->GetPosition()); context->foundTarget = true; // We want to continue with this behavior - const auto count = static_cast(targets.size()); - bitStream.Write(count); - for (auto* target : targets) { - bitStream.Write(target->GetObjectID()); - } - - for (auto* target : targets) { - branch.target = target->GetObjectID(); + std::vector closestFirst; + for (const auto* target : targets) closestFirst.push_back(target->GetObjectID()); + for (const auto target : WriteTargets(bitStream, closestFirst, this->m_maxTargets)) { + branch.target = target; this->m_action->Calculate(context, bitStream, branch); } } else { diff --git a/dGame/dBehaviors/TacArcBehavior.h b/dGame/dBehaviors/TacArcBehavior.h index b331e6fc3..f1dbd6050 100644 --- a/dGame/dBehaviors/TacArcBehavior.h +++ b/dGame/dBehaviors/TacArcBehavior.h @@ -3,6 +3,8 @@ #include "dCommonVars.h" #include "NiPoint3.h" #include +#include +#include class TacArcBehavior final : public Behavior { public: @@ -10,6 +12,14 @@ public: void Handle(BehaviorContext* context, RakNet::BitStream& bitStream, BehaviorBranchContext branch) override; void Calculate(BehaviorContext* context, RakNet::BitStream& bitStream, BehaviorBranchContext branch) override; void Load() override; + + // Reads the target count and ids the way the client writes them: at most maxTargets, returned ascending and + // without empty ids. False when the data is cut short or lists too many targets. + static bool ReadTargets(RakNet::BitStream& bitStream, uint32_t maxTargets, std::set& targets); + + // Writes the closest maxTargets of closestFirst as the client does and returns them in the order their action + // data follows (ascending id) + static std::set WriteTargets(RakNet::BitStream& bitStream, const std::vector& closestFirst, uint32_t maxTargets); private: float m_maxRange; float m_height; diff --git a/tests/dGameTests/CMakeLists.txt b/tests/dGameTests/CMakeLists.txt index d3179832d..207a46d6a 100644 --- a/tests/dGameTests/CMakeLists.txt +++ b/tests/dGameTests/CMakeLists.txt @@ -23,6 +23,7 @@ set(DGAMETEST_SOURCES "SetCurrencySourceTests.cpp" "LootDropPositionTests.cpp" "SpiderQueenTests.cpp" + "TacArcTests.cpp" ) add_subdirectory(dComponentsTests) diff --git a/tests/dGameTests/TacArcTests.cpp b/tests/dGameTests/TacArcTests.cpp new file mode 100644 index 000000000..d9a4a1ad3 --- /dev/null +++ b/tests/dGameTests/TacArcTests.cpp @@ -0,0 +1,75 @@ +#include + +#include "BitStream.h" +#include "GameDependencies.h" +#include "TacArcBehavior.h" + +// TacArc target lists as the client writes and reads them (TacArcBehavior::DoHit / DoUnserializeBS) +class TacArcTests : public GameDependenciesTest { +protected: + void SetUp() override { SetUpDependencies(); } + void TearDown() override { TearDownDependencies(); } +}; + +namespace { + void WriteIds(RakNet::BitStream& stream, const std::vector& ids) { + stream.Write(ids.size()); + for (const auto id : ids) stream.Write(id); + } +} + +TEST_F(TacArcTests, TargetsAreReadAscendingAndOnce) { + // The client handles each listed target once, in ascending id order, and skips empty ids + RakNet::BitStream stream; + WriteIds(stream, { 30, 10, LWOOBJID_EMPTY, 20, 10 }); + stream.Write(0xAB); // the actions' data follows + + std::set targets; + ASSERT_TRUE(TacArcBehavior::ReadTargets(stream, 100, targets)); + EXPECT_EQ(std::vector(targets.begin(), targets.end()), (std::vector{ 10, 20, 30 })); + + uint8_t next = 0; + ASSERT_TRUE(stream.Read(next)); + EXPECT_EQ(next, 0xAB); +} + +TEST_F(TacArcTests, TooManyTargetsAreRefused) { + RakNet::BitStream stream; + WriteIds(stream, { 1, 2, 3 }); + std::set targets; + EXPECT_FALSE(TacArcBehavior::ReadTargets(stream, 2, targets)); +} + +TEST_F(TacArcTests, CutShortDataIsRefused) { + RakNet::BitStream stream; + stream.Write(2); + stream.Write(5); + std::set targets; + EXPECT_FALSE(TacArcBehavior::ReadTargets(stream, 100, targets)); +} + +TEST_F(TacArcTests, TheClosestTargetsAreWrittenAscending) { + // Closest first; the two closest are kept and written in ascending id order, as the client's DoHit does + RakNet::BitStream stream; + const auto order = TacArcBehavior::WriteTargets(stream, { 90, 40, 70 }, 2); + EXPECT_EQ(std::vector(order.begin(), order.end()), (std::vector{ 40, 90 })); + + uint32_t count = 0; + LWOOBJID first = 0; + LWOOBJID second = 0; + ASSERT_TRUE(stream.Read(count)); + ASSERT_TRUE(stream.Read(first)); + ASSERT_TRUE(stream.Read(second)); + EXPECT_EQ(count, 2u); + EXPECT_EQ(first, 40); + EXPECT_EQ(second, 90); + EXPECT_EQ(stream.GetNumberOfUnreadBits(), 0); +} + +TEST_F(TacArcTests, WrittenTargetsReadBack) { + RakNet::BitStream stream; + const auto written = TacArcBehavior::WriteTargets(stream, { 7, 3, 5 }, 100); + std::set read; + ASSERT_TRUE(TacArcBehavior::ReadTargets(stream, 100, read)); + EXPECT_EQ(read, written); +}