Instead of re-writing groups on the fly when inserting into the targe…

…t scene, the new approach is to remove any conflicts before starting to migrate the nodes. This is similar to the approach taken when removing the name conflicts (NamespaceManager::ensureNoConflicts).

radiant/map/algorithm/Import.cpp

@@ -1,6 +1,7 @@
 #include "Import.h"
 
 #include <map>
+#include <limits>
 
 #include "imap.h"
 #include "imapformat.h"
@@ -23,68 +24,71 @@ namespace map
 namespace algorithm
 {
 
-// Will map source group IDs to new groups created in the target map
+// Will rewrite the group memberships of visited nodes to not be 
+// in conflict with any of the groups present in the target scene
 class SelectionGroupRemapper :
-	private std::map<std::size_t, selection::ISelectionGroupPtr>
+	public scene::NodeVisitor
 {
 private:
+	// The group manager of the target scene
 	selection::ISelectionGroupManager& _targetGroupManager;
 
+	// Maps old group IDs to new selection groups
+	std::map<std::size_t, selection::ISelectionGroupPtr> _groupMap;
+
+	std::size_t _nextNewGroupId;
+
 public:
-	// The given groupManager will be used to create one corresponding group
-	// for each distinct group found in the source nodes
 	SelectionGroupRemapper(selection::ISelectionGroupManager& targetGroupManager) :
-		_targetGroupManager(targetGroupManager)
+		_targetGroupManager(targetGroupManager),
+		_nextNewGroupId(0)
 	{}
 
-	void assignRemappedGroups(const scene::INodePtr& node, const IGroupSelectable::GroupIds& oldGroupIds)
-	{
-		rMessage() << "Node " << node->name() << " had the groups " << string::join(oldGroupIds, "|");
-
-		// Get the Groups the source node was assigned to, and add the
-		// cloned node to the mapped group, one by one, keeping the order intact
-		for (std::size_t id : oldGroupIds)
-		{
-			// Try to insert the ID, ignore if already exists
-			// Get a new mapping for the given group ID
-			const selection::ISelectionGroupPtr& mappedGroup = getMappedGroup(id);
-
-			// Assign the new group ID to this clone
-			mappedGroup->addNode(node);
-		}
-
-		rMessage() << " => " << string::join(std::dynamic_pointer_cast<IGroupSelectable>(node)->getGroupIds(), "|") << std::endl;
-	}
-
-	void remapSelectionGroups(const scene::INodePtr& node)
+	bool pre(const scene::INodePtr& node) override
 	{
-		std::shared_ptr<IGroupSelectable> groupSelectable = std::dynamic_pointer_cast<IGroupSelectable>(node);
+		// Check if this node is a group selectable in the first place
+		auto groupSelectable = std::dynamic_pointer_cast<IGroupSelectable>(node);
 
 		if (groupSelectable)
 		{
-			auto sourceRoot = node->getRootNode();
-			assert(sourceRoot);
+			assert(node->getRootNode());
+
+			auto& sourceGroupManager = node->getRootNode()->getSelectionGroupManager();
 
-			// Save the current set of group IDs
+			// Copy the current set of group IDs locally
 			IGroupSelectable::GroupIds oldGroupIds = groupSelectable->getGroupIds();
 
-			// Remove the node from all its groups
+			// Remove the node from all its groups in the source space before continuing
 			for (auto id : oldGroupIds)
 			{
-				auto group = sourceRoot->getSelectionGroupManager().getSelectionGroup(id);
+				auto group = sourceGroupManager.getSelectionGroup(id);
 
 				group->removeNode(node);
 			}
 
 			// Assign the new set of groups for this node
-			assignRemappedGroups(node, oldGroupIds);
+			for (auto id : oldGroupIds)
+			{
+				// Only do a remap if the group ID is in use in the target space
+				auto group = _targetGroupManager.getSelectionGroup(id) ?
+					getMappedGroup(id, sourceGroupManager) : sourceGroupManager.getSelectionGroup(id);
+
+				group->addNode(node);
+			}
+
+#ifndef NDEBUG
+			rMessage() << "Node " << node->name() << " had the groups " << string::join(oldGroupIds, "|");
+			rMessage() << " remapped to " << string::join(groupSelectable->getGroupIds(), "|") << std::endl;
+#endif
 		}
+
+		return true;
 	}
 
 private:
-	const selection::ISelectionGroupPtr& getMappedGroup(std::size_t id)
+	const selection::ISelectionGroupPtr& getMappedGroup(std::size_t idToRemap, selection::ISelectionGroupManager& sourceGroupManager)
 	{
-		auto found = emplace(id, selection::ISelectionGroupPtr());
+		auto found = _groupMap.emplace(idToRemap, selection::ISelectionGroupPtr());
 
 		if (!found.second)
 		{
@@ -93,32 +97,42 @@ class SelectionGroupRemapper :
 		}
 
 		// Insertion was successful, so we didn't cover this ID yet
-		found.first->second = _targetGroupManager.createSelectionGroup();
+		// Find a new ID that is not in known to the target manager
+		auto newGroupId = generateNonConflictingGroupId();
+
+		// Create this group in the source space
+		found.first->second = sourceGroupManager.findOrCreateSelectionGroup(newGroupId);
 
 		return found.first->second;
 	}
+
+	std::size_t generateNonConflictingGroupId()
+	{
+		while (++_nextNewGroupId < std::numeric_limits<std::size_t>::max())
+		{
+			if (!_targetGroupManager.getSelectionGroup(_nextNewGroupId))
+			{
+				return _nextNewGroupId;
+			}
+		}
+
+		throw std::runtime_error("Out of group IDs.");
+	}
 };
 
 class PrimitiveMerger :
 	public scene::PrimitiveReparentor
 {
-private:
-	SelectionGroupRemapper _groupRemapper;
-
 public:
-	PrimitiveMerger(const scene::INodePtr& newParent, SelectionGroupRemapper& remapper) :
-		PrimitiveReparentor(newParent),
-		_groupRemapper(remapper)
+	PrimitiveMerger(const scene::INodePtr& newParent) :
+		PrimitiveReparentor(newParent)
 	{}
 
 	void post(const scene::INodePtr& node) override
 	{
 		// Base class is doing the reparenting
 		PrimitiveReparentor::post(node);
 
-		// Re-generate the group IDs of this node
-		_groupRemapper.remapSelectionGroups(node);
-
 		// After reparenting, highlight the imported node
 		Node_setSelected(node, true);
 	}
@@ -131,12 +145,9 @@ class EntityMerger :
 	// The target path
 	mutable scene::Path _path;
 
-	SelectionGroupRemapper _groupRemapper;
-
 public:
 	EntityMerger(const scene::INodePtr& root) :
-		_path(scene::Path(root)),
-		_groupRemapper(root->getRootNode()->getSelectionGroupManager())
+		_path(scene::Path(root))
 	{}
 
 	bool pre(const scene::INodePtr& originalNode) override
@@ -181,7 +192,7 @@ class EntityMerger :
 				_path.push(worldSpawn);
 
 				// Move all children of this node to the target worldspawn
-				PrimitiveMerger visitor(worldSpawn, _groupRemapper);
+				PrimitiveMerger visitor(worldSpawn);
 				node->traverseChildren(visitor);
 			}
 		}
@@ -211,9 +222,6 @@ class EntityMerger :
 
 	void post(const scene::INodePtr& node) override
 	{
-		// Re-generate the group IDs of this entity node
-		_groupRemapper.remapSelectionGroups(node);
-
 		_path.pop();
 	}
 };
@@ -230,6 +238,11 @@ void mergeMap(const scene::INodePtr& node)
 		node->traverse(walker);
 	}
 
+	// Rewrite the incoming group IDs to not be in conflict with the target scene
+	SelectionGroupRemapper remapper(GlobalSceneGraph().root()->getSelectionGroupManager());
+	node->traverseChildren(remapper);
+
+	// Now move everything into the target map
 	EntityMerger merger(GlobalSceneGraph().root());
 	node->traverseChildren(merger);
 }