From c2dba31f70c2699c9ce7c6896721df8af1fc52a6 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Wed, 15 Oct 2025 18:45:09 -0500 Subject: [PATCH] fix: bbb splitting dupe issue (#1908) * fix bbb group splitting issues * address feedback --- .github/copilot-instructions.md | 29 ++++ dCommon/Lxfml.cpp | 80 ++++++++-- dGame/dGameMessages/GameMessages.cpp | 8 + resources/worldconfig.ini | 3 + .../LxfmlTestFiles/CMakeLists.txt | 2 + .../LxfmlTestFiles/complex_grouping.lxfml | 132 ++++++++++++++++ .../LxfmlTestFiles/group_issue.lxfml | 48 ++++++ tests/dCommonTests/LxfmlTests.cpp | 142 ++++++++++++++++-- 8 files changed, 421 insertions(+), 23 deletions(-) create mode 100644 .github/copilot-instructions.md create mode 100644 tests/dCommonTests/LxfmlTestFiles/complex_grouping.lxfml create mode 100644 tests/dCommonTests/LxfmlTestFiles/group_issue.lxfml diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..ec4fa8c9 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,29 @@ +# GitHub Copilot Instructions + + * c++20 standard, please use the latest features except NO modules. + * use `.contains` for searching in associative containers + * use const as much as possible. If it can be const, it should be made const + * DO NOT USE const_cast EVER. + * use `cstdint` bitwidth types ALWAYS for integral types. + * NEVER use std::wstring. If wide strings are necessary, use std::u16string with conversion utilties in GeneralUtils.h. + * Functions are ALWAYS PascalCase. + * local variables are camelCase + * NEVER use snake case + * indentation is TABS, not SPACES. + * TABS are 4 spaces by default + * Use trailing braces ALWAYS + * global variables are prefixed with `g_` + * if global variables or functions are needed, they should be located in an anonymous namespace + * Use `GeneralUtils::TryParse` for ANY parsing of strings to integrals. + * Use brace initialization when possible. + * ALWAYS default initialize variables. + * Pointers should be avoided unless necessary. Use references when the pointer has been checked and should not be null + * headers should be as compact as possible. Do NOT include extra data that isnt needed. + * Remember to include logs (LOG macro uses printf style logging) while putting verbose logs under LOG_DEBUG. + * NEVER USE `RakNet::BitStream::ReadBit` + * NEVER assume pointers are good, always check if they are null. Once a pointer is checked and is known to be non-null, further accesses no longer need checking + * Be wary of TOCTOU. Prevent all possible issues relating to TOCTOU. + * new memory allocations should never be used unless absolutely necessary. + * new for reconstruction of objects is allowed + * Prefer following the format of the file over correct formatting. Consistency over correctness. + * When using auto, ALWAYS put a * for pointers. \ No newline at end of file diff --git a/dCommon/Lxfml.cpp b/dCommon/Lxfml.cpp index fd71be4c..b950d037 100644 --- a/dCommon/Lxfml.cpp +++ b/dCommon/Lxfml.cpp @@ -271,6 +271,9 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi std::unordered_set usedBrickRefs; std::unordered_set usedRigidSystems; + // Track used groups to avoid processing them twice + std::unordered_set usedGroups; + // Helper to create output document from sets of brick refs and rigidsystem pointers auto makeOutput = [&](const std::unordered_set& bricksToInclude, const std::vector& rigidSystemsToInclude, const std::vector& groupsToInclude = {}) { tinyxml2::XMLDocument outDoc; @@ -323,19 +326,27 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi // 1) Process groups (each top-level Group becomes one output; nested groups are included) for (auto* groupRoot : groupRoots) { - // collect all partRefs in this group's subtree - std::unordered_set partRefs; - std::function collectParts = [&](const tinyxml2::XMLElement* g) { + // Skip if this group was already processed as part of another group + if (usedGroups.find(groupRoot) != usedGroups.end()) continue; + + // Helper to collect all partRefs in a group's subtree + std::function&)> collectParts = [&](const tinyxml2::XMLElement* g, std::unordered_set& partRefs) { if (!g) return; const char* partAttr = g->Attribute("partRefs"); if (partAttr) { for (auto& tok : GeneralUtils::SplitString(partAttr, ',')) partRefs.insert(tok); } - for (auto* child = g->FirstChildElement("Group"); child; child = child->NextSiblingElement("Group")) collectParts(child); + for (auto* child = g->FirstChildElement("Group"); child; child = child->NextSiblingElement("Group")) collectParts(child, partRefs); }; - collectParts(groupRoot); - // Build initial sets of bricks and boneRefs + // Collect all groups that need to be merged into this output + std::vector groupsToInclude{ groupRoot }; + usedGroups.insert(groupRoot); + + // Build initial sets of bricks and boneRefs from the starting group + std::unordered_set partRefs; + collectParts(groupRoot, partRefs); + std::unordered_set bricksIncluded; std::unordered_set boneRefsIncluded; for (const auto& pref : partRefs) { @@ -355,6 +366,7 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi } // Iteratively include any RigidSystems that reference any boneRefsIncluded + // and check if those rigid systems' bricks span other groups bool changed = true; std::vector rigidSystemsToInclude; int maxIterations = 1000; // Safety limit to prevent infinite loops @@ -362,6 +374,8 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi while (changed && iteration < maxIterations) { changed = false; iteration++; + + // First, expand rigid systems based on current boneRefsIncluded for (auto* rs : rigidSystems) { if (usedRigidSystems.find(rs) != usedRigidSystems.end()) continue; // parse boneRefs of this rigid system (from its children) @@ -392,6 +406,53 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi } } } + + // Second, check if the newly included bricks span any other groups + // If so, merge those groups into the current output + for (auto* otherGroup : groupRoots) { + if (usedGroups.find(otherGroup) != usedGroups.end()) continue; + + // Collect partRefs from this other group + std::unordered_set otherPartRefs; + collectParts(otherGroup, otherPartRefs); + + // Check if any of these partRefs correspond to bricks we've already included + bool spansOtherGroup = false; + for (const auto& pref : otherPartRefs) { + auto pit = partRefToBrick.find(pref); + if (pit != partRefToBrick.end()) { + const char* bref = pit->second->Attribute("refID"); + if (bref && bricksIncluded.find(std::string(bref)) != bricksIncluded.end()) { + spansOtherGroup = true; + break; + } + } + } + + if (spansOtherGroup) { + // Merge this group into the current output + usedGroups.insert(otherGroup); + groupsToInclude.push_back(otherGroup); + changed = true; + + // Add all partRefs, boneRefs, and bricks from this group + for (const auto& pref : otherPartRefs) { + auto pit = partRefToBrick.find(pref); + if (pit != partRefToBrick.end()) { + const char* bref = pit->second->Attribute("refID"); + if (bref) bricksIncluded.insert(std::string(bref)); + } + auto partIt = partRefToPart.find(pref); + if (partIt != partRefToPart.end()) { + auto* bone = partIt->second->FirstChildElement("Bone"); + if (bone) { + const char* bref = bone->Attribute("refID"); + if (bref) boneRefsIncluded.insert(std::string(bref)); + } + } + } + } + } } if (iteration >= maxIterations) { @@ -402,10 +463,9 @@ std::vector Lxfml::Split(const std::string_view data, const NiPoi // include bricks from bricksIncluded into used set for (const auto& b : bricksIncluded) usedBrickRefs.insert(b); - // make output doc and push result (include this group's XML) - std::vector groupsVec{ groupRoot }; - auto normalized = makeOutput(bricksIncluded, rigidSystemsToInclude, groupsVec); - results.push_back(normalized); + // make output doc and push result (include all merged groups' XML) + auto normalized = makeOutput(bricksIncluded, rigidSystemsToInclude, groupsToInclude); + results.push_back(normalized); } // 2) Process remaining RigidSystems (each becomes its own file) diff --git a/dGame/dGameMessages/GameMessages.cpp b/dGame/dGameMessages/GameMessages.cpp index 6855778d..42fd2a58 100644 --- a/dGame/dGameMessages/GameMessages.cpp +++ b/dGame/dGameMessages/GameMessages.cpp @@ -2551,6 +2551,14 @@ void GameMessages::HandleBBBSaveRequest(RakNet::BitStream& inStream, Entity* ent // Uncompress the data, split, and nornmalize the model const auto asStr = sd0.GetAsStringUncompressed(); + + if (Game::config->GetValue("save_lxfmls") == "1") { + // save using localId to avoid conflicts + std::ofstream outFile("debug_lxfml_uncompressed_" + std::to_string(localId) + ".lxfml"); + outFile << asStr; + outFile.close(); + } + auto splitLxfmls = Lxfml::Split(asStr); LOG_DEBUG("Split into %zu models", splitLxfmls.size()); diff --git a/resources/worldconfig.ini b/resources/worldconfig.ini index 38350086..5d23a32f 100644 --- a/resources/worldconfig.ini +++ b/resources/worldconfig.ini @@ -102,3 +102,6 @@ hardcore_disabled_worlds= # Keeps this percentage of a players' coins on death in hardcore hardcore_coin_keep= + +# save pre-split lxfmls to disk for debugging +save_lxfmls=0 diff --git a/tests/dCommonTests/LxfmlTestFiles/CMakeLists.txt b/tests/dCommonTests/LxfmlTestFiles/CMakeLists.txt index e07cbd62..52e709c2 100644 --- a/tests/dCommonTests/LxfmlTestFiles/CMakeLists.txt +++ b/tests/dCommonTests/LxfmlTestFiles/CMakeLists.txt @@ -8,6 +8,8 @@ set(LXFMLTESTFILES "no_bricks.lxfml" "test.lxfml" "too_few_values.lxfml" + "group_issue.lxfml" + "complex_grouping.lxfml" ) # Get the folder name and prepend it to the files above diff --git a/tests/dCommonTests/LxfmlTestFiles/complex_grouping.lxfml b/tests/dCommonTests/LxfmlTestFiles/complex_grouping.lxfml new file mode 100644 index 00000000..b17fe16b --- /dev/null +++ b/tests/dCommonTests/LxfmlTestFiles/complex_grouping.lxfml @@ -0,0 +1,132 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/dCommonTests/LxfmlTestFiles/group_issue.lxfml b/tests/dCommonTests/LxfmlTestFiles/group_issue.lxfml new file mode 100644 index 00000000..7536fd1a --- /dev/null +++ b/tests/dCommonTests/LxfmlTestFiles/group_issue.lxfml @@ -0,0 +1,48 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/dCommonTests/LxfmlTests.cpp b/tests/dCommonTests/LxfmlTests.cpp index 7e1ca4b7..67db1149 100644 --- a/tests/dCommonTests/LxfmlTests.cpp +++ b/tests/dCommonTests/LxfmlTests.cpp @@ -27,20 +27,25 @@ std::string SerializeElement(tinyxml2::XMLElement* elem) { return std::string(p.CStr()); }; -TEST(LxfmlTests, SplitUsesAllBricksAndNoDuplicates) { - // Read the test.lxfml file copied to build directory by CMake - std::string data = ReadFile("test.lxfml"); - ASSERT_FALSE(data.empty()) << "Failed to read test.lxfml from build directory"; +// Helper function to test splitting functionality +static void TestSplitUsesAllBricksAndNoDuplicatesHelper(const std::string& filename) { + // Read the LXFML file + std::string data = ReadFile(filename); + ASSERT_FALSE(data.empty()) << "Failed to read " << filename << " from build directory"; + std::cout << "\n=== Testing LXFML splitting for: " << filename << " ===" << std::endl; + auto results = Lxfml::Split(data); - ASSERT_GT(results.size(), 0); + ASSERT_GT(results.size(), 0) << "Split results should not be empty for " << filename; + + std::cout << "Split produced " << results.size() << " output(s)" << std::endl; // parse original to count bricks tinyxml2::XMLDocument doc; - ASSERT_EQ(doc.Parse(data.c_str()), tinyxml2::XML_SUCCESS); + ASSERT_EQ(doc.Parse(data.c_str()), tinyxml2::XML_SUCCESS) << "Failed to parse " << filename; DocumentReader reader(doc); auto lxfml = reader["LXFML"]; - ASSERT_TRUE(lxfml); + ASSERT_TRUE(lxfml) << "No LXFML element found in " << filename; std::unordered_set originalRigidSet; if (auto* rsParent = doc.FirstChildElement("LXFML")->FirstChildElement("RigidSystems")) { @@ -75,7 +80,20 @@ TEST(LxfmlTests, SplitUsesAllBricksAndNoDuplicates) { // Track used rigid systems and groups (serialized strings) std::unordered_set usedRigidSet; std::unordered_set usedGroupSet; + + std::cout << "Original file contains " << originalBricks.size() << " bricks: "; + for (const auto& brick : originalBricks) { + std::cout << brick << " "; + } + std::cout << std::endl; + + int splitIndex = 0; + std::filesystem::path baseFilename = std::filesystem::path(filename).stem(); + for (const auto& res : results) { + splitIndex++; + std::cout << "\n--- Split " << splitIndex << " ---" << std::endl; + tinyxml2::XMLDocument outDoc; ASSERT_EQ(outDoc.Parse(res.lxfml.c_str()), tinyxml2::XML_SUCCESS); DocumentReader outReader(outDoc); @@ -104,32 +122,130 @@ TEST(LxfmlTests, SplitUsesAllBricksAndNoDuplicates) { } } } + + // Collect and display bricks in this split + std::vector splitBricks; for (const auto& brick : outLxfml["Bricks"]) { const auto* ref = brick.Attribute("refID"); if (ref) { // no duplicate allowed ASSERT_EQ(usedBricks.find(ref), usedBricks.end()) << "Duplicate brick ref across splits: " << ref; usedBricks.insert(ref); + splitBricks.push_back(ref); } } + + std::cout << "Contains " << splitBricks.size() << " bricks: "; + for (const auto& brick : splitBricks) { + std::cout << brick << " "; + } + std::cout << std::endl; + + // Count rigid systems and groups + int rigidCount = 0; + if (auto* rsParent = outDoc.FirstChildElement("LXFML")->FirstChildElement("RigidSystems")) { + for (auto* rs = rsParent->FirstChildElement("RigidSystem"); rs; rs = rs->NextSiblingElement("RigidSystem")) { + rigidCount++; + } + } + + int groupCount = 0; + if (auto* gsParent = outDoc.FirstChildElement("LXFML")->FirstChildElement("GroupSystems")) { + for (auto* gs = gsParent->FirstChildElement("GroupSystem"); gs; gs = gs->NextSiblingElement("GroupSystem")) { + for (auto* g = gs->FirstChildElement("Group"); g; g = g->NextSiblingElement("Group")) { + groupCount++; + } + } + } + + std::cout << "Contains " << rigidCount << " rigid systems and " << groupCount << " groups" << std::endl; } // Every original brick must be used in one of the outputs for (const auto& bref : originalBricks) { - ASSERT_NE(usedBricks.find(bref), usedBricks.end()) << "Brick not used in splits: " << bref; + ASSERT_NE(usedBricks.find(bref), usedBricks.end()) << "Brick not used in splits: " << bref << " in " << filename; } // And usedBricks should not contain anything outside original for (const auto& ub : usedBricks) { - ASSERT_NE(originalBricks.find(ub), originalBricks.end()) << "Split produced unknown brick: " << ub; + ASSERT_NE(originalBricks.find(ub), originalBricks.end()) << "Split produced unknown brick: " << ub << " in " << filename; } // Ensure all original rigid systems and groups were used exactly once - ASSERT_EQ(originalRigidSet.size(), usedRigidSet.size()) << "RigidSystem count mismatch"; - for (const auto& s : originalRigidSet) ASSERT_NE(usedRigidSet.find(s), usedRigidSet.end()) << "RigidSystem missing in splits"; + ASSERT_EQ(originalRigidSet.size(), usedRigidSet.size()) << "RigidSystem count mismatch in " << filename; + for (const auto& s : originalRigidSet) ASSERT_NE(usedRigidSet.find(s), usedRigidSet.end()) << "RigidSystem missing in splits in " << filename; - ASSERT_EQ(originalGroupSet.size(), usedGroupSet.size()) << "Group count mismatch"; - for (const auto& s : originalGroupSet) ASSERT_NE(usedGroupSet.find(s), usedGroupSet.end()) << "Group missing in splits"; + ASSERT_EQ(originalGroupSet.size(), usedGroupSet.size()) << "Group count mismatch in " << filename; + for (const auto& s : originalGroupSet) ASSERT_NE(usedGroupSet.find(s), usedGroupSet.end()) << "Group missing in splits in " << filename; +} + +TEST(LxfmlTests, SplitGroupIssueFile) { + // Specific test for the group issue file + TestSplitUsesAllBricksAndNoDuplicatesHelper("group_issue.lxfml"); +} + +TEST(LxfmlTests, SplitTestFile) { + // Specific test for the larger test file + TestSplitUsesAllBricksAndNoDuplicatesHelper("test.lxfml"); +} + +TEST(LxfmlTests, SplitComplexGroupingFile) { + // Test for the complex grouping file - should produce only one split + // because all groups are connected via rigid systems + std::string data = ReadFile("complex_grouping.lxfml"); + ASSERT_FALSE(data.empty()) << "Failed to read complex_grouping.lxfml from build directory"; + + std::cout << "\n=== Testing complex grouping file ===" << std::endl; + + auto results = Lxfml::Split(data); + ASSERT_GT(results.size(), 0) << "Split results should not be empty"; + + // The complex grouping file should produce exactly ONE split + // because all groups share bricks through rigid systems + if (results.size() != 1) { + FAIL() << "Complex grouping file produced " << results.size() + << " splits instead of 1 (all groups should be merged)"; + } + + std::cout << "✓ Correctly produced 1 merged split" << std::endl; + + // Verify the split contains all the expected elements + tinyxml2::XMLDocument doc; + ASSERT_EQ(doc.Parse(results[0].lxfml.c_str()), tinyxml2::XML_SUCCESS); + + auto* lxfml = doc.FirstChildElement("LXFML"); + ASSERT_NE(lxfml, nullptr); + + // Count bricks + int brickCount = 0; + if (auto* bricks = lxfml->FirstChildElement("Bricks")) { + for (auto* brick = bricks->FirstChildElement("Brick"); brick; brick = brick->NextSiblingElement("Brick")) { + brickCount++; + } + } + std::cout << "Contains " << brickCount << " bricks" << std::endl; + + // Count rigid systems + int rigidCount = 0; + if (auto* rigidSystems = lxfml->FirstChildElement("RigidSystems")) { + for (auto* rs = rigidSystems->FirstChildElement("RigidSystem"); rs; rs = rs->NextSiblingElement("RigidSystem")) { + rigidCount++; + } + } + std::cout << "Contains " << rigidCount << " rigid systems" << std::endl; + EXPECT_GT(rigidCount, 0) << "Should contain rigid systems"; + + // Count groups + int groupCount = 0; + if (auto* groupSystems = lxfml->FirstChildElement("GroupSystems")) { + for (auto* gs = groupSystems->FirstChildElement("GroupSystem"); gs; gs = gs->NextSiblingElement("GroupSystem")) { + for (auto* g = gs->FirstChildElement("Group"); g; g = g->NextSiblingElement("Group")) { + groupCount++; + } + } + } + std::cout << "Contains " << groupCount << " groups" << std::endl; + EXPECT_GT(groupCount, 1) << "Should contain multiple groups (all merged into one split)"; } // Tests for invalid input handling - now working with the improved Split function