Merge pull request #4190 from vcmi/fix_replacing_artifacts

Draw random object to replace as an atomic operation

lib/mapping/CMapEditManager.cpp

@@ -38,6 +38,7 @@ void CMapUndoManager::redo()
 
 void CMapUndoManager::clearAll()
 {
+	//FIXME: Will crash if an object was added twice to actions
 	undoStack.clear();
 	redoStack.clear();
 	onUndoRedo();

lib/rmg/modificators/QuestArtifactPlacer.cpp

@@ -64,6 +64,22 @@ std::vector<CGObjectInstance*> QuestArtifactPlacer::getPossibleArtifactsToReplac
 	return artifactsToReplace;
 }
 
+CGObjectInstance * QuestArtifactPlacer::drawObjectToReplace()
+{
+	RecursiveLock lock(externalAccessMutex);
+
+	if (artifactsToReplace.empty())
+	{
+		return nullptr;
+	}
+	else
+	{
+		auto ret = *RandomGeneratorUtil::nextItem(artifactsToReplace, zone.getRand());
+		vstd::erase_if_present(artifactsToReplace, ret);
+		return ret;
+	}
+}
+
 void QuestArtifactPlacer::findZonesForQuestArts()
 {
 	const auto& distances = generator.getZonePlacer()->getDistanceMap().at(zone.getId());
@@ -87,14 +103,14 @@ void QuestArtifactPlacer::placeQuestArtifacts(CRandomGenerator & rand)
 		for (auto zone : questArtZones)
 		{
 			auto* qap = zone->getModificator<QuestArtifactPlacer>();
-			std::vector<CGObjectInstance *> artifactsToReplace = qap->getPossibleArtifactsToReplace();
-			if (artifactsToReplace.empty())
+			
+			auto objectToReplace = qap->drawObjectToReplace();
+			if (!objectToReplace)
 				continue;
 
-			auto artifactToReplace = *RandomGeneratorUtil::nextItem(artifactsToReplace, rand);
 			logGlobal->trace("Replacing %s at %s with the quest artifact %s",
-				artifactToReplace->getObjectName(),
-				artifactToReplace->getPosition().toString(),
+				objectToReplace->getObjectName(),
+				objectToReplace->getPosition().toString(),
 				VLC->artifacts()->getById(artifactToPlace)->getNameTranslated());
 
 			//Update appearance. Terrain is irrelevant.
@@ -103,24 +119,15 @@ void QuestArtifactPlacer::placeQuestArtifacts(CRandomGenerator & rand)
 			auto templates = handler->getTemplates();
 			//artifactToReplace->appearance = templates.front();
 			newObj->appearance  = templates.front();
-			newObj->pos = artifactToReplace->pos;
+			newObj->pos = objectToReplace->pos;
 			mapProxy->insertObject(newObj);
-
-			for (auto z : map.getZones())
-			{
-				//Every qap has its OWN collection of artifacts
-				auto * localQap = zone->getModificator<QuestArtifactPlacer>();
-				if (localQap)
-				{
-					localQap->dropReplacedArtifact(artifactToReplace);
-				}
-			}
-			mapProxy->removeObject(artifactToReplace);
+			mapProxy->removeObject(objectToReplace);
 			break;
 		}
 	}
 }
 
+// TODO: Unused?
 void QuestArtifactPlacer::dropReplacedArtifact(CGObjectInstance* obj)
 {
 	RecursiveLock lock(externalAccessMutex);

lib/rmg/modificators/QuestArtifactPlacer.h

@@ -31,6 +31,7 @@ public:
 	void addQuestArtifact(const ArtifactID& id);
 	void removeQuestArtifact(const ArtifactID& id);
 	void rememberPotentialArtifactToReplace(CGObjectInstance* obj);
+	CGObjectInstance * drawObjectToReplace();
 	std::vector<CGObjectInstance*> getPossibleArtifactsToReplace() const;
 	void placeQuestArtifacts(CRandomGenerator & rand);
 	void dropReplacedArtifact(CGObjectInstance* obj);