#5643: Handle primitive removals and addition targeting the same entity

codereader · Jun 15, 2021 · 3a86bd8 · 3a86bd8
1 parent 10d3b0a
commit 3a86bd8
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 59 deletions.
diff --git a/libs/scene/merge/GraphComparer.cpp b/libs/scene/merge/GraphComparer.cpp
@@ -22,8 +22,8 @@ ComparisonResult::Ptr GraphComparer::Compare(const IMapRootNodePtr& source, cons
 {
     auto result = std::make_shared<ComparisonResult>(source, base);
 
-    auto sourceEntities = collectEntityFingerprints(source);
-    auto baseEntities = collectEntityFingerprints(base);
+    auto sourceEntities = NodeUtils::CollectEntityFingerprints(source);
+    auto baseEntities = NodeUtils::CollectEntityFingerprints(base);
 
     // Filter out all the matching nodes and store them in the result
     if (sourceEntities.empty())
@@ -227,8 +227,8 @@ std::list<ComparisonResult::PrimitiveDifference> GraphComparer::compareChildNode
 {
     std::list<ComparisonResult::PrimitiveDifference> result;
 
-    auto sourceChildren = collectPrimitiveFingerprints(sourceNode);
-    auto baseChildren = collectPrimitiveFingerprints(baseNode);
+    auto sourceChildren = NodeUtils::CollectPrimitiveFingerprints(sourceNode);
+    auto baseChildren = NodeUtils::CollectPrimitiveFingerprints(baseNode);
 
     std::vector<Fingerprints::value_type> missingInSource;
     std::vector<Fingerprints::value_type> missingInBase;
@@ -266,50 +266,6 @@ std::list<ComparisonResult::PrimitiveDifference> GraphComparer::compareChildNode
     return result;
 }
 
-GraphComparer::Fingerprints GraphComparer::collectNodeFingerprints(const INodePtr& parent, 
-    const std::function<bool(const INodePtr& node)>& nodePredicate)
-{
-    Fingerprints result;
-
-    parent->foreachNode([&](const INodePtr& node)
-    {
-        if (!nodePredicate(node)) return true; // predicate says "skip"
-
-        auto comparable = std::dynamic_pointer_cast<IComparableNode>(node);
-        assert(comparable);
-
-        if (!comparable) return true; // skip
-
-        // Store the fingerprint and check for collisions
-        auto insertResult = result.try_emplace(comparable->getFingerprint(), node);
-
-        if (!insertResult.second)
-        {
-            rWarning() << "More than one node with the same fingerprint found in the parent node with name " << parent->name() << std::endl;
-        }
-
-        return true;
-    });
-
-    return result;
-}
-
-GraphComparer::Fingerprints GraphComparer::collectPrimitiveFingerprints(const INodePtr& parent)
-{
-    return collectNodeFingerprints(parent, [](const INodePtr& node)
-    {
-        return node->getNodeType() == INode::Type::Brush || node->getNodeType() == INode::Type::Patch;
-    });
-}
-
-GraphComparer::Fingerprints GraphComparer::collectEntityFingerprints(const INodePtr& root)
-{
-    return collectNodeFingerprints(root, [](const INodePtr& node)
-    {
-        return node->getNodeType() == INode::Type::Entity;
-    });
-}
-
 void GraphComparer::compareSelectionGroups(ComparisonResult& result)
 {
     // Compare all matching entities first, their primitives are matching
@@ -340,8 +296,8 @@ void GraphComparer::compareSelectionGroups(ComparisonResult& result)
 void GraphComparer::compareSelectionGroupsOfPrimitives(ComparisonResult& result, const INodePtr& sourceNode, const INodePtr& baseNode)
 {
     // Check each node of the mismatching source entity, it might have counter-parts in the base map
-    auto sourcePrimitives = collectPrimitiveFingerprints(sourceNode);
-    auto basePrimitives = collectPrimitiveFingerprints(baseNode);
+    auto sourcePrimitives = NodeUtils::CollectPrimitiveFingerprints(sourceNode);
+    auto basePrimitives = NodeUtils::CollectPrimitiveFingerprints(baseNode);
 
     for (const auto& pair : sourcePrimitives)
     {

diff --git a/libs/scene/merge/GraphComparer.h b/libs/scene/merge/GraphComparer.h
@@ -45,12 +45,6 @@ class GraphComparer
     static void processDifferingEntities(ComparisonResult& result, const EntityMismatchByName& sourceMismatches, 
         const EntityMismatchByName& baseMismatches);
 
-    static Fingerprints collectEntityFingerprints(const INodePtr& root);
-    static Fingerprints collectPrimitiveFingerprints(const INodePtr& parent);
-
-    static Fingerprints collectNodeFingerprints(const INodePtr& parent,
-        const std::function<bool(const INodePtr& node)>& nodePredicate);
-
     static std::list<ComparisonResult::KeyValueDifference> compareKeyValues(
         const INodePtr& sourceNode, const INodePtr& baseNode);
 

diff --git a/libs/scene/merge/NodeUtils.h b/libs/scene/merge/NodeUtils.h
@@ -1,15 +1,19 @@
 #pragma once
 
+#include <map>
 #include "inode.h"
 #include "icomparablenode.h"
 #include "ientity.h"
+#include "itextstream.h"
 
 namespace scene
 {
 
 namespace merge
 {
 
+using Fingerprints = std::map<std::string, INodePtr>;
+
 class NodeUtils
 {
 public:
@@ -31,7 +35,51 @@ class NodeUtils
         return GetEntityNameOrFingerprint(member);
     }
 
+    static Fingerprints CollectEntityFingerprints(const INodePtr& root)
+    {
+        return CollectNodeFingerprints(root, [](const INodePtr& node)
+        {
+            return node->getNodeType() == INode::Type::Entity;
+        });
+    }
+
+    static Fingerprints CollectPrimitiveFingerprints(const INodePtr& parent)
+    {
+        return CollectNodeFingerprints(parent, [](const INodePtr& node)
+        {
+            return node->getNodeType() == INode::Type::Brush || node->getNodeType() == INode::Type::Patch;
+        });
+    }
+
 private:
+    static Fingerprints CollectNodeFingerprints(const INodePtr& parent,
+        const std::function<bool(const INodePtr& node)>& nodePredicate)
+    {
+        Fingerprints result;
+
+        parent->foreachNode([&](const INodePtr& node)
+        {
+            if (!nodePredicate(node)) return true; // predicate says "skip"
+
+            auto comparable = std::dynamic_pointer_cast<IComparableNode>(node);
+            assert(comparable);
+
+            if (!comparable) return true; // skip
+
+            // Store the fingerprint and check for collisions
+            auto insertResult = result.try_emplace(comparable->getFingerprint(), node);
+
+            if (!insertResult.second)
+            {
+                rWarning() << "More than one node with the same fingerprint found in the parent node with name " << parent->name() << std::endl;
+            }
+
+            return true;
+        });
+
+        return result;
+    }
+
     static std::string GetEntityNameOrFingerprint(const INodePtr& member)
     {
         if (member->getNodeType() == INode::Type::Entity)

diff --git a/libs/scene/merge/ThreeWayMergeOperation.cpp b/libs/scene/merge/ThreeWayMergeOperation.cpp
@@ -1,6 +1,7 @@
 #include "ThreeWayMergeOperation.h"
 
 #include "itextstream.h"
+#include "NodeUtils.h"
 
 namespace scene
 {
@@ -61,18 +62,43 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
     if (targetDiff.type == ComparisonResult::EntityDifference::Type::EntityMissingInSource)
     {
         // This is a conflicting change, the source modified it, the target removed it
-        // TODO: Add a conflict resolution action
+        // When the user chooses to import the change, it will be an AddEntity action
+        addAction(std::make_shared<EntityConflictResolutionAction>(
+            targetDiff.sourceNode,
+            std::make_shared<AddEntityAction>(sourceDiff.sourceNode, _targetRoot)
+        ));
         return;
     }
 
     // Both graphs modified this entity, do an in-depth comparison
+    auto targetChildren = NodeUtils::CollectPrimitiveFingerprints(targetDiff.sourceNode);
 
     // Every primitive change that has been done to the target map can be applied
     // to the source map, since we can't detect whether one of them has been moved or retextured
     for (const auto& primitiveDiff : sourceDiff.differingChildren)
     {
-        // TODO: Don't create duplicates though
-        addActionsForPrimitiveDiff(primitiveDiff, targetDiff.sourceNode);
+        bool primitivePresentInTargetMap = targetChildren.count(primitiveDiff.fingerprint) != 0;
+
+        switch (primitiveDiff.type)
+        {
+        case ComparisonResult::PrimitiveDifference::Type::PrimitiveAdded:
+        {
+            // Add this primitive if it isn't there already
+            if (!primitivePresentInTargetMap)
+            {
+                addAction(std::make_shared<AddChildAction>(primitiveDiff.node, targetDiff.sourceNode));
+            }
+            break;
+        }
+
+        case ComparisonResult::PrimitiveDifference::Type::PrimitiveRemoved:
+            // Check if this primitive is still present in the target map, otherwise we can't remove it
+            if (primitivePresentInTargetMap)
+            {
+                addAction(std::make_shared<RemoveChildAction>(targetChildren[primitiveDiff.fingerprint]));
+            }
+            break;
+        }
     }
 
     // The key value changes can be applied only if they are not targeting the same key