From 0ff5d2bf87e34007f8b535adbe75f59c4dfafc94 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 27 Oct 2026 09:34:31 -0500 Subject: [PATCH] fix: property model and brick by brick fixes (#2041) * test: the server mock keeps a copy of the last bitstream sent The bitstreams sent are usually on the sender's stack, so the pointer it kept was dangling once Send returned. Co-Authored-By: Claude Opus 5.5 * fix: PlaceModelResponse writes the model's rotation The client's PlaceModelResponse::Deserialize (0x00dc0170) reads the rotation as an optional w, x, y, z quaternion, but the server wrote the 4-byte response after the rotation flag, so with any rotation other than identity the client ran out of data. Co-Authored-By: Claude Opus 5.5 * fix: naming a model no longer renames the property The model info window sends UpdatePropertyOrModelForFilterCheck with isProperty false (client 0x00cc42e0); only the property editor sends it true (0x00c2e710). A model's name and description aren't stored yet. Co-Authored-By: Claude Opus 5.5 * fix: keep a model item's blueprint across saves A brick built model item's config has blueprintid (lowercase), which the saved item XML didn't keep (only blueprintID), so a picked up model lost its blueprint on the next load and could not be placed again. It is now saved as bp. Co-Authored-By: Claude Opus 5.5 * fix: write model behaviors only for property and inventory models The client makes the mutable model component, which reads the behaviors and editing info, only when the object's config has propertyObjectID or inInventory (ObjectLoader2::LoadModelBehaviorsComponent); every other model gets the plain one, which reads only the model block. Writing the behaviors for every model made the client read them as the next component's data. Co-Authored-By: Claude Opus 5.5 * fix: the model vault uses the models' inventory groups, as the client LWOInventoryComponent_Common::msgUpdateInventoryGroup (0x00c955b0) and msgUpdateInventoryGroupContents (0x00ce1f50) look the inventory type up in m_VaultToPlayerInventory first, which maps VAULT_MODELS to MODELS. The server refused group updates for the model vault. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- dGame/dComponents/InventoryComponent.cpp | 6 +- dGame/dComponents/InventoryComponent.h | 2 + dGame/dComponents/ModelComponent.cpp | 5 ++ dGame/dGameMessages/GameMessages.cpp | 10 +++- dGame/dInventory/Item.cpp | 2 + tests/dGameTests/GameDependencies.h | 10 +++- .../dComponentsTests/CMakeLists.txt | 2 + .../dComponentsTests/InventoryGroupTests.cpp | 56 +++++++++++++++++++ .../dComponentsTests/ModelComponentTests.cpp | 55 ++++++++++++++++++ .../dGameMessagesTests/GameMessageTests.cpp | 45 +++++++++++++++ 10 files changed, 187 insertions(+), 6 deletions(-) create mode 100644 tests/dGameTests/dComponentsTests/InventoryGroupTests.cpp create mode 100644 tests/dGameTests/dComponentsTests/ModelComponentTests.cpp diff --git a/dGame/dComponents/InventoryComponent.cpp b/dGame/dComponents/InventoryComponent.cpp index 076625164..cba009546 100644 --- a/dGame/dComponents/InventoryComponent.cpp +++ b/dGame/dComponents/InventoryComponent.cpp @@ -1688,12 +1688,14 @@ bool InventoryComponent::SetSkill(BehaviorSlot slot, uint32_t skillId) { void InventoryComponent::UpdateGroup(const GroupUpdate& groupUpdate) { if (groupUpdate.groupId.empty()) return; - if (groupUpdate.inventory != eInventoryType::BRICKS && groupUpdate.inventory != eInventoryType::MODELS) { + // The model vault shares the models' groups (the client's m_VaultToPlayerInventory: VAULT_MODELS -> MODELS) + const auto inventory = groupUpdate.inventory == eInventoryType::VAULT_MODELS ? eInventoryType::MODELS : groupUpdate.inventory; + if (inventory != eInventoryType::BRICKS && inventory != eInventoryType::MODELS) { LOG("Invalid inventory type for grouping %s", StringifiedEnum::ToString(groupUpdate.inventory).data()); return; } - auto& groups = m_Groups[groupUpdate.inventory]; + auto& groups = m_Groups[inventory]; auto groupItr = std::ranges::find_if(groups, [&groupUpdate](const Group& group) { return group.groupId == groupUpdate.groupId; }); diff --git a/dGame/dComponents/InventoryComponent.h b/dGame/dComponents/InventoryComponent.h index d7b67cfc2..a46954937 100644 --- a/dGame/dComponents/InventoryComponent.h +++ b/dGame/dComponents/InventoryComponent.h @@ -409,6 +409,8 @@ public: void UpdateGroup(const GroupUpdate& groupUpdate); + const std::map>& GetGroups() const { return m_Groups; } + std::unordered_map& GetPetsMut() { return m_Pets; }; void FixInvisibleItems(); diff --git a/dGame/dComponents/ModelComponent.cpp b/dGame/dComponents/ModelComponent.cpp index b09c655a0..eb5f06927 100644 --- a/dGame/dComponents/ModelComponent.cpp +++ b/dGame/dComponents/ModelComponent.cpp @@ -156,6 +156,11 @@ void ModelComponent::Serialize(RakNet::BitStream& outBitStream, bool bIsInitialU outBitStream.Write(m_OriginalPosition); // Original position outBitStream.Write(m_OriginalRotation); // Original rotation + // The client makes the mutable model component, which reads the behaviors and editing info, only for objects whose + // config (the settings sent with a model) has propertyObjectID or inInventory; the plain one it makes for every + // other model reads only the block above, as live wrote it + if (!m_Parent->HasVar(u"propertyObjectID") && !m_Parent->GetVar(u"inInventory")) return; + outBitStream.Write1(); // We are writing behavior info outBitStream.Write(m_Behaviors.size()); // Number of behaviors outBitStream.Write(m_IsPaused); // Is this model paused diff --git a/dGame/dGameMessages/GameMessages.cpp b/dGame/dGameMessages/GameMessages.cpp index 19d5859d6..aa2bd970d 100644 --- a/dGame/dGameMessages/GameMessages.cpp +++ b/dGame/dGameMessages/GameMessages.cpp @@ -2033,9 +2033,13 @@ void GameMessages::SendPlaceModelResponse(LWOOBJID objectId, const SystemAddress bitStream.Write(response); } + // The client's PlaceModelResponse::Deserialize (0x00dc0170) reads the rotation as a w, x, y, z quaternion bitStream.Write(rotation != QuatUtils::IDENTITY); if (rotation != QuatUtils::IDENTITY) { - bitStream.Write(response); + bitStream.Write(rotation.w); + bitStream.Write(rotation.x); + bitStream.Write(rotation.y); + bitStream.Write(rotation.z); } if (sysAddr == UNASSIGNED_SYSTEM_ADDRESS) SEND_PACKET_BROADCAST; @@ -2151,6 +2155,10 @@ void GameMessages::HandleUpdatePropertyOrModelForFilterCheck(RakNet::BitStream& name.push_back(character); } + // A model's name and description (the property editor's model info window, + // LWOPropertyEditorComponent::OnUpdateModelDescription 0x00cc42e0) aren't stored yet; they must not rename the property + if (!isProperty) return; + PropertyManagementComponent::Instance()->UpdatePropertyDetails(GeneralUtils::UTF16ToWTF8(name), GeneralUtils::UTF16ToWTF8(description)); } diff --git a/dGame/dInventory/Item.cpp b/dGame/dInventory/Item.cpp index f18cd0b3d..a06ea474b 100644 --- a/dGame/dInventory/Item.cpp +++ b/dGame/dInventory/Item.cpp @@ -34,6 +34,8 @@ namespace { const std::map ExtraSettingAbbreviations = { { "assemblyPartLOTs", "ma" }, { "blueprintID", "b" }, + // A brick built model item's blueprint, kept so the model can be placed again after a reload + { "blueprintid", "bp" }, { "userModelID", "ui" }, { "userModelName", "un" }, { "userModelDesc", "ud" }, diff --git a/tests/dGameTests/GameDependencies.h b/tests/dGameTests/GameDependencies.h index 480d1387f..112665f1a 100644 --- a/tests/dGameTests/GameDependencies.h +++ b/tests/dGameTests/GameDependencies.h @@ -17,12 +17,16 @@ class dZoneManager; class AssetManager; class dServerMock : public dServer { - RakNet::BitStream* sentBitStream = nullptr; + // A copy: the bitstreams sent are usually on the sender's stack + std::unique_ptr sentBitStream; public: dServerMock() {}; ~dServerMock() {}; - RakNet::BitStream* GetMostRecentBitStream() { return sentBitStream; }; - void Send(RakNet::BitStream& bitStream, const SystemAddress& sysAddr, bool broadcast) override { sentBitStream = &bitStream; }; + RakNet::BitStream* GetMostRecentBitStream() { return sentBitStream.get(); }; + void Send(RakNet::BitStream& bitStream, const SystemAddress& sysAddr, bool broadcast) override { + sentBitStream = std::make_unique(bitStream.GetData(), bitStream.GetNumberOfBytesUsed(), true); + sentBitStream->SetWriteOffset(bitStream.GetNumberOfBitsUsed()); + }; void SetZoneId(unsigned int zoneId) { mZoneID = zoneId; } }; diff --git a/tests/dGameTests/dComponentsTests/CMakeLists.txt b/tests/dGameTests/dComponentsTests/CMakeLists.txt index f73f1214c..776074a5b 100644 --- a/tests/dGameTests/dComponentsTests/CMakeLists.txt +++ b/tests/dGameTests/dComponentsTests/CMakeLists.txt @@ -1,5 +1,7 @@ set(DCOMPONENTS_TESTS "DestroyableComponentTests.cpp" + "InventoryGroupTests.cpp" + "ModelComponentTests.cpp" "PetComponentTests.cpp" "SimplePhysicsComponentTests.cpp" "SavingTests.cpp" diff --git a/tests/dGameTests/dComponentsTests/InventoryGroupTests.cpp b/tests/dGameTests/dComponentsTests/InventoryGroupTests.cpp new file mode 100644 index 000000000..acb7a73bb --- /dev/null +++ b/tests/dGameTests/dComponentsTests/InventoryGroupTests.cpp @@ -0,0 +1,56 @@ +#include "GameDependencies.h" + +#include "CDClientManager.h" +#include "CDComponentsRegistryTable.h" +#include "Entity.h" +#include "InventoryComponent.h" + +#include + +class InventoryGroupTests : public GameDependenciesTest { +protected: + std::unique_ptr entity; + InventoryComponent* inventory{}; + + void SetUp() override { + SetUpDependencies(); + CDClientManager::GetEntriesMutable().insert_or_assign(static_cast(info.lot), 0); + entity = std::make_unique(1, info); + inventory = entity->AddComponent(-1); + } + + void TearDown() override { + entity.reset(); + TearDownDependencies(); + } + + void Update(const InventoryComponent::GroupUpdateCommand command, const std::string& id, const eInventoryType type, const LOT lot = LOT_NULL) { + InventoryComponent::GroupUpdate update{}; + update.groupId = id; + update.groupName = "name"; + update.lot = lot; + update.inventory = type; + update.command = command; + inventory->UpdateGroup(update); + } +}; + +// The client's LWOInventoryComponent_Common::msgUpdateInventoryGroup (0x00c955b0) and msgUpdateInventoryGroupContents +// (0x00ce1f50) map the model vault to the models inventory (m_VaultToPlayerInventory) +TEST_F(InventoryGroupTests, TheModelVaultUsesTheModelsGroups) { + Update(InventoryComponent::GroupUpdateCommand::ADD, "user_group1", eInventoryType::VAULT_MODELS); + Update(InventoryComponent::GroupUpdateCommand::ADD_LOT, "user_group1", eInventoryType::VAULT_MODELS, 3001); + + const auto& groups = inventory->GetGroups(); + ASSERT_EQ(groups.at(eInventoryType::MODELS).size(), 1u); + EXPECT_EQ(groups.at(eInventoryType::MODELS)[0].lots.size(), 1u); + EXPECT_FALSE(groups.contains(eInventoryType::VAULT_MODELS)); +} + +TEST_F(InventoryGroupTests, OnlyBricksAndModelsHaveGroups) { + Update(InventoryComponent::GroupUpdateCommand::ADD, "user_group1", eInventoryType::ITEMS); + EXPECT_FALSE(inventory->GetGroups().contains(eInventoryType::ITEMS)); + + Update(InventoryComponent::GroupUpdateCommand::ADD, "user_group1", eInventoryType::BRICKS); + EXPECT_EQ(inventory->GetGroups().at(eInventoryType::BRICKS).size(), 1u); +} diff --git a/tests/dGameTests/dComponentsTests/ModelComponentTests.cpp b/tests/dGameTests/dComponentsTests/ModelComponentTests.cpp new file mode 100644 index 000000000..23373fa93 --- /dev/null +++ b/tests/dGameTests/dComponentsTests/ModelComponentTests.cpp @@ -0,0 +1,55 @@ +#include "GameDependencies.h" + +#include "BitStream.h" +#include "CDClientManager.h" +#include "CDComponentsRegistryTable.h" +#include "Entity.h" +#include "GameMessages.h" +#include "ModelComponent.h" + +#include + +class ModelComponentTests : public GameDependenciesTest { +protected: + void SetUp() override { + SetUpDependencies(); + CDClientManager::GetEntriesMutable().insert_or_assign(static_cast(info.lot), 0); + } + + void TearDown() override { + TearDownDependencies(); + } + + // The bits a model's serialization writes + static uint32_t SerializedBits(Entity& entity) { + auto* model = entity.AddComponent(-1); + RakNet::BitStream bitStream; + model->Serialize(bitStream, true); + return bitStream.GetNumberOfBitsUsed(); + } +}; + +// Item block (1 + 64 + 32 + 1), model block (1 + 1 + 32 + 96 + 128) +constexpr uint32_t ModelBlockBits = 98 + 258; +// Behavior info: flag, count, paused, and on the initial update the editing info flag +constexpr uint32_t BehaviorBits = 1 + 32 + 1 + 1; + +// The client reads the behaviors only for property and inventory models (ObjectLoader2::LoadModelBehaviorsComponent) +TEST_F(ModelComponentTests, OtherModelsHaveNoBehaviorInfo) { + Entity entity(1, info); + EXPECT_EQ(SerializedBits(entity), ModelBlockBits); +} + +TEST_F(ModelComponentTests, PropertyModelsHaveBehaviorInfo) { + EntityInfo propertyInfo = info; + propertyInfo.settings.Insert(u"propertyObjectID", true); + Entity entity(1, propertyInfo); + EXPECT_EQ(SerializedBits(entity), ModelBlockBits + BehaviorBits); +} + +TEST_F(ModelComponentTests, InventoryModelsHaveBehaviorInfo) { + EntityInfo inventoryInfo = info; + inventoryInfo.settings.Insert(u"inInventory", true); + Entity entity(1, inventoryInfo); + EXPECT_EQ(SerializedBits(entity), ModelBlockBits + BehaviorBits); +} diff --git a/tests/dGameTests/dGameMessagesTests/GameMessageTests.cpp b/tests/dGameTests/dGameMessagesTests/GameMessageTests.cpp index e75dc5c42..7e6b704e1 100644 --- a/tests/dGameTests/dGameMessagesTests/GameMessageTests.cpp +++ b/tests/dGameTests/dGameMessagesTests/GameMessageTests.cpp @@ -85,6 +85,51 @@ TEST_F(GameMessageTests, SendBlueprintLoadItemResponse) { ASSERT_EQ(bitStream->GetNumberOfUnreadBits(), 0); } +/** + * @brief PlaceModelResponse is laid out as the client's PlaceModelResponse::Deserialize (0x00dc0170) reads it: every + * field optional, the rotation a w, x, y, z quaternion + */ +TEST_F(GameMessageTests, SendPlaceModelResponse) { + const NiQuaternion rotation = NiQuaternion(0.5f, 0.1f, 0.2f, 0.3f); // w, x, y, z + GameMessages::SendPlaceModelResponse(77, UNASSIGNED_SYSTEM_ADDRESS, NiPoint3(1.0f, 2.0f, 3.0f), LWOOBJID_EMPTY, 14, rotation); + auto* bitStream = static_cast(Game::server)->GetMostRecentBitStream(); + ASSERT_NE(bitStream, nullptr); + bitStream->IgnoreBits(64); // packet header + + LWOOBJID objectId{}; + MessageType::Game messageId{}; + bitStream->Read(objectId); + bitStream->Read(messageId); + EXPECT_EQ(objectId, 77); + EXPECT_EQ(messageId, MessageType::Game::PLACE_MODEL_RESPONSE); + + bool hasPosition{}, hasPlaque{}, hasResponse{}, hasRotation{}; + NiPoint3 position{}; + int32_t response{}; + float w{}, x{}, y{}, z{}; + bitStream->Read(hasPosition); + bitStream->Read(position); + bitStream->Read(hasPlaque); + bitStream->Read(hasResponse); + bitStream->Read(response); + bitStream->Read(hasRotation); + bitStream->Read(w); + bitStream->Read(x); + bitStream->Read(y); + bitStream->Read(z); + EXPECT_TRUE(hasPosition); + EXPECT_EQ(position, NiPoint3(1.0f, 2.0f, 3.0f)); + EXPECT_FALSE(hasPlaque); + EXPECT_TRUE(hasResponse); + EXPECT_EQ(response, 14); + EXPECT_TRUE(hasRotation); + EXPECT_FLOAT_EQ(w, rotation.w); + EXPECT_FLOAT_EQ(x, rotation.x); + EXPECT_FLOAT_EQ(y, rotation.y); + EXPECT_FLOAT_EQ(z, rotation.z); + EXPECT_EQ(bitStream->GetNumberOfUnreadBits(), 0); +} + TEST_F(GameMessageTests, ControlBehaviorAddStrip) { auto data = ReadFromFile("addStrip"); RakNet::BitStream inStream(reinterpret_cast(&data[0]), data.length(), true);