#5643: Add the only seemingly trivial test which performs a three-way…

… merge with the same map used as source and target.

Fixup the implementation to get a green test result.

libs/scene/merge/ComparisonResult.h

@@ -45,6 +45,12 @@ class ComparisonResult
         };
 
         Type type;
+
+        // A KeyValueDiff is the same if the same key is changed to the same value the same way
+        bool operator==(const KeyValueDifference& other) const
+        {
+            return other.type == type && other.key == key && other.value == value;
+        }
     };
 
     struct PrimitiveDifference
@@ -66,6 +72,8 @@ class ComparisonResult
         INodePtr sourceNode;
         INodePtr baseNode;
         std::string entityName;
+        std::string sourceFingerprint;
+        std::string baseFingerprint;
 
         enum class Type
         {

libs/scene/merge/GraphComparer.cpp

@@ -98,22 +98,24 @@ void GraphComparer::processDifferingEntities(ComparisonResult& result, const Ent
 
     for (const auto& match : matchingByName)
     {
-        auto sourceNode = sourceMismatches.find(match.second.entityName)->second.node;
-        auto baseNode = baseMismatches.find(match.second.entityName)->second.node;
+        auto sourceMismatch = sourceMismatches.find(match.second.entityName)->second;
+        auto baseMismatch = baseMismatches.find(match.second.entityName)->second;
 
         auto& entityDiff = result.differingEntities.emplace_back(ComparisonResult::EntityDifference
         {
-            sourceNode,
-            baseNode,
+            sourceMismatch.node,
+            baseMismatch.node,
             match.second.entityName,
+            sourceMismatch.fingerPrint,
+            baseMismatch.fingerPrint,
             ComparisonResult::EntityDifference::Type::EntityPresentButDifferent
         });
 
         // Analyse the key values
-        entityDiff.differingKeyValues = compareKeyValues(sourceNode, baseNode);
+        entityDiff.differingKeyValues = compareKeyValues(sourceMismatch.node, baseMismatch.node);
 
         // Analyse the child nodes
-        entityDiff.differingChildren = compareChildNodes(sourceNode, baseNode);
+        entityDiff.differingChildren = compareChildNodes(sourceMismatch.node, baseMismatch.node);
     }
 
     for (const auto& mismatch : missingInSource)
@@ -123,6 +125,8 @@ void GraphComparer::processDifferingEntities(ComparisonResult& result, const Ent
             INodePtr(), // source node is empty
             mismatch.second.node,
             mismatch.second.entityName,
+            std::string(),// source fingerprint is empty
+            mismatch.second.fingerPrint, // base fingerprint
             ComparisonResult::EntityDifference::Type::EntityMissingInSource
         });
     }
@@ -134,6 +138,8 @@ void GraphComparer::processDifferingEntities(ComparisonResult& result, const Ent
             mismatch.second.node,
             INodePtr(), // base node is empty
             mismatch.second.entityName,
+            mismatch.second.fingerPrint, // source fingerprint
+            std::string(),// base fingerprint is empty
             ComparisonResult::EntityDifference::Type::EntityMissingInBase
         });
     }

libs/scene/merge/ThreeWayMergeOperation.cpp

@@ -47,8 +47,8 @@ bool ThreeWayMergeOperation::KeyValueDiffHasConflicts(const ComparisonResult::Ke
     // On key value change or add, the value must be the same to not conflict
     case ComparisonResult::KeyValueDifference::Type::KeyValueAdded:
     case ComparisonResult::KeyValueDifference::Type::KeyValueChanged:
-        return sourceKeyValueDiff.type != ComparisonResult::KeyValueDifference::Type::KeyValueRemoved &&
-            sourceKeyValueDiff.value == targetKeyValueDiff.value;
+        return sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueRemoved ||
+            sourceKeyValueDiff.value != targetKeyValueDiff.value;
     }
 
     throw std::logic_error("Unhandled key value diff type in ThreeWayMergeOperation::KeyValueDiffHasConflicts");
@@ -120,8 +120,13 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
             continue;
         }
 
-        // Check if this key change is conflicting with the target change
+        // Ignore NOP changes, when the target obviously made the same change
+        if (sourceKeyValueDiff == *targetKeyValueDiff)
+        {
+            continue;
+        }
 
+        // Check if this key change is conflicting with the target change
         if (!KeyValueDiffHasConflicts(sourceKeyValueDiff, *targetKeyValueDiff))
         {
             // Accept this change
@@ -220,7 +225,11 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::list<Comparison
             }
 
             // Both graphs had this entity added, mark this for inclusion
-            addAction(std::make_shared<AddEntityAction>(pair.second->sourceNode, _targetRoot));
+            // unless it turns out this added entity in the source is 100% the same as in the target
+            if (pair.second->sourceFingerprint != targetDiff->second->sourceFingerprint)
+            {
+                addAction(std::make_shared<AddEntityAction>(pair.second->sourceNode, _targetRoot));
+            }
             break;
 
         case ComparisonResult::EntityDifference::Type::EntityMissingInSource: // entity was removed in source

test/MapMerging.cpp

@@ -1758,9 +1758,9 @@ TEST_F(LayerMergeTest, MergeLayersFlagNotSet)
     EXPECT_FALSE(merger->getChangeLog().empty());
 }
 
-// Map changelog of source and target against their base, used in several test cases below:
+// Map changelog of source and target against their base (threeway_merge_base.mapx), used in several test cases below:
 // 
-// Source:
+// Source (threeway_merge_source_1.mapx):
 // - light_2 has been added
 // - brush 16 added to worldspawn
 // - brush_8 in func_static_8 retextured to brush 9
@@ -1771,7 +1771,7 @@ TEST_F(LayerMergeTest, MergeLayersFlagNotSet)
 // - func_static_5 had two brush_5 added (were part of worldspawn before)
 // - brush_11 got moved to the left
 // 
-// Target Map:
+// Target Map (threeway_merge_target_1.mapx):
 // - light_3 has been added
 // - brush_17 been added to worldspawn
 // - brush_7 in func_static_7 retextured to brush 9
@@ -1781,6 +1781,7 @@ TEST_F(LayerMergeTest, MergeLayersFlagNotSet)
 // - both brush_4 have been deleted from worldspawn
 // - func_static_3 had two brush_3 added (were part of worldspawn before)
 // - brush_12 got moved to the left
+
 ThreeWayMergeOperation::Ptr setupThreeWayMergeOperation(const std::string& basePath, const std::string& targetPath, const std::string& sourcePath)
 {
     auto baseResource = GlobalMapResourceManager().createFromPath(basePath);
@@ -2043,4 +2044,17 @@ TEST_F(ThreeWayMergeTest, NonconflictingPrimitiveMove)
     verifyTargetChanges(operation->getTargetRoot());
 }
 
+// A seemingly trivial case where the source changes and the target changes against their base match up 1:1
+TEST_F(ThreeWayMergeTest, SourceAndTargetAreTheSame)
+{
+    // Load the same map twice as source and target
+    auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_1.mapx", "maps/threeway_merge_target_1.mapx");
+
+    verifyTargetChanges(operation->getTargetRoot());
+
+    auto actionCount = countActions<IMergeAction>(operation, [&](const IMergeAction::Ptr&) { return true; });
+
+    EXPECT_EQ(actionCount, 0);
+}
+
 }