Skip to content

Commit

Permalink
#5643: Restructure the whole process into two phases.
Browse files Browse the repository at this point in the history 
Phase 1 will detect any name conflicts that will occur during merge and will adjust the entity names accordingly (using Namespace algorithms).
Phase 2 will create the actions necessary to bring the source changes into the target scene.
Start preparing the corresponding unit test.
  • Loading branch information
@codereader
codereader committed Jun 18, 2021
1 parent 0ab693c commit 440a55d
Show file tree
Hide file tree
Showing 6 changed files with 364 additions and 67 deletions.
152 changes: 106 additions & 46 deletions libs/scene/merge/ThreeWayMergeOperation.cpp
View file
@@ -1,14 +1,61 @@
#include "ThreeWayMergeOperation.h"

#include "itextstream.h"
#include "inamespace.h"
#include "NodeUtils.h"
#include "GraphComparer.h"

namespace scene
{

namespace merge
{

// Contains lookup tables needed during analysis of the two scenes
struct ThreeWayMergeOperation::ComparisonData
{
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;

ComparisonResult::Ptr baseToSource;
ComparisonResult::Ptr baseToTarget;

ComparisonData(const IMapRootNodePtr& baseRoot, const IMapRootNodePtr& sourceRoot, const IMapRootNodePtr& targetRoot)
{
baseToSource = GraphComparer::Compare(sourceRoot, baseRoot);
baseToTarget = GraphComparer::Compare(targetRoot, baseRoot);

// Create source and target entity diff dictionaries (by entity name)
for (auto it = baseToSource->differingEntities.begin(); it != baseToSource->differingEntities.end(); ++it)
{
sourceDifferences[it->entityName] = it;
}

for (auto it = baseToTarget->differingEntities.begin(); it != baseToTarget->differingEntities.end(); ++it)
{
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;
});
}

INodePtr findTargetEntityByName(const std::string& name) const
{
auto found = targetEntities.find(name);
return found != targetEntities.end() ? found->second : INodePtr();
}
};

ThreeWayMergeOperation::ThreeWayMergeOperation(const scene::IMapRootNodePtr& baseRoot,
const scene::IMapRootNodePtr& sourceRoot, const scene::IMapRootNodePtr& targetRoot) :
_baseRoot(baseRoot),
Expand Down Expand Up @@ -144,45 +191,24 @@ void ThreeWayMergeOperation::processEntityModification(const ComparisonResult::E
}
}

void ThreeWayMergeOperation::processEntityDifferences(const std::list<ComparisonResult::EntityDifference>& sourceDiffs,
const std::list<ComparisonResult::EntityDifference>& targetDiffs)
void ThreeWayMergeOperation::compareAndCreateActions()
{
// Create source and target entity diff dictionaries (by entity name)
for (auto it = sourceDiffs.begin(); it != sourceDiffs.end(); ++it)
{
_sourceDifferences[it->entityName] = it;
}

for (auto it = targetDiffs.begin(); it != targetDiffs.end(); ++it)
{
_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;
});
ComparisonData data(_baseRoot, _sourceRoot, _targetRoot);

