diff --git a/libs/scene/merge/MergeOperation.cpp b/libs/scene/merge/MergeOperation.cpp index 4fe8913d04..ecc412434b 100644 --- a/libs/scene/merge/MergeOperation.cpp +++ b/libs/scene/merge/MergeOperation.cpp @@ -9,6 +9,39 @@ namespace scene namespace merge { +MergeOperation::~MergeOperation() +{ + clearActions(); +} + +void MergeOperation::createActionsForEntity(const ComparisonResult::EntityDifference& difference, const IMapRootNodePtr& targetRoot) +{ + switch (difference.type) + { + case ComparisonResult::EntityDifference::Type::EntityMissingInSource: + addAction(std::make_shared(difference.baseNode)); + break; + + case ComparisonResult::EntityDifference::Type::EntityMissingInBase: + addAction(std::make_shared(difference.sourceNode, targetRoot)); + break; + + case ComparisonResult::EntityDifference::Type::EntityPresentButDifferent: + { + for (const auto& keyValueDiff : difference.differingKeyValues) + { + addActionForKeyValueDiff(keyValueDiff, difference.baseNode); + } + + for (const auto& primitiveDiff : difference.differingChildren) + { + addActionsForPrimitiveDiff(primitiveDiff, difference.baseNode); + } + break; + } + }; +} + MergeOperation::Ptr MergeOperation::CreateFromComparisonResult(const ComparisonResult& result) { auto operation = std::make_shared(result.getSourceRootNode(), result.getBaseRootNode()); diff --git a/libs/scene/merge/MergeOperation.h b/libs/scene/merge/MergeOperation.h index a290e01351..1e726e73de 100644 --- a/libs/scene/merge/MergeOperation.h +++ b/libs/scene/merge/MergeOperation.h @@ -34,6 +34,8 @@ class MergeOperation : _mergeLayers(true) {} + virtual ~MergeOperation(); + // Creates the merge operation from the given comparison result. // The operation will (on application) change the base map such that it matches the source map. static Ptr CreateFromComparisonResult(const ComparisonResult& comparisonResult); @@ -43,6 +45,9 @@ class MergeOperation : void setMergeSelectionGroups(bool enabled) override; void setMergeLayers(bool enabled) override; + +private: + void createActionsForEntity(const ComparisonResult::EntityDifference& difference, const IMapRootNodePtr& targetRoot); }; } diff --git a/libs/scene/merge/MergeOperationBase.cpp b/libs/scene/merge/MergeOperationBase.cpp index 1d27354880..ab7c1db69b 100644 --- a/libs/scene/merge/MergeOperationBase.cpp +++ b/libs/scene/merge/MergeOperationBase.cpp @@ -80,34 +80,6 @@ void MergeOperationBase::addActionsForPrimitiveDiff(const ComparisonResult::Prim } } -void MergeOperationBase::createActionsForEntity(const ComparisonResult::EntityDifference& difference, const IMapRootNodePtr& targetRoot) -{ - switch (difference.type) - { - case ComparisonResult::EntityDifference::Type::EntityMissingInSource: - addAction(std::make_shared(difference.baseNode)); - break; - - case ComparisonResult::EntityDifference::Type::EntityMissingInBase: - addAction(std::make_shared(difference.sourceNode, targetRoot)); - break; - - case ComparisonResult::EntityDifference::Type::EntityPresentButDifferent: - { - for (const auto& keyValueDiff : difference.differingKeyValues) - { - addActionForKeyValueDiff(keyValueDiff, difference.baseNode); - } - - for (const auto& primitiveDiff : difference.differingChildren) - { - addActionsForPrimitiveDiff(primitiveDiff, difference.baseNode); - } - break; - } - }; -} - } } diff --git a/libs/scene/merge/MergeOperationBase.h b/libs/scene/merge/MergeOperationBase.h index 5eb4ce34f5..bdfa4b1e0b 100644 --- a/libs/scene/merge/MergeOperationBase.h +++ b/libs/scene/merge/MergeOperationBase.h @@ -34,8 +34,6 @@ class MergeOperationBase : void addActionsForPrimitiveDiff(const ComparisonResult::PrimitiveDifference& difference, const scene::INodePtr& targetEntity); - - void createActionsForEntity(const ComparisonResult::EntityDifference& difference, const IMapRootNodePtr& targetRoot); }; } diff --git a/libs/scene/merge/ThreeWayMergeOperation.cpp b/libs/scene/merge/ThreeWayMergeOperation.cpp index 3bd3137843..6e8dd00048 100644 --- a/libs/scene/merge/ThreeWayMergeOperation.cpp +++ b/libs/scene/merge/ThreeWayMergeOperation.cpp @@ -153,6 +153,17 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::listentityName] = it; } + // Collect a map of all target entities for faster lookup later + _targetRoot->foreachNode([&](const INodePtr& candidate) + { + if (candidate->getNodeType() == INode::Type::Entity) + { + _targetEntities.emplace(NodeUtils::GetEntityName(candidate), candidate); + } + + return true; + }); + // Check each entity difference from the base to the source map // fully accept only those entities that are not altered in the target map, and detect conflicts for (const auto& pair : _sourceDifferences) @@ -162,7 +173,37 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::list accept - createActionsForEntity(*pair.second, _targetRoot); + switch (pair.second->type) + { + case ComparisonResult::EntityDifference::Type::EntityMissingInSource: + { + auto entityToRemove = findTargetEntityByName(pair.first); + assert(entityToRemove); + addAction(std::make_shared(entityToRemove)); + } + break; + + case ComparisonResult::EntityDifference::Type::EntityMissingInBase: + addAction(std::make_shared(pair.second->sourceNode, _targetRoot)); + break; + + case ComparisonResult::EntityDifference::Type::EntityPresentButDifferent: + { + auto entityToModify = findTargetEntityByName(pair.first); + assert(entityToModify); + + for (const auto& keyValueDiff : pair.second->differingKeyValues) + { + addActionForKeyValueDiff(keyValueDiff, entityToModify); + } + + for (const auto& primitiveDiff : pair.second->differingChildren) + { + addActionsForPrimitiveDiff(primitiveDiff, entityToModify); + } + } + break; + }; continue; } @@ -238,6 +279,12 @@ void ThreeWayMergeOperation::setMergeLayers(bool enabled) // TODO } +INodePtr ThreeWayMergeOperation::findTargetEntityByName(const std::string& name) +{ + auto found = _targetEntities.find(name); + return found != _targetEntities.end() ? found->second : INodePtr(); +} + } } diff --git a/libs/scene/merge/ThreeWayMergeOperation.h b/libs/scene/merge/ThreeWayMergeOperation.h index c28d6780bc..a01f0bbfbf 100644 --- a/libs/scene/merge/ThreeWayMergeOperation.h +++ b/libs/scene/merge/ThreeWayMergeOperation.h @@ -30,6 +30,7 @@ class ThreeWayMergeOperation : // Volatile data only needed to construct the actions std::map::const_iterator> _sourceDifferences; std::map::const_iterator> _targetDifferences; + std::map _targetEntities; public: using Ptr = std::shared_ptr; @@ -57,6 +58,8 @@ class ThreeWayMergeOperation : } private: + INodePtr findTargetEntityByName(const std::string& name); + void processEntityDifferences(const std::list& sourceDiffs, const std::list& targetDiffs); void processEntityModification(const ComparisonResult::EntityDifference& sourceDiff, diff --git a/test/MapMerging.cpp b/test/MapMerging.cpp index 6bfcff9cef..29972a0879 100644 --- a/test/MapMerging.cpp +++ b/test/MapMerging.cpp @@ -1815,7 +1815,7 @@ void verifyTargetChanges(const scene::IMapRootNodePtr& targetRoot) EXPECT_TRUE(algorithm::findFirstBrushWithMaterial(algorithm::findWorldspawn(targetRoot), "textures/numbers/12")); // brush_12 got moved to the left } -TEST_F(ThreeWayMergeTest, IndependentEntityAddition) +TEST_F(ThreeWayMergeTest, NonconflictingEntityAddition) { auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_1.mapx", "maps/threeway_merge_source_1.mapx"); @@ -1842,4 +1842,56 @@ TEST_F(ThreeWayMergeTest, IndependentEntityAddition) verifyTargetChanges(operation->getTargetRoot()); } +TEST_F(ThreeWayMergeTest, NonconflictingWorldspawnPrimitiveAddition) +{ + auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_1.mapx", "maps/threeway_merge_source_1.mapx"); + + verifyTargetChanges(operation->getTargetRoot()); + + // brush_16 should be added to worldspawn + auto action = findAction(operation, [](const std::shared_ptr& action) + { + auto sourceBrush = Node_getIBrush(action->getSourceNodeToAdd()); + return sourceBrush && sourceBrush->hasShader("textures/numbers/16"); + }); + + EXPECT_TRUE(action) << "No merge action found for missing brush"; + + // Check pre-requisites and apply the action + EXPECT_FALSE(algorithm::findFirstBrushWithMaterial(algorithm::findWorldspawn(operation->getTargetRoot()), "textures/numbers/16")); // brush_16 not in worldspawn + + action->applyChanges(); + + EXPECT_TRUE(algorithm::findFirstBrushWithMaterial(algorithm::findWorldspawn(operation->getTargetRoot()), "textures/numbers/16")); // brush_16 added to worldspawn + + verifyTargetChanges(operation->getTargetRoot()); +} + +TEST_F(ThreeWayMergeTest, NonconflictingFuncStaticPrimitiveAddition) +{ + auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_1.mapx", "maps/threeway_merge_source_1.mapx"); + + verifyTargetChanges(operation->getTargetRoot()); + + auto func_static_8 = algorithm::getEntityByName(operation->getTargetRoot(), "func_static_8"); + + // brush_9 should be added to func_static_8 + auto action = findAction(operation, [](const std::shared_ptr& action) + { + auto sourceBrush = Node_getIBrush(action->getSourceNodeToAdd()); + return sourceBrush && sourceBrush->hasShader("textures/numbers/9"); + }); + + EXPECT_TRUE(action) << "No merge action found for retextured brush"; + + // Check pre-requisites and apply the action + EXPECT_FALSE(algorithm::findFirstBrushWithMaterial(func_static_8, "textures/numbers/9")); // brush_9 not in func_static_8 + + action->applyChanges(); + + EXPECT_TRUE(algorithm::findFirstBrushWithMaterial(func_static_8, "textures/numbers/9")); // brush_9 added to func_static_8 + + verifyTargetChanges(operation->getTargetRoot()); +} + }