Merge pull request #4482 from MichalZr6/mapeditor_fixes

Mapeditor refactor, fixes and additional validation checks

lib/mapObjectConstructors/CObjectClassesHandler.cpp

@@ -120,7 +120,7 @@ std::vector<JsonNode> CObjectClassesHandler::loadLegacyData()
 		legacyTemplates.insert(std::make_pair(key, tmpl));
 	}
 
-	objects.resize(256);
+	mapObjectTypes.resize(256);
 
 	std::vector<JsonNode> ret(dataSize);// create storage for 256 objects
 	assert(dataSize == 256);
@@ -162,39 +162,39 @@ std::vector<JsonNode> CObjectClassesHandler::loadLegacyData()
 	return ret;
 }
 
-void CObjectClassesHandler::loadSubObject(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * obj)
+void CObjectClassesHandler::loadSubObject(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * baseObject)
 {
-	auto object = loadSubObjectFromJson(scope, identifier, entry, obj, obj->objects.size());
+	auto subObject = loadSubObjectFromJson(scope, identifier, entry, baseObject, baseObject->objectTypeHandlers.size());
 
-	assert(object);
-	obj->objects.push_back(object);
+	assert(subObject);
+	baseObject->objectTypeHandlers.push_back(subObject);
 
-	registerObject(scope, obj->getJsonKey(), object->getSubTypeName(), object->subtype);
+	registerObject(scope, baseObject->getJsonKey(), subObject->getSubTypeName(), subObject->subtype);
 	for(const auto & compatID : entry["compatibilityIdentifiers"].Vector())
-		registerObject(scope, obj->getJsonKey(), compatID.String(), object->subtype);
+		registerObject(scope, baseObject->getJsonKey(), compatID.String(), subObject->subtype);
 }
 
-void CObjectClassesHandler::loadSubObject(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * obj, size_t index)
+void CObjectClassesHandler::loadSubObject(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * baseObject, size_t index)
 {
-	auto object = loadSubObjectFromJson(scope, identifier, entry, obj, index);
+	auto subObject = loadSubObjectFromJson(scope, identifier, entry, baseObject, index);
 
-	assert(object);
-	if (obj->objects.at(index) != nullptr)
+	assert(subObject);
+	if (baseObject->objectTypeHandlers.at(index) != nullptr)
 		throw std::runtime_error("Attempt to load already loaded object:" + identifier);
 
-	obj->objects.at(index) = object;
+	baseObject->objectTypeHandlers.at(index) = subObject;
 
-	registerObject(scope, obj->getJsonKey(), object->getSubTypeName(), object->subtype);
+	registerObject(scope, baseObject->getJsonKey(), subObject->getSubTypeName(), subObject->subtype);
 	for(const auto & compatID : entry["compatibilityIdentifiers"].Vector())
-		registerObject(scope, obj->getJsonKey(), compatID.String(), object->subtype);
+		registerObject(scope, baseObject->getJsonKey(), compatID.String(), subObject->subtype);
 }
 
-TObjectTypeHandler CObjectClassesHandler::loadSubObjectFromJson(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * obj, size_t index)
+TObjectTypeHandler CObjectClassesHandler::loadSubObjectFromJson(const std::string & scope, const std::string & identifier, const JsonNode & entry, ObjectClass * baseObject, size_t index)
 {
 	assert(identifier.find(':') == std::string::npos);
 	assert(!scope.empty());
 
-	std::string handler = obj->handlerName;
+	std::string handler = baseObject->handlerName;
 	if(!handlerConstructors.count(handler))
 	{
 		logMod->error("Handler with name %s was not found!", handler);
@@ -206,10 +206,10 @@ TObjectTypeHandler CObjectClassesHandler::loadSubObjectFromJson(const std::strin
 	auto createdObject = handlerConstructors.at(handler)();
 
 	createdObject->modScope = scope;
-	createdObject->typeName = obj->identifier;
+	createdObject->typeName = baseObject->identifier;
 	createdObject->subTypeName = identifier;
 
-	createdObject->type = obj->id;
+	createdObject->type = baseObject->id;
 	createdObject->subtype = index;
 	createdObject->init(entry);
 
@@ -223,7 +223,7 @@ TObjectTypeHandler CObjectClassesHandler::loadSubObjectFromJson(const std::strin
 		}
 	}
 
-	auto range = legacyTemplates.equal_range(std::make_pair(obj->id, index));
+	auto range = legacyTemplates.equal_range(std::make_pair(baseObject->id, index));
 	for (auto & templ : boost::make_iterator_range(range.first, range.second))
 	{
 		if (staticObject)
@@ -238,7 +238,7 @@ TObjectTypeHandler CObjectClassesHandler::loadSubObjectFromJson(const std::strin
 	}
 	legacyTemplates.erase(range.first, range.second);
 
-	logGlobal->debug("Loaded object %s(%d)::%s(%d)", obj->getJsonKey(), obj->id, identifier, index);
+	logGlobal->debug("Loaded object %s(%d)::%s(%d)", baseObject->getJsonKey(), baseObject->id, identifier, index);
 
 	return createdObject;
 }
@@ -263,17 +263,17 @@ std::string ObjectClass::getNameTranslated() const
 
 std::unique_ptr<ObjectClass> CObjectClassesHandler::loadFromJson(const std::string & scope, const JsonNode & json, const std::string & name, size_t index)
 {
-	auto obj = std::make_unique<ObjectClass>();
+	auto newObject = std::make_unique<ObjectClass>();
 
-	obj->modScope = scope;
-	obj->identifier = name;
-	obj->handlerName = json["handler"].String();
-	obj->base = json["base"];
-	obj->id = index;
+	newObject->modScope = scope;
+	newObject->identifier = name;
+	newObject->handlerName = json["handler"].String();
+	newObject->base = json["base"];
+	newObject->id = index;
 
-	VLC->generaltexth->registerString(scope, obj->getNameTextID(), json["name"].String());
+	VLC->generaltexth->registerString(scope, newObject->getNameTextID(), json["name"].String());
 
-	obj->objects.resize(json["lastReservedIndex"].Float() + 1);
+	newObject->objectTypeHandlers.resize(json["lastReservedIndex"].Float() + 1);
 
 	for (auto subData : json["types"].Struct())
 	{
@@ -284,68 +284,71 @@ std::unique_ptr<ObjectClass> CObjectClassesHandler::loadFromJson(const std::stri
 			if ( subMeta == "core")
 			{
 				size_t subIndex = subData.second["index"].Integer();
-				loadSubObject(subData.second.getModScope(), subData.first, subData.second, obj.get(), subIndex);
+				loadSubObject(subData.second.getModScope(), subData.first, subData.second, newObject.get(), subIndex);
 			}
 			else
 			{
 				logMod->error("Object %s:%s.%s - attempt to load object with preset index! This option is reserved for built-in mod", subMeta, name, subData.first );
-				loadSubObject(subData.second.getModScope(), subData.first, subData.second, obj.get());
+				loadSubObject(subData.second.getModScope(), subData.first, subData.second, newObject.get());
 			}
 		}
 		else
-			loadSubObject(subData.second.getModScope(), subData.first, subData.second, obj.get());
+			loadSubObject(subData.second.getModScope(), subData.first, subData.second, newObject.get());
 	}
 
-	if (obj->id == MapObjectID::MONOLITH_TWO_WAY)
-		generateExtraMonolithsForRMG(obj.get());
+	if (newObject->id == MapObjectID::MONOLITH_TWO_WAY)
+		generateExtraMonolithsForRMG(newObject.get());
 
-	return obj;
+	return newObject;
 }
 
 void CObjectClassesHandler::loadObject(std::string scope, std::string name, const JsonNode & data)
 {
-	objects.push_back(loadFromJson(scope, data, name, objects.size()));
+	mapObjectTypes.push_back(loadFromJson(scope, data, name, mapObjectTypes.size()));
 
-	VLC->identifiersHandler->registerObject(scope, "object", name, objects.back()->id);
+	VLC->identifiersHandler->registerObject(scope, "object", name, mapObjectTypes.back()->id);
 }
 
 void CObjectClassesHandler::loadObject(std::string scope, std::string name, const JsonNode & data, size_t index)
 {
-	assert(objects.at(index) == nullptr); // ensure that this id was not loaded before
+	assert(mapObjectTypes.at(index) == nullptr); // ensure that this id was not loaded before
 
-	objects.at(index) = loadFromJson(scope, data, name, index);
-	VLC->identifiersHandler->registerObject(scope, "object", name, objects.at(index)->id);
+	mapObjectTypes.at(index) = loadFromJson(scope, data, name, index);
+	VLC->identifiersHandler->registerObject(scope, "object", name, mapObjectTypes.at(index)->id);
 }
 
 void CObjectClassesHandler::loadSubObject(const std::string & identifier, JsonNode config, MapObjectID ID, MapObjectSubID subID)
 {
 	config.setType(JsonNode::JsonType::DATA_STRUCT); // ensure that input is not NULL
-	assert(objects.at(ID.getNum()));
 
-	if ( subID.getNum() >= objects.at(ID.getNum())->objects.size())
-		objects.at(ID.getNum())->objects.resize(subID.getNum()+1);
+	assert(mapObjectTypes.at(ID.getNum()));
 
-	JsonUtils::inherit(config, objects.at(ID.getNum())->base);
-	loadSubObject(config.getModScope(), identifier, config, objects.at(ID.getNum()).get(), subID.getNum());
+	if (subID.getNum() >= mapObjectTypes.at(ID.getNum())->objectTypeHandlers.size())
+	{
+		mapObjectTypes.at(ID.getNum())->objectTypeHandlers.resize(subID.getNum() + 1);
+	}
+
+	JsonUtils::inherit(config, mapObjectTypes.at(ID.getNum())->base);
+	loadSubObject(config.getModScope(), identifier, config, mapObjectTypes.at(ID.getNum()).get(), subID.getNum());
 }
 
 void CObjectClassesHandler::removeSubObject(MapObjectID ID, MapObjectSubID subID)
 {
-	assert(objects.at(ID.getNum()));
-	objects.at(ID.getNum())->objects.at(subID.getNum()) = nullptr;
+	assert(mapObjectTypes.at(ID.getNum()));
+	mapObjectTypes.at(ID.getNum())->objectTypeHandlers.at(subID.getNum()) = nullptr;
 }
 
 TObjectTypeHandler CObjectClassesHandler::getHandlerFor(MapObjectID type, MapObjectSubID subtype) const
 {
 	try
 	{
-		if (objects.at(type.getNum()) == nullptr)
-			return objects.front()->objects.front();
+		if (mapObjectTypes.at(type.getNum()) == nullptr)
+			return mapObjectTypes.front()->objectTypeHandlers.front();
 
 		auto subID = subtype.getNum();
 		if (type == Obj::PRISON)
 			subID = 0;
-		auto result = objects.at(type.getNum())->objects.at(subID);
+		auto result = mapObjectTypes.at(type.getNum())->objectTypeHandlers.at(subID);
 
 		if (result != nullptr)
 			return result;
@@ -365,11 +368,11 @@ TObjectTypeHandler CObjectClassesHandler::getHandlerFor(const std::string & scop
 	std::optional<si32> id = VLC->identifiers()->getIdentifier(scope, "object", type);
 	if(id)
 	{
-		const auto & object = objects.at(id.value());
+		const auto & object = mapObjectTypes.at(id.value());
 		std::optional<si32> subID = VLC->identifiers()->getIdentifier(scope, object->getJsonKey(), subtype);
 
 		if (subID)
-			return object->objects.at(subID.value());
+			return object->objectTypeHandlers.at(subID.value());
 	}
 
 	std::string errorString = "Failed to find object of type " + type + "::" + subtype;
@@ -386,7 +389,7 @@ std::set<MapObjectID> CObjectClassesHandler::knownObjects() const
 {
 	std::set<MapObjectID> ret;
 
-	for(auto & entry : objects)
+	for(auto & entry : mapObjectTypes)
 		if (entry)
 			ret.insert(entry->id);
 
@@ -397,13 +400,13 @@ std::set<MapObjectSubID> CObjectClassesHandler::knownSubObjects(MapObjectID prim
 {
 	std::set<MapObjectSubID> ret;
 
-	if (!objects.at(primaryID.getNum()))
+	if (!mapObjectTypes.at(primaryID.getNum()))
 	{
 		logGlobal->error("Failed to find object %d", primaryID);
 		return ret;
 	}
 
-	for(const auto & entry : objects.at(primaryID.getNum())->objects)
+	for(const auto & entry : mapObjectTypes.at(primaryID.getNum())->objectTypeHandlers)
 		if (entry)
 			ret.insert(entry->subtype);
 
@@ -436,12 +439,12 @@ void CObjectClassesHandler::beforeValidate(JsonNode & object)
 
 void CObjectClassesHandler::afterLoadFinalization()
 {
-	for(auto & entry : objects)
+	for(auto & entry : mapObjectTypes)
 	{
 		if (!entry)
 			continue;
 
-		for(const auto & obj : entry->objects)
+		for(const auto & obj : entry->objectTypeHandlers)
 		{
 			if (!obj)
 				continue;
@@ -456,7 +459,7 @@ void CObjectClassesHandler::afterLoadFinalization()
 void CObjectClassesHandler::generateExtraMonolithsForRMG(ObjectClass * container)
 {
 	//duplicate existing two-way portals to make reserve for RMG
-	auto& portalVec = container->objects;
+	auto& portalVec = container->objectTypeHandlers;
 	//FIXME: Monoliths  in this vector can be already not useful for every terrain
 	const size_t portalCount = portalVec.size();
 
@@ -500,10 +503,10 @@ std::string CObjectClassesHandler::getObjectName(MapObjectID type, MapObjectSubI
 	if (handler && handler->hasNameTextID())
 		return handler->getNameTranslated();
 
-	if (objects.at(type.getNum()))
-		return objects.at(type.getNum())->getNameTranslated();
+	if (mapObjectTypes.at(type.getNum()))
+		return mapObjectTypes.at(type.getNum())->getNameTranslated();
 
-	return objects.front()->getNameTranslated();
+	return mapObjectTypes.front()->getNameTranslated();
 }
 
 SObjectSounds CObjectClassesHandler::getObjectSounds(MapObjectID type, MapObjectSubID subtype) const
@@ -515,27 +518,27 @@ SObjectSounds CObjectClassesHandler::getObjectSounds(MapObjectID type, MapObject
 	if(type == Obj::PRISON || type == Obj::HERO || type == Obj::SPELL_SCROLL)
 		subtype = 0;
 
-	if(objects.at(type.getNum()))
+	if(mapObjectTypes.at(type.getNum()))
 		return getHandlerFor(type, subtype)->getSounds();
 	else
-		return objects.front()->objects.front()->getSounds();
+		return mapObjectTypes.front()->objectTypeHandlers.front()->getSounds();
 }
 
 std::string CObjectClassesHandler::getObjectHandlerName(MapObjectID type) const
 {
-	if (objects.at(type.getNum()))
-		return objects.at(type.getNum())->handlerName;
+	if (mapObjectTypes.at(type.getNum()))
+		return mapObjectTypes.at(type.getNum())->handlerName;
 	else
-		return objects.front()->handlerName;
+		return mapObjectTypes.front()->handlerName;
 }
 
 std::string CObjectClassesHandler::getJsonKey(MapObjectID type) const
 {
-	if (objects.at(type.getNum()) != nullptr)
-		return objects.at(type.getNum())->getJsonKey();
+	if (mapObjectTypes.at(type.getNum()) != nullptr)
+		return mapObjectTypes.at(type.getNum())->getJsonKey();
 
 	logGlobal->warn("Unknown object of type %d!", type);
-	return objects.front()->getJsonKey();
+	return mapObjectTypes.front()->getJsonKey();
 }
 
 VCMI_LIB_NAMESPACE_END

lib/mapObjectConstructors/CObjectClassesHandler.h

@@ -55,7 +55,7 @@ public:
 	std::string handlerName; // ID of handler that controls this object, should be determined using handlerConstructor map
 
 	JsonNode base;
-	std::vector<TObjectTypeHandler> objects;
+	std::vector<TObjectTypeHandler> objectTypeHandlers;
 
 	ObjectClass();
 	~ObjectClass();
@@ -69,7 +69,7 @@ public:
 class DLL_LINKAGE CObjectClassesHandler : public IHandlerBase, boost::noncopyable
 {
 	/// list of object handlers, each of them handles only one type
-	std::vector< std::unique_ptr<ObjectClass> > objects;
+	std::vector< std::unique_ptr<ObjectClass> > mapObjectTypes;
 
 	/// map that is filled during construction with all known handlers. Not serializeable due to usage of std::function
 	std::map<std::string, std::function<TObjectTypeHandler()> > handlerConstructors;

lib/spells/CSpellHandler.cpp

@@ -687,7 +687,7 @@ std::vector<int> CSpellHandler::spellRangeInHexes(std::string input) const
 	std::set<BattleHex> ret;
 	std::string rng = input + ','; //copy + artificial comma for easier handling
 
-	if(rng.size() >= 2 && rng[0] != 'X' && rng[0] != 'x') //there is at least one hex in range (+artificial comma)
+	if(rng.size() >= 2 && std::tolower(rng[0]) != 'x') //there is at least one hex in range (+artificial comma)
 	{
 		std::string number1;
 		std::string number2;

mapeditor/graphics.cpp

@@ -48,9 +48,9 @@ void Graphics::loadPaletteAndColors()
 	for(int i = 0; i < 256; ++i)
 	{
 		QColor col;
-		col.setRed(pals[startPoint++]);
-		col.setGreen(pals[startPoint++]);
-		col.setBlue(pals[startPoint++]);
+		col.setRed(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));
+		col.setGreen(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));
+		col.setBlue(std::clamp(static_cast<int>(pals[startPoint++]), 0, 255));
 		col.setAlpha(255);
 		startPoint++;
 		playerColorPalette[i] = col.rgba();

mapeditor/mapcontroller.cpp

@@ -381,9 +381,15 @@ void MapController::pasteFromClipboard(int level)
 	if(_clipboardShiftIndex == int3::getDirs().size())
 		_clipboardShiftIndex = 0;
 	
+	QStringList errors;
 	for(auto & objUniquePtr : _clipboard)
 	{
 		auto * obj = CMemorySerializer::deepCopy(*objUniquePtr).release();
+		QString errorMsg;
+		if (!canPlaceObject(level, obj, errorMsg))
+		{
+			errors.push_back(std::move(errorMsg));
+		}
 		auto newPos = objUniquePtr->pos + shift;
 		if(_map->isInTheMap(newPos))
 			obj->pos = newPos;
@@ -394,6 +400,8 @@ void MapController::pasteFromClipboard(int level)
 		_scenes[level]->selectionObjectsView.selectObject(obj);
 		_mapHandler->invalidate(obj);
 	}
+
+	QMessageBox::warning(main, QObject::tr("Can't place object"), errors.join('\n'));
 	
 	_scenes[level]->objectsView.draw();
 	_scenes[level]->passabilityView.update();
@@ -565,13 +573,13 @@ bool MapController::canPlaceObject(int level, CGObjectInstance * newObj, QString
 	{
 		auto typeName = QString::fromStdString(newObj->typeName);
 		auto subTypeName = QString::fromStdString(newObj->subTypeName);
-		error = QString("There can be only one grail object on the map");
+		error = QObject::tr("There can only be one grail object on the map.");
 		return false; //maplimit reached
 	}
 	
 	if(defaultPlayer == PlayerColor::NEUTRAL && (newObj->ID == Obj::HERO || newObj->ID == Obj::RANDOM_HERO))
 	{
-		error = "Hero cannot be created as NEUTRAL";
+		error = QObject::tr("Hero %1 cannot be created as NEUTRAL.").arg(QString::fromStdString(newObj->instanceName));
 		return false;
 	}
 	

mapeditor/validator.cpp

@@ -43,13 +43,13 @@ Validator::~Validator()
 	delete ui;
 }
 
-std::list<Validator::Issue> Validator::validate(const CMap * map)
+std::set<Validator::Issue> Validator::validate(const CMap * map)
 {
-	std::list<Validator::Issue> issues;
+	std::set<Validator::Issue> issues;
 	
 	if(!map)
 	{
-		issues.emplace_back(tr("Map is not loaded"), true);
+		issues.insert({ tr("Map is not loaded"), true });
 		return issues;
 	}
 	
@@ -58,27 +58,29 @@ std::list<Validator::Issue> Validator::validate(const CMap * map)
 		//check player settings
 		int hplayers = 0;
 		int cplayers = 0;
-		std::map<int, int> amountOfCastles;
+		std::map<PlayerColor, int> amountOfTowns;
+		std::map<PlayerColor, int> amountOfHeroes;
+
 		for(int i = 0; i < map->players.size(); ++i)
 		{
 			auto & p = map->players[i];
-			if(p.canAnyonePlay())
-				amountOfCastles[i] = 0;
+			if (p.canAnyonePlay())
+				amountOfTowns[PlayerColor(i)] = 0;
 			if(p.canComputerPlay)
 				++cplayers;
 			if(p.canHumanPlay)
 				++hplayers;
 			if(p.allowedFactions.empty())
-				issues.emplace_back(QString(tr("No factions allowed for player %1")).arg(i), true);
+				issues.insert({ tr("No factions allowed for player %1").arg(i), true });
 		}
 		if(hplayers + cplayers == 0)
-			issues.emplace_back(tr("No players allowed to play this map"), true);
+			issues.insert({ tr("No players allowed to play this map"), true });
 		if(hplayers + cplayers == 1)
-			issues.emplace_back(tr("Map is allowed for one player and cannot be started"), true);
+			issues.insert({ tr("Map is allowed for one player and cannot be started"), true });
 		if(!hplayers)
-			issues.emplace_back(tr("No human players allowed to play this map"), true);
+			issues.insert({ tr("No human players allowed to play this map"), true });
 
-		std::set<const CHero*> allHeroesOnMap; //used to find hero duplicated
+		std::set<const CHero * > allHeroesOnMap; //used to find hero duplicated
 		
 		//checking all objects in the map
 		for(auto o : map->objects)
@@ -86,105 +88,111 @@ std::list<Validator::Issue> Validator::validate(const CMap * map)
 			//owners for objects
 			if(o->getOwner() == PlayerColor::UNFLAGGABLE)
 			{
-				if(dynamic_cast<CGMine*>(o.get()) ||
-				   dynamic_cast<CGDwelling*>(o.get()) ||
-				   dynamic_cast<CGTownInstance*>(o.get()) ||
-				   dynamic_cast<CGGarrison*>(o.get()) ||
-				   dynamic_cast<CGHeroInstance*>(o.get()))
+				if(dynamic_cast<CGMine *>(o.get()) ||
+				   dynamic_cast<CGDwelling *>(o.get()) ||
+				   dynamic_cast<CGTownInstance *>(o.get()) ||
+				   dynamic_cast<CGGarrison *>(o.get()) ||
+				   dynamic_cast<CGHeroInstance *>(o.get()))
 				{
-					issues.emplace_back(QString(tr("Armored instance %1 is UNFLAGGABLE but must have NEUTRAL or player owner")).arg(o->instanceName.c_str()), true);
+					issues.insert({ tr("Armored instance %1 is UNFLAGGABLE but must have NEUTRAL or player owner").arg(o->instanceName.c_str()), true });
 				}
 			}
 			if(o->getOwner() != PlayerColor::NEUTRAL && o->getOwner().getNum() < map->players.size())
 			{
 				if(!map->players[o->getOwner().getNum()].canAnyonePlay())
-					issues.emplace_back(QString(tr("Object %1 is assigned to non-playable player %2")).arg(o->instanceName.c_str(), o->getOwner().toString().c_str()), true);
+					issues.insert({ tr("Object %1 is assigned to non-playable player %2").arg(o->instanceName.c_str(), o->getOwner().toString().c_str()), true });
 			}
-			//checking towns
-			if(auto * ins = dynamic_cast<CGTownInstance*>(o.get()))
+			//count towns
+			if(auto * ins = dynamic_cast<CGTownInstance *>(o.get()))
 			{
-				bool has = amountOfCastles.count(ins->getOwner().getNum());
-				if(!has && ins->getOwner() != PlayerColor::NEUTRAL)
-					issues.emplace_back(tr("Town %1 has undefined owner %2").arg(ins->instanceName.c_str(), ins->getOwner().toString().c_str()), true);
-				if(has)
-					++amountOfCastles[ins->getOwner().getNum()];
+					++amountOfTowns[ins->getOwner()];
 			}
-			//checking heroes and prisons
-			if(auto * ins = dynamic_cast<CGHeroInstance*>(o.get()))
+			//checking and counting heroes and prisons
+			if(auto * ins = dynamic_cast<CGHeroInstance *>(o.get()))
 			{
 				if(ins->ID == Obj::PRISON)
 				{
 					if(ins->getOwner() != PlayerColor::NEUTRAL)
-						issues.emplace_back(QString(tr("Prison %1 must be a NEUTRAL")).arg(ins->instanceName.c_str()), true);
+						issues.insert({ tr("Prison %1 must be a NEUTRAL").arg(ins->instanceName.c_str()), true });
 				}
 				else
 				{
-					bool has = amountOfCastles.count(ins->getOwner().getNum());
-					if(!has)
-						issues.emplace_back(QString(tr("Hero %1 must have an owner")).arg(ins->instanceName.c_str()), true);
+					if(ins->getOwner() == PlayerColor::NEUTRAL)
+						issues.insert({ tr("Hero %1 must have an owner").arg(ins->instanceName.c_str()), true });
+
+					++amountOfHeroes[ins->getOwner()];
 				}
 				if(ins->type)
 				{
 					if(map->allowedHeroes.count(ins->getHeroType()) == 0)
-						issues.emplace_back(QString(tr("Hero %1 is prohibited by map settings")).arg(ins->type->getNameTranslated().c_str()), false);
+						issues.insert({ tr("Hero %1 is prohibited by map settings").arg(ins->type->getNameTranslated().c_str()), false });
 					
 					if(!allHeroesOnMap.insert(ins->type).second)
-						issues.emplace_back(QString(tr("Hero %1 has duplicate on map")).arg(ins->type->getNameTranslated().c_str()), false);
+						issues.insert({ tr("Hero %1 has duplicate on map").arg(ins->type->getNameTranslated().c_str()), false });
 				}
 				else if(ins->ID != Obj::RANDOM_HERO)
-					issues.emplace_back(QString(tr("Hero %1 has an empty type and must be removed")).arg(ins->instanceName.c_str()), true);
+					issues.insert({ tr("Hero %1 has an empty type and must be removed").arg(ins->instanceName.c_str()), true });
 			}
 			
 			//checking for arts
-			if(auto * ins = dynamic_cast<CGArtifact*>(o.get()))
+			if(auto * ins = dynamic_cast<CGArtifact *>(o.get()))
 			{
 				if(ins->ID == Obj::SPELL_SCROLL)
 				{
-					if(ins->storedArtifact)
+					if (ins->storedArtifact)
 					{
-						if(map->allowedSpells.count(ins->storedArtifact->getScrollSpellID()) == 0)
-							issues.emplace_back(QString(tr("Spell scroll %1 is prohibited by map settings")).arg(ins->storedArtifact->getScrollSpellID().toEntity(VLC->spells())->getNameTranslated().c_str()), false);
+						if (map->allowedSpells.count(ins->storedArtifact->getScrollSpellID()) == 0)
+							issues.insert({ tr("Spell scroll %1 is prohibited by map settings").arg(ins->storedArtifact->getScrollSpellID().toEntity(VLC->spells())->getNameTranslated().c_str()), false });
 					}
 					else
-						issues.emplace_back(QString(tr("Spell scroll %1 doesn't have instance assigned and must be removed")).arg(ins->instanceName.c_str()), true);
+						issues.insert({ tr("Spell scroll % 1 doesn't have instance assigned and must be removed").arg(ins->instanceName.c_str()), true });
 				}
 				else
 				{
 					if(ins->ID == Obj::ARTIFACT && map->allowedArtifact.count(ins->getArtifact()) == 0)
 					{
-						issues.emplace_back(QString(tr("Artifact %1 is prohibited by map settings")).arg(ins->getObjectName().c_str()), false);
+						issues.insert({ tr("Artifact % 1 is prohibited by map settings").arg(ins->getObjectName().c_str()), false });
 					}
 				}
 			}
 		}
 
 		//verification of starting towns
-		for(auto & mp : amountOfCastles)
-			if(mp.second == 0)
-				issues.emplace_back(QString(tr("Player %1 doesn't have any starting town")).arg(mp.first), false);
+		for (const auto & [player, counter] : amountOfTowns)
+		{
+			if (counter == 0)
+			{
+				// FIXME: heroesNames are empty even though heroes are on the map
+				// if(map->players[playerTCounter.first].heroesNames.empty())
+				if(amountOfHeroes.count(player) == 0)
+					issues.insert({ tr("Player %1 has no towns and heroes assigned").arg(player + 1), true });
+				else
+					issues.insert({ tr("Player %1 doesn't have any starting town").arg(player + 1), false });
+			}
+		}
 
 		//verification of map name and description
 		if(map->name.empty())
-			issues.emplace_back(tr("Map name is not specified"), false);
+			issues.insert({ tr("Map name is not specified"), false });
 		if(map->description.empty())
-			issues.emplace_back(tr("Map description is not specified"), false);
+			issues.insert({ tr("Map description is not specified"), false });
 		
 		//verificationfor mods
 		for(auto & mod : MapController::modAssessmentMap(*map))
 		{
 			if(!map->mods.count(mod.first))
 			{
-				issues.emplace_back(QString(tr("Map contains object from mod \"%1\", but doesn't require it")).arg(QString::fromStdString(VLC->modh->getModInfo(mod.first).getVerificationInfo().name)), true);
+				issues.insert({ tr("Map contains object from mod \"%1\", but doesn't require it").arg(QString::fromStdString(VLC->modh->getModInfo(mod.first).getVerificationInfo().name)), true });
 			}
 		}
 	}
 	catch(const std::exception & e)
 	{
-		issues.emplace_back(QString(tr("Exception occurs during validation: %1")).arg(e.what()), true);
+		issues.insert({ tr("Exception occurs during validation: %1").arg(e.what()), true });
 	}
 	catch(...)
 	{
-		issues.emplace_back(tr("Unknown exception occurs during validation"), true);
+		issues.insert({ tr("Unknown exception occurs during validation"), true });
 	}
 	
 	return issues;

mapeditor/validator.h

@@ -11,6 +11,7 @@
 #pragma once
 
 #include <QDialog>
+#include <set>
 
 VCMI_LIB_NAMESPACE_BEGIN
 class CMap;
@@ -30,13 +31,18 @@ public:
 		bool critical;
 		
 		Issue(const QString & m, bool c): message(m), critical(c) {}
+
+		bool operator <(const Issue & other) const
+		{
+			return message < other.message;
+		}
 	};
 	
 public:
 	explicit Validator(const CMap * map, QWidget *parent = nullptr);
 	~Validator();
 	
-	static std::list<Issue> validate(const CMap * map);
+	static std::set<Issue> validate(const CMap * map);
 
 private:
 	Ui::Validator *ui;