#5643: Fix implementation for merging primitive changes into the targ…

…et map

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<RemoveEntityAction>(difference.baseNode));
+        break;
+
+    case ComparisonResult::EntityDifference::Type::EntityMissingInBase:
+        addAction(std::make_shared<AddEntityAction>(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<MergeOperation>(result.getSourceRootNode(), result.getBaseRootNode());

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);
 };
 
 }

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<RemoveEntityAction>(difference.baseNode));
-        break;
-
-    case ComparisonResult::EntityDifference::Type::EntityMissingInBase:
-        addAction(std::make_shared<AddEntityAction>(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;
-    }
-    };
-}
-
 }
 
 }

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);
 };
 
 }

libs/scene/merge/ThreeWayMergeOperation.cpp

@@ -153,6 +153,17 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::list<Comparison
         _targetDifferences[it->entityName] = 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<Comparison
         if (targetDiff == _targetDifferences.end())
         {
             // Change is targeting an entity that has not been altered in the source map => 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<RemoveEntityAction>(entityToRemove));
+                }
+                break;
+
+            case ComparisonResult::EntityDifference::Type::EntityMissingInBase:
+                addAction(std::make_shared<AddEntityAction>(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();
+}
+
 }
 
 }

libs/scene/merge/ThreeWayMergeOperation.h

@@ -30,6 +30,7 @@ class ThreeWayMergeOperation :
     // Volatile data only needed to construct the actions
     std::map<std::string, std::list<ComparisonResult::EntityDifference>::const_iterator> _sourceDifferences;
     std::map<std::string, std::list<ComparisonResult::EntityDifference>::const_iterator> _targetDifferences;
+    std::map<std::string, INodePtr> _targetEntities;
 
 public:
     using Ptr = std::shared_ptr<ThreeWayMergeOperation>;
@@ -57,6 +58,8 @@ class ThreeWayMergeOperation :
     }
 
 private:
+    INodePtr findTargetEntityByName(const std::string& name);
+
     void processEntityDifferences(const std::list<ComparisonResult::EntityDifference>& sourceDiffs, 
         const std::list<ComparisonResult::EntityDifference>& targetDiffs);
     void processEntityModification(const ComparisonResult::EntityDifference& sourceDiff,

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<AddChildAction>(operation, [](const std::shared_ptr<AddChildAction>& 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<AddChildAction>(operation, [](const std::shared_ptr<AddChildAction>& 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());
+}
+
 }