// 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)
for (const auto& pair : data.sourceDifferences)
{
auto targetDiff = _targetDifferences.find(pair.first);
auto targetDiff = data.targetDifferences.find(pair.first);

if (targetDiff == _targetDifferences.end())
if (targetDiff == data.targetDifferences.end())
{
// Change is targeting an entity that has not been altered in the source map => accept
switch (pair.second->type)
{
case ComparisonResult::EntityDifference::Type::EntityMissingInSource:
{
auto entityToRemove = findTargetEntityByName(pair.first);
auto entityToRemove = data.findTargetEntityByName(pair.first);
assert(entityToRemove);
addAction(std::make_shared<RemoveEntityAction>(entityToRemove));
}
Expand All @@ -194,7 +220,7 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::list<Comparison

case ComparisonResult::EntityDifference::Type::EntityPresentButDifferent:
{
auto entityToModify = findTargetEntityByName(pair.first);
auto entityToModify = data.findTargetEntityByName(pair.first);
assert(entityToModify);

for (const auto& keyValueDiff : pair.second->differingKeyValues)
Expand Down Expand Up @@ -260,43 +286,77 @@ void ThreeWayMergeOperation::processEntityDifferences(const std::list<Comparison
break;
}
}

// Cleanup temporary data
_sourceDifferences.clear();
_targetDifferences.clear();
_targetEntities.clear();
}

ThreeWayMergeOperation::Ptr ThreeWayMergeOperation::CreateFromComparisonResults(
const ComparisonResult& baseToSource, const ComparisonResult& baseToTarget)
ThreeWayMergeOperation::Ptr ThreeWayMergeOperation::Create(const IMapRootNodePtr& baseRoot,
const IMapRootNodePtr& sourceRoot, const IMapRootNodePtr& targetRoot)
{
if (baseToSource.getBaseRootNode() != baseToTarget.getBaseRootNode())
if (baseRoot == sourceRoot || baseRoot == targetRoot || sourceRoot == targetRoot)
{
throw std::runtime_error("The base scene of the two comparison results must be the same");
throw std::runtime_error("None of the root nodes must be equal to any of the other two");
}

auto operation = std::make_shared<ThreeWayMergeOperation>(baseToSource.getBaseRootNode(),
baseToSource.getSourceRootNode(), baseToTarget.getSourceRootNode());
auto operation = std::make_shared<ThreeWayMergeOperation>(baseRoot, sourceRoot, targetRoot);

operation->processEntityDifferences(baseToSource.differingEntities, baseToTarget.differingEntities);
// Phase 1 is to detect any entity additions from the source to the target that might cause name conflicts
// After this pass some key values might have been changed
operation->adjustSourceEntitiesWithNameConflicts();

// Phase 2 will run another comparison of the graphs (since key values might have been modified)
operation->compareAndCreateActions();

return operation;
}

void ThreeWayMergeOperation::setMergeSelectionGroups(bool enabled)
void ThreeWayMergeOperation::adjustSourceEntitiesWithNameConflicts()
{
// TODO
ComparisonData data(_baseRoot, _sourceRoot, _targetRoot);

std::set<INodePtr> sourceEntitiesToBeRenamed;

// Check each entity difference from the base to the source map
for (const auto& pair : data.sourceDifferences)
{
if (pair.second->type != ComparisonResult::EntityDifference::Type::EntityMissingInBase)
{
continue; // we're only looking for additions
}

// Find an entity change in the target map that affects the same entity name
// These are the source entities we have to rename first
auto targetDiff = data.targetDifferences.find(pair.first);

if (targetDiff == data.targetDifferences.end() || targetDiff->second->type != pair.second->type)
{
continue; // no target diff or not a target addition
}

// There's a chance this target entity is exactly the same as in the source
if (targetDiff->second->sourceFingerprint == pair.second->sourceFingerprint)
{
continue; // The entity turns out to be the same
}

rMessage() << "Source entity needs to be renamed." << std::endl;
sourceEntitiesToBeRenamed.insert(pair.second->sourceNode);
}

rMessage() << "Got " << sourceEntitiesToBeRenamed.size() << " entities to be renamed in "
"the source before they can be imported in the target" << std::endl;

// Let the target namespace detect any conflicts and assign new names to the source nodes
// This might change a few key values across the source scene
_targetRoot->getNamespace()->ensureNoConflicts(_sourceRoot, sourceEntitiesToBeRenamed);
}

void ThreeWayMergeOperation::setMergeLayers(bool enabled)
void ThreeWayMergeOperation::setMergeSelectionGroups(bool enabled)
{
// TODO
}

INodePtr ThreeWayMergeOperation::findTargetEntityByName(const std::string& name)
void ThreeWayMergeOperation::setMergeLayers(bool enabled)
{
auto found = _targetEntities.find(name);
return found != _targetEntities.end() ? found->second : INodePtr();
// TODO
}

}
Expand Down
20 changes: 8 additions & 12 deletions libs/scene/merge/ThreeWayMergeOperation.h
View file
Expand Up @@ -26,11 +26,8 @@ class ThreeWayMergeOperation :
IMapRootNodePtr _sourceRoot; // the map to be merged
IMapRootNodePtr _targetRoot; // the map where elements are going to be merged into

private:
// 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;
// Volatile data only needed during analysis
struct ComparisonData;

public:
using Ptr = std::shared_ptr<ThreeWayMergeOperation>;
Expand All @@ -40,9 +37,9 @@ class ThreeWayMergeOperation :

virtual ~ThreeWayMergeOperation();

// Creates the merge operation from the given comparison results.
// The operation will apply the missing changes in the source map to the target map
static Ptr CreateFromComparisonResults(const ComparisonResult& baseToSource, const ComparisonResult& baseToTarget);
// Creates the merge operation from the given root nodes. The base root is the common ancestor of source and target,
// and none of the three must be equal to any of them.
static Ptr Create(const IMapRootNodePtr& baseRoot, const IMapRootNodePtr& sourceRoot, const IMapRootNodePtr& targetRoot);

void setMergeSelectionGroups(bool enabled) override;
void setMergeLayers(bool enabled) override;
Expand All @@ -58,10 +55,9 @@ class ThreeWayMergeOperation :
}

private:
INodePtr findTargetEntityByName(const std::string& name);

void processEntityDifferences(const std::list<ComparisonResult::EntityDifference>& sourceDiffs,
const std::list<ComparisonResult::EntityDifference>& targetDiffs);
void adjustSourceEntitiesWithNameConflicts();

void compareAndCreateActions();
void processEntityModification(const ComparisonResult::EntityDifference& sourceDiff,
const ComparisonResult::EntityDifference& targetDiff);

Expand Down
54 changes: 50 additions & 4 deletions test/MapMerging.cpp
View file
Expand Up @@ -1793,10 +1793,7 @@ ThreeWayMergeOperation::Ptr setupThreeWayMergeOperation(const std::string& baseP
auto sourceResource = GlobalMapResourceManager().createFromPath(sourcePath);
EXPECT_TRUE(sourceResource->load()) << "Test map not found: " << sourcePath;

auto baseToSource = GraphComparer::Compare(sourceResource->getRootNode(), baseResource->getRootNode());
auto baseToTarget = GraphComparer::Compare(targetResource->getRootNode(), baseResource->getRootNode());

return ThreeWayMergeOperation::CreateFromComparisonResults(*baseToSource, *baseToTarget);
return ThreeWayMergeOperation::Create(baseResource->getRootNode(), sourceResource->getRootNode(), targetResource->getRootNode());
}

// Asserts that the changes to the target map have not been reverted
Expand Down Expand Up @@ -2056,10 +2053,13 @@ TEST_F(ThreeWayMergeTest, SourceAndTargetAreTheSame)
// Source (threeway_merge_source_2.mapx):
// - add all brush "2" to func_static_1 (a subset of the target change)
// - 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.
//
// 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

TEST_F(ThreeWayMergeTest, MergePrimitiveChangesOfSameEntity)
{
Expand Down Expand Up @@ -2097,4 +2097,50 @@ TEST_F(ThreeWayMergeTest, MergePrimitiveChangesOfSameEntity)
EXPECT_EQ(algorithm::getChildCount(worldspawn, algorithm::brushHasMaterial("textures/numbers/5")), 0); // removed from worldspawn
}

// Source:
// - 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.
// Target:
// - added two new nodes (4 & 5) that are extend the node chain n1->n2->n3->n4->n5->n1
TEST_F(ThreeWayMergeTest, MergeEntityNameCollisions)
{
auto operation = setupThreeWayMergeOperation("maps/threeway_merge_base.mapx", "maps/threeway_merge_target_2.mapx", "maps/threeway_merge_source_2.mapx");

// The expected result is that the three nodes added in the source have their names
// changed to not conflict with the target map and keep their links intact after import.
// The nodes n4 and n5 in the target should be preserved including their links.

// TODO: Entity names should not conflict before calculating the diff
// TODO: Changed entity names might cause other spawnargs to change in the source map
// TODO: These key value changes need to be merged too, they'd cause a conflict here
#if 0
auto func_static_1 = algorithm::getEntityByName(operation->getTargetRoot(), "func_static_1");
auto func_static_4 = algorithm::getEntityByName(operation->getTargetRoot(), "func_static_4");
auto worldspawn = algorithm::findWorldspawn(operation->getTargetRoot());

// brush_1 should get no actions, since they are already below func_static_1
auto brush1ActionCount = countActions<MergeAction>(operation, [](const std::shared_ptr<MergeAction>& action)
{
return algorithm::brushHasMaterial("textures/numbers/1")(action->getAffectedNode());
});
auto brush5ActionCount = countActions<MergeAction>(operation, [](const std::shared_ptr<MergeAction>& action)
{
return algorithm::brushHasMaterial("textures/numbers/5")(action->getAffectedNode());
});

EXPECT_EQ(brush1ActionCount, 0) << "Brush 1 should not take any changes";
EXPECT_EQ(brush5ActionCount, 4) << "Brush 5 should be 2x removed from worldspawn and 2x added to func_static_4";

EXPECT_EQ(algorithm::getChildCount(func_static_1, algorithm::brushHasMaterial("textures/numbers/1")), 4);
EXPECT_EQ(algorithm::getChildCount(func_static_4, algorithm::brushHasMaterial("textures/numbers/5")), 0);
EXPECT_EQ(algorithm::getChildCount(worldspawn, algorithm::brushHasMaterial("textures/numbers/5")), 2);

operation->applyActions();

EXPECT_EQ(algorithm::getChildCount(func_static_1, algorithm::brushHasMaterial("textures/numbers/1")), 4); // no change here
EXPECT_EQ(algorithm::getChildCount(func_static_4, algorithm::brushHasMaterial("textures/numbers/5")), 2); // added to func_static_4
EXPECT_EQ(algorithm::getChildCount(worldspawn, algorithm::brushHasMaterial("textures/numbers/5")), 0); // removed from worldspawn
#endif
}

}

0 comments on commit 440a55d

Please sign in to comment.