#5643: Add a ConflictType enum to be able to specifically tell what a…

… conflict is about.

Expand unit tests by some value change conflicts.

include/imapmerge.h

@@ -20,8 +20,28 @@ enum class ActionType
     ChangeKeyValue,
     AddChildNode,
     RemoveChildNode,
-    EntityNodeConflict,
-    EntityKeyValueConflict,
+    ConflictResolution,
+};
+
+enum class ConflictType
+{
+    // Not a conflict
+    NoConflict,
+
+    // Entity has been removed in target, source tries to modify it
+    ModificationOfRemovedEntity,
+
+    // Entity has been modified in target, source tries to remove it
+    RemovalOfModifiedEntity,
+
+    // Key Value has been removed in targed, source tries to change it
+    ModificationOfRemovedKeyValue,
+
+    // Key Value has been modified in targed, source tries to remove it
+    RemovalOfModifiedKeyValue,
+
+    // Both sides try to set the same key to a different value
+    SettingKeyToDifferentValue,
 };
 
 /**

libs/scene/merge/MergeAction.h

@@ -280,6 +280,7 @@ class ConflictResolutionAction :
     public MergeAction
 {
 protected:
+    ConflictType _conflictType;
     INodePtr _conflictingEntity;
 
     // The action the source diff is trying to apply
@@ -290,12 +291,13 @@ class ConflictResolutionAction :
     bool _applySourceChange;
 
 protected:
-    ConflictResolutionAction(ActionType actionType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction) :
-        ConflictResolutionAction(actionType, conflictingEntity, sourceAction, MergeAction::Ptr())
+    ConflictResolutionAction(ConflictType conflictType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction) :
+        ConflictResolutionAction(conflictType, conflictingEntity, sourceAction, MergeAction::Ptr())
     {}
 
-    ConflictResolutionAction(ActionType actionType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction, const MergeAction::Ptr& targetAction) :
-        MergeAction(actionType),
+    ConflictResolutionAction(ConflictType conflictType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction, const MergeAction::Ptr& targetAction) :
+        MergeAction(ActionType::ConflictResolution),
+        _conflictType(conflictType),
         _conflictingEntity(conflictingEntity),
         _sourceAction(sourceAction),
         _targetAction(targetAction),
@@ -305,6 +307,11 @@ class ConflictResolutionAction :
 public:
     using Ptr = std::shared_ptr<ConflictResolutionAction>;
 
+    ConflictType getConflictType() const
+    {
+        return _conflictType;
+    }
+
     // The action the source diff is trying to apply
     const MergeAction::Ptr& getSourceAction() const
     {
@@ -353,14 +360,15 @@ class EntityConflictResolutionAction :
     public ConflictResolutionAction
 {
 public:
-    EntityConflictResolutionAction(const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction) :
-        EntityConflictResolutionAction(conflictingEntity, sourceAction, MergeAction::Ptr())
+    EntityConflictResolutionAction(ConflictType conflictType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction) :
+        EntityConflictResolutionAction(conflictType, conflictingEntity, sourceAction, MergeAction::Ptr())
     {}
 
-    EntityConflictResolutionAction(const INodePtr& conflictingEntity, 
+    EntityConflictResolutionAction(ConflictType conflictType, 
+                                   const INodePtr& conflictingEntity,
                                    const MergeAction::Ptr& sourceAction, 
                                    const MergeAction::Ptr& targetAction) :
-        ConflictResolutionAction(ActionType::EntityNodeConflict, conflictingEntity, sourceAction, targetAction)
+        ConflictResolutionAction(conflictType, conflictingEntity, sourceAction, targetAction)
     {}
 };
 
@@ -369,10 +377,11 @@ class EntityKeyValueConflictResolutionAction :
     public ConflictResolutionAction
 {
 public:
-    EntityKeyValueConflictResolutionAction(const INodePtr& conflictingEntity, 
+    EntityKeyValueConflictResolutionAction(ConflictType conflictType,
+                                           const INodePtr& conflictingEntity,
                                            const MergeAction::Ptr& sourceAction, 
                                            const MergeAction::Ptr& targetAction) :
-        ConflictResolutionAction(ActionType::EntityKeyValueConflict, conflictingEntity, sourceAction, targetAction)
+        ConflictResolutionAction(conflictType, conflictingEntity, sourceAction, targetAction)
     {}
 };
 

libs/scene/merge/ThreeWayMergeOperation.cpp

@@ -79,23 +79,53 @@ std::list<ComparisonResult::KeyValueDifference>::const_iterator ThreeWayMergeOpe
     });
 }
 
-bool ThreeWayMergeOperation::KeyValueDiffHasConflicts(const ComparisonResult::KeyValueDifference& sourceKeyValueDiff, 
+ConflictType ThreeWayMergeOperation::GetKeyValueConflictType(const ComparisonResult::KeyValueDifference& sourceKeyValueDiff,
     const ComparisonResult::KeyValueDifference& targetKeyValueDiff)
 {
     assert(string::iequals(targetKeyValueDiff.key, sourceKeyValueDiff.key));
 
     // Key is matching, there's still a chance that this is not a conflict
     switch (targetKeyValueDiff.type)
     {
-    // If both are removing the key, that's fine
     case ComparisonResult::KeyValueDifference::Type::KeyValueRemoved:
-        return targetKeyValueDiff.type != sourceKeyValueDiff.type;
+    {
+        // Target had the key removed
+        if (sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueAdded)
+        {
+            throw std::logic_error("Source cannot add a key that has been removed in target, as it was present in base.");
+        }
+
+        // If both are removing the key, that's fine, otherwise it's a conflict
+        return sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueChanged ?
+            ConflictType::ModificationOfRemovedKeyValue : ConflictType::NoConflict;
+    }
 
     // On key value change or add, the value must be the same to not conflict
     case ComparisonResult::KeyValueDifference::Type::KeyValueAdded:
+    {
+        if (sourceKeyValueDiff.type != ComparisonResult::KeyValueDifference::Type::KeyValueAdded)
+        {
+            throw std::logic_error("Source cannot remove or modify a key that has been added in target, as it was present in base.");
+        }
+
+        // Value must match
+        return sourceKeyValueDiff.value != targetKeyValueDiff.value ? ConflictType::SettingKeyToDifferentValue : ConflictType::NoConflict;
+    }
     case ComparisonResult::KeyValueDifference::Type::KeyValueChanged:
-        return sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueRemoved ||
-            sourceKeyValueDiff.value != targetKeyValueDiff.value;
+    {
+        if (sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueAdded)
+        {
+            throw std::logic_error("Source cannot add a key that has been modified in target, as it was present in base.");
+        }
+
+        if (sourceKeyValueDiff.type == ComparisonResult::KeyValueDifference::Type::KeyValueRemoved)
+        {
+            return ConflictType::RemovalOfModifiedKeyValue;
+        }
+
+        // Both maps changes this value, compare the strings
+        return sourceKeyValueDiff.value != targetKeyValueDiff.value ? ConflictType::SettingKeyToDifferentValue : ConflictType::NoConflict;
+    }
     }
 
     throw std::logic_error("Unhandled key value diff type in ThreeWayMergeOperation::KeyValueDiffHasConflicts");
@@ -117,7 +147,8 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
         // This is a conflicting change, the source modified it, the target removed it
         // When the user chooses to import the change, it will be an AddEntity action
         addAction(std::make_shared<EntityConflictResolutionAction>(
-            targetDiff.sourceNode,
+            ConflictType::ModificationOfRemovedEntity,
+            sourceDiff.sourceNode, // the conflicting entity (comes from the source map)
             std::make_shared<AddEntityAction>(sourceDiff.sourceNode, _targetRoot)
         ));
         return;
@@ -174,7 +205,9 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
         }
 
         // Check if this key change is conflicting with the target change
-        if (!KeyValueDiffHasConflicts(sourceKeyValueDiff, *targetKeyValueDiff))
+        auto conflictType = GetKeyValueConflictType(sourceKeyValueDiff, *targetKeyValueDiff);
+
+        if (conflictType == ConflictType::NoConflict)
         {
             // Accept this change
             addActionForKeyValueDiff(sourceKeyValueDiff, targetDiff.sourceNode);
@@ -183,6 +216,7 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
         {
             // Create a conflict resolution action for this key value change
             addAction(std::make_shared<EntityKeyValueConflictResolutionAction>(
+                conflictType,
                 targetDiff.sourceNode, // conflicting entity
                 createActionForKeyValueDiff(sourceKeyValueDiff, targetDiff.sourceNode), // conflicting source change 
                 createActionForKeyValueDiff(*targetKeyValueDiff, targetDiff.sourceNode) // conflicting target change
@@ -274,6 +308,7 @@ void ThreeWayMergeOperation::compareAndCreateActions()
 
             // Entity has been removed in source, but modified in target, this is a conflict
             addAction(std::make_shared<EntityConflictResolutionAction>(
+                ConflictType::RemovalOfModifiedEntity,
                 targetDiff->second->sourceNode, // conflicting entity
                 std::make_shared<RemoveEntityAction>(targetDiff->second->sourceNode) // conflicting change 
             ));

libs/scene/merge/ThreeWayMergeOperation.h

@@ -61,7 +61,7 @@ class ThreeWayMergeOperation :
     void processEntityModification(const ComparisonResult::EntityDifference& sourceDiff,
         const ComparisonResult::EntityDifference& targetDiff);
 
-    static bool KeyValueDiffHasConflicts(const ComparisonResult::KeyValueDifference& sourceKeyValueDiff,
+    static ConflictType GetKeyValueConflictType(const ComparisonResult::KeyValueDifference& sourceKeyValueDiff,
         const ComparisonResult::KeyValueDifference& targetKeyValueDiff);
     static std::list<ComparisonResult::KeyValueDifference>::const_iterator FindTargetDiffByKey(
         const std::list<ComparisonResult::KeyValueDifference>& targetKeyValueDiffs, const std::string& key);

test/MapMerging.cpp

@@ -2055,11 +2055,17 @@ TEST_F(ThreeWayMergeTest, SourceAndTargetAreTheSame)
 // - add all brush "4" & "5" to func_static_4
 // - add two new nodes with the same name and one in between ("4", "between_4_and_5" and "5")
 //   with links from n1->n2->n3->n4->nbetween_4_and_5->n5->n1. The position of n4 and n5 is different from the target.
+// - light_1 got a new spawnarg _color (CONFLICT)
+// - func_static_8 has been deleted (CONFLICT)
+// - func_static_6 changes its origin to 224 180 32
 // 
 // Target Map (threeway_merge_target_2.mapx):
 // - add all brush "1" & "2" to func_static_1
 // - add all brush "4" to func_static_4 (a subset of the source change)
 // - added two new nodes (4 & 5) that are extend the node chain n1->n2->n3->n4->n5->n1
+// - light_1 has been removed (CONFLICT)
+// - func_static_8 got a new spawnarg "changed_in_target" => "value" (CONFLICT)
+// - func_static_6 changes its origin to 224 200 32
 
 TEST_F(ThreeWayMergeTest, MergePrimitiveChangesOfSameEntity)
 {
@@ -2155,6 +2161,7 @@ TEST_F(ThreeWayMergeTest, MergeEntityNameCollisions)
         return Node_getEntity(action->getConflictingEntity())->getKeyValue("name") == "node_3";
     });
     EXPECT_TRUE(keyValueConflict);
+    EXPECT_EQ(keyValueConflict->getConflictType(), ConflictType::SettingKeyToDifferentValue);
     EXPECT_EQ(keyValueConflict->getTargetAction()->getType(), scene::merge::ActionType::AddKeyValue);
 
     EXPECT_EQ(std::dynamic_pointer_cast<AddEntityKeyValueAction>(keyValueConflict->getTargetAction())->getKey(), "target0");
@@ -2190,4 +2197,89 @@ TEST_F(ThreeWayMergeTest, MergeEntityNameCollisions)
     EXPECT_EQ(Node_getEntity(node_3)->getKeyValue("target0"), newNode4Name);
 }
 
+// The source map tries to remove an entity that has been modified in target (func_static_8)
+TEST_F(ThreeWayMergeTest, RemovalOfModifiedEntity)
+{
+    auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_2.mapx", "maps/threeway_merge_source_2.mapx");
+
+    // func_static_8 is still present in the target
+    EXPECT_TRUE(algorithm::getEntityByName(operation->getTargetRoot(), "func_static_8"));
+
+    auto entityConflict = findAction<EntityConflictResolutionAction>(operation,
+        [](const std::shared_ptr<EntityConflictResolutionAction>& action)
+    {
+        return Node_getEntity(action->getConflictingEntity())->getKeyValue("name") == "func_static_8";
+    });
+    EXPECT_TRUE(entityConflict) << "Didn't find the conflicting with subject func_static_8";
+    EXPECT_EQ(entityConflict->getConflictType(), ConflictType::RemovalOfModifiedEntity);
+
+    operation->applyActions();
+
+    // The entity func_static_8 is still present
+    EXPECT_TRUE(algorithm::getEntityByName(operation->getTargetRoot(), "func_static_8"));
+
+    // Now accept the change explicitly
+    entityConflict->setResolvedByUsingSource(true);
+    entityConflict->applyChanges();
+
+    // The entity func_static_8 has now been removed in target
+    EXPECT_FALSE(algorithm::getEntityByName(operation->getTargetRoot(), "func_static_8"));
+}
+
+// The source map tries to modify a spawnarg of the removed entity light_1
+TEST_F(ThreeWayMergeTest, ModificationOfRemovedEntity)
+{
+    auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_2.mapx", "maps/threeway_merge_source_2.mapx");
+
+    EXPECT_FALSE(algorithm::getEntityByName(operation->getTargetRoot(), "light_1"));
+
+    auto entityConflict = findAction<EntityConflictResolutionAction>(operation,
+        [](const std::shared_ptr<EntityConflictResolutionAction>& action)
+    {
+        return Node_getEntity(action->getConflictingEntity())->getKeyValue("name") == "light_1";
+    });
+    EXPECT_TRUE(entityConflict) << "Didn't find the conflicting with subject light_1";
+    EXPECT_EQ(entityConflict->getConflictType(), ConflictType::ModificationOfRemovedEntity);
+
+    operation->applyActions();
+
+    // The entity light_1 is still gone
+    EXPECT_FALSE(algorithm::getEntityByName(operation->getTargetRoot(), "light_1"));
+
+    // Now accept the change explicitly
+    entityConflict->setResolvedByUsingSource(true);
+    entityConflict->applyChanges();
+
+    // The entity light_1 has now been imported to target
+    EXPECT_TRUE(algorithm::getEntityByName(operation->getTargetRoot(), "light_1"));
+}
+
+TEST_F(ThreeWayMergeTest, SettingKeyValueToConflictingValues)
+{
+    auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_2.mapx", "maps/threeway_merge_source_2.mapx");
+
+    auto entity = algorithm::getEntityByName(operation->getTargetRoot(), "func_static_6");
+    EXPECT_EQ(Node_getEntity(entity)->getKeyValue("origin"), "224 200 32");
+
+    auto valueConflict = findAction<EntityKeyValueConflictResolutionAction>(operation,
+        [](const std::shared_ptr<EntityKeyValueConflictResolutionAction>& action)
+    {
+        return Node_getEntity(action->getConflictingEntity())->getKeyValue("name") == "func_static_6";
+    });
+    EXPECT_TRUE(valueConflict) << "Didn't find the conflicting with subject func_static_6";
+    EXPECT_EQ(valueConflict->getConflictType(), ConflictType::SettingKeyToDifferentValue);
+
+    operation->applyActions();
+
+    // The key value is still the same
+    EXPECT_EQ(Node_getEntity(entity)->getKeyValue("origin"), "224 200 32");
+
+    // Now accept the change explicitly
+    valueConflict->setResolvedByUsingSource(true);
+    valueConflict->applyChanges();
+
+    // The changed value has now been imported
+    EXPECT_EQ(Node_getEntity(entity)->getKeyValue("origin"), "224 180 32");
+}
+
 }