From 3efa9b836c138e30fa11515acc7f1201a6fd1be5 Mon Sep 17 00:00:00 2001 From: codereader Date: Tue, 15 Jun 2021 21:09:46 +0200 Subject: [PATCH] #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 | 8 ++++++++ libs/scene/merge/GraphComparer.cpp | 18 ++++++++++++------ libs/scene/merge/ThreeWayMergeOperation.cpp | 17 +++++++++++++---- test/MapMerging.cpp | 20 +++++++++++++++++--- 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/libs/scene/merge/ComparisonResult.h b/libs/scene/merge/ComparisonResult.h index e1e84bc4b4..70652aebd7 100644 --- a/libs/scene/merge/ComparisonResult.h +++ b/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 { diff --git a/libs/scene/merge/GraphComparer.cpp b/libs/scene/merge/GraphComparer.cpp index c6f55b4717..7434b2e7d9 100644 --- a/libs/scene/merge/GraphComparer.cpp +++ b/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 }); } diff --git a/libs/scene/merge/ThreeWayMergeOperation.cpp b/libs/scene/merge/ThreeWayMergeOperation.cpp index 6e8dd00048..01fe54c3a4 100644 --- a/libs/scene/merge/ThreeWayMergeOperation.cpp +++ b/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(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(pair.second->sourceNode, _targetRoot)); + } break; case ComparisonResult::EntityDifference::Type::EntityMissingInSource: // entity was removed in source diff --git a/test/MapMerging.cpp b/test/MapMerging.cpp index 88e0149b5d..f49a64b156 100644 --- a/test/MapMerging.cpp +++ b/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(operation, [&](const IMergeAction::Ptr&) { return true; }); + + EXPECT_EQ(actionCount, 0); +} + }