Make string instance names persistent

lib/NetPacksLib.cpp

@@ -386,6 +386,7 @@ DLL_LINKAGE void RemoveObject::applyGs( CGameState *gs )
 		//If hero on Boat is removed, the Boat disappears
 		if(h->boat)
 		{
+			gs->map->instanceNames.erase(h->boat->instanceName);
 			gs->map->objects[h->boat->id.getNum()].dellNull();
 			h->boat = nullptr;
 		}
@@ -430,7 +431,7 @@ DLL_LINKAGE void RemoveObject::applyGs( CGameState *gs )
 		};
 		event.trigger = event.trigger.morph(patcher);
 	}
-
+	gs->map->instanceNames.erase(obj->instanceName);
 	gs->map->objects[id.getNum()].dellNull();
 	gs->map->calculateGuardingGreaturePositions();
 }

lib/mapObjects/CObjectHandler.cpp

@@ -139,18 +139,6 @@ CGObjectInstance::~CGObjectInstance()
 {
 }
 
-const std::string & CGObjectInstance::getStringId() const
-{
-	if(stringId == "")
-	{
-		boost::format fmt("%s_%d");
-		fmt % typeName % id.getNum();
-		stringId = fmt.str();
-	}
-
-	return stringId;
-}
-
 void CGObjectInstance::setOwner(PlayerColor ow)
 {
 	tempOwner = ow;

lib/mapObjects/CObjectHandler.h

@@ -118,14 +118,13 @@ public:
 	/// If true hero can visit this object only from neighbouring tiles and can't stand on this object
 	bool blockVisit;
 
+	std::string instanceName;
 	std::string typeName;
 	std::string subTypeName;
 
 	CGObjectInstance();
 	~CGObjectInstance();
 
-	const std::string & getStringId() const;
-
 	/// "center" tile from which the sight distance is calculated
 	int3 getSightCenter() const;
 
@@ -177,13 +176,13 @@ public:
 	///Entry point of binary (de-)serialization
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{
-		h & pos & ID & subID & id & tempOwner & blockVisit & appearance;
-		//definfo is handled by map serializer
-
 		if(version >= 759)
 		{
-			h & typeName & subTypeName;
+			h & instanceName & typeName & subTypeName;
 		}
+
+		h & pos & ID & subID & id & tempOwner & blockVisit & appearance;
+		//definfo is handled by map serializer
 	}
 
 	///Entry point of Json (de-)serialization
@@ -200,9 +199,6 @@ protected:
 	virtual void serializeJsonOptions(JsonSerializeFormat & handler);
 
 	void serializeJsonOwner(JsonSerializeFormat & handler);
-
-private:
-	mutable std::string stringId;///<alternate id, dynamically generated, do not serialize
 };
 
 /// function object which can be used to find an object with an specific sub ID

lib/mapping/CMap.cpp

@@ -558,6 +558,32 @@ void CMap::addQuest(CGObjectInstance * quest)
 	quests.push_back(q->quest);
 }
 
+void CMap::addNewObject(CGObjectInstance * obj)
+{
+    if(obj->id != ObjectInstanceID(objects.size()))
+		throw std::runtime_error("Invalid object instance id");
+
+	if(obj->instanceName == "")
+		throw std::runtime_error("Object instance name missing");
+
+	auto it = instanceNames.find(obj->instanceName);
+	if(it != instanceNames.end())
+		throw std::runtime_error("Object instance name duplicated:"+obj->instanceName);
+
+    objects.push_back(obj);
+    instanceNames[obj->instanceName] = obj;
+    addBlockVisTiles(obj);
+
+	if(obj->ID == Obj::TOWN)
+	{
+		towns.push_back(static_cast<CGTownInstance *>(obj));
+	}
+	if(obj->ID == Obj::HERO)
+	{
+		heroesOnMap.push_back(static_cast<CGHeroInstance*>(obj));
+	}
+}
+
 void CMap::initTerrain()
 {
 	int level = twoLevel ? 2 : 1;

lib/mapping/CMap.h

@@ -299,6 +299,7 @@ public:
 	void addNewArtifactInstance(CArtifactInstance * art);
 	void eraseArtifactInstance(CArtifactInstance * art);
 	void addQuest(CGObjectInstance * quest);
+	void addNewObject(CGObjectInstance * obj);
 
 	/// Gets object of specified type on requested position
 	const CGObjectInstance * getObjectiveObjectFrom(int3 pos, Obj::EObj type);
@@ -336,6 +337,8 @@ public:
 
 	int3 ***guardingCreaturePositions;
 
+	std::map<std::string, ConstTransitivePtr<CGObjectInstance> > instanceNames;
+
 private:
 	/// a 3-dimensional array of terrain tiles, access is as follows: x, y, level. where level=1 is underground
 	TerrainTile*** terrain;
@@ -403,5 +406,10 @@ public:
 		h & CGObelisk::obeliskCount & CGObelisk::visited;
 		h & CGTownInstance::merchantArtifacts;
 		h & CGTownInstance::universitySkills;
+
+		if(version >= 759)
+		{
+			h & instanceNames;
+		}
 	}
 };

lib/mapping/CMapEditManager.cpp

@@ -1043,16 +1043,12 @@ void CInsertObjectOperation::execute()
 {
 	obj->pos = pos;
 	obj->id = ObjectInstanceID(map->objects.size());
-	map->objects.push_back(obj);
-	if(obj->ID == Obj::TOWN)
-	{
-		map->towns.push_back(static_cast<CGTownInstance *>(obj));
-	}
-	if(obj->ID == Obj::HERO)
-	{
-		map->heroesOnMap.push_back(static_cast<CGHeroInstance*>(obj));
-	}
-	map->addBlockVisTiles(obj);
+
+	boost::format fmt("%s_%d");
+	fmt % obj->typeName % obj->id.getNum();
+	obj->instanceName = fmt.str();
+
+	map->addNewObject(obj);
 }
 
 void CInsertObjectOperation::undo()

lib/mapping/MapFormatH3M.cpp

@@ -1459,16 +1459,16 @@ void CMapLoaderH3M::readObjects()
 		}
 		nobj->appearance = objTempl;
 		assert(idToBeGiven == ObjectInstanceID(map->objects.size()));
-		map->objects.push_back(nobj);
-		if(nobj->ID == Obj::TOWN)
-		{
-			map->towns.push_back(static_cast<CGTownInstance *>(nobj));
-		}
-		if(nobj->ID == Obj::HERO)
+
 		{
-			logGlobal->debugStream() << "Hero: " << VLC->heroh->heroes[nobj->subID]->name << " at " << objPos;
-			map->heroesOnMap.push_back(static_cast<CGHeroInstance*>(nobj));
+			//TODO: define valid typeName and subtypeName fro H3M maps
+			//boost::format fmt("%s_%d");
+			//fmt % nobj->typeName % nobj->id.getNum();
+			boost::format fmt("obj_%d");
+			fmt % nobj->id.getNum();
+			nobj->instanceName = fmt.str();
 		}
+		map->addNewObject(nobj);
 	}
 
 	std::sort(map->heroesOnMap.begin(), map->heroesOnMap.end(), [](const ConstTransitivePtr<CGHeroInstance> & a, const ConstTransitivePtr<CGHeroInstance> & b)

lib/mapping/MapFormatJson.cpp

@@ -150,21 +150,6 @@ namespace TerrainDetail
 	};
 }
 
-static std::string tailString(const std::string & input, char separator)
-{
-	std::string ret;
-	size_t splitPos = input.find(separator);
-	if (splitPos != std::string::npos)
-		ret = input.substr(splitPos + 1);
-	return ret;
-}
-
-static si32 extractNumber(const std::string & input, char separator)
-{
-	std::string tmp = tailString(input, separator);
-	return atoi(tmp.c_str());
-}
-
 ///CMapFormatJson
 const int CMapFormatJson::VERSION_MAJOR = 1;
 const int CMapFormatJson::VERSION_MINOR = 0;
@@ -547,11 +532,6 @@ void CMapLoaderJson::readMap()
 	readTerrain();
 	readObjects();
 
-	// Calculate blocked / visitable positions
-	for(auto & elem : map->objects)
-	{
-		map->addBlockVisTiles(elem);
-	}
 	map->calculateGuardingGreaturePositions();
 }
 
@@ -747,7 +727,7 @@ void CMapLoaderJson::readTerrain()
 }
 
 CMapLoaderJson::MapObjectLoader::MapObjectLoader(CMapLoaderJson * _owner, JsonMap::value_type& json):
-	owner(_owner), instance(nullptr),id(-1), jsonKey(json.first), configuration(json.second), internalId(extractNumber(jsonKey, '_'))
+	owner(_owner), instance(nullptr),id(-1), jsonKey(json.first), configuration(json.second)
 {
 
 }
@@ -788,7 +768,8 @@ void CMapLoaderJson::MapObjectLoader::construct()
 
 	instance = handler->create(ObjectTemplate());
 	instance->id = ObjectInstanceID(owner->map->objects.size());
-	owner->map->objects.push_back(instance);
+	instance->instanceName = jsonKey;
+	owner->map->addNewObject(instance);
 }
 
 void CMapLoaderJson::MapObjectLoader::configure()
@@ -800,16 +781,7 @@ void CMapLoaderJson::MapObjectLoader::configure()
 
 	instance->serializeJson(handler);
 
-	if(instance->ID == Obj::TOWN)
-	{
-		owner->map->towns.push_back(static_cast<CGTownInstance *>(instance));
-	}
-	else if(instance->ID == Obj::HERO)
-	{
-		logGlobal->debugStream() << "Hero: " << VLC->heroh->heroes[instance->subID]->name << " at " << instance->pos;
-		owner->map->heroesOnMap.push_back(static_cast<CGHeroInstance *>(instance));
-	}
-	else if(auto art = dynamic_cast<CGArtifact *>(instance))
+	if(auto art = dynamic_cast<CGArtifact *>(instance))
 	{
 		//todo: find better place for this code
 
@@ -848,13 +820,6 @@ void CMapLoaderJson::readObjects()
 	for(auto & p : data.Struct())
 		loaders.push_back(vstd::make_unique<MapObjectLoader>(this, p));
 
-	auto sortInfos = [](const std::unique_ptr<MapObjectLoader> & lhs, const std::unique_ptr<MapObjectLoader> & rhs) -> bool
-	{
-		return lhs->internalId < rhs->internalId;
-	};
-	boost::sort(loaders, sortInfos);//this makes instance ids consistent after save-load, needed for testing
-	//todo: use internalId in CMap
-
 	for(auto & ptr : loaders)
 		ptr->construct();
 
@@ -1003,7 +968,7 @@ void CMapSaverJson::writeObjects()
 
 	for(CGObjectInstance * obj : map->objects)
 	{
-		auto temp = handler.enterStruct(obj->getStringId());
+		auto temp = handler.enterStruct(obj->instanceName);
 
 		obj->serializeJson(handler);
 	}

lib/mapping/MapFormatJson.h

@@ -165,7 +165,7 @@ private:
 		ObjectInstanceID id;
 		std::string jsonKey;//full id defined by map creator
 		JsonNode & configuration;
-		si32 internalId;//unique part of id defined by map creator (also = quest identifier)
+
 		///constructs object (without configuration)
 		void construct();
 

test/MapComparer.cpp

@@ -182,7 +182,7 @@ void MapComparer::compareOptions()
 
 void MapComparer::compareObject(const CGObjectInstance * actual, const CGObjectInstance * expected)
 {
-	BOOST_CHECK_EQUAL(actual->getStringId(), expected->getStringId());
+	BOOST_CHECK_EQUAL(actual->instanceName, expected->instanceName);
 	BOOST_CHECK_EQUAL(typeid(actual).name(), typeid(expected).name());//todo: remove and use just comparison
 
 	std::string actualFullID = boost::to_string(boost::format("%s(%d)|%s(%d) %d") % actual->typeName % actual->ID % actual->subTypeName % actual->subID % actual->tempOwner);
@@ -190,18 +190,34 @@ void MapComparer::compareObject(const CGObjectInstance * actual, const CGObjectI
 
 	BOOST_CHECK_EQUAL(actualFullID, expectedFullID);
 	BOOST_CHECK_EQUAL(actual->pos, expected->pos);
-	//BOOST_CHECK_EQUAL(actual->tempOwner,expected->tempOwner);
+
 }
 
 void MapComparer::compareObjects()
 {
 	BOOST_CHECK_EQUAL(actual->objects.size(), expected->objects.size());
 
-	for(size_t idx = 0; idx < std::min(actual->objects.size(), expected->objects.size()); idx++)
+	for(size_t idx = 0; idx < expected->objects.size(); idx++)
 	{
-		BOOST_REQUIRE_EQUAL(idx, expected->objects[idx]->id.getNum());
-		BOOST_CHECK_EQUAL(idx, actual->objects[idx]->id.getNum());
-		compareObject(actual->objects[idx], expected->objects[idx]);
+		auto expectedObject = expected->objects[idx];
+
+		BOOST_REQUIRE_EQUAL(idx, expectedObject->id.getNum());
+
+		{
+			auto it = expected->instanceNames.find(expectedObject->instanceName);
+
+			BOOST_REQUIRE(it != expected->instanceNames.end());
+		}
+
+		{
+			auto it = actual->instanceNames.find(expectedObject->instanceName);
+
+			BOOST_REQUIRE(it != expected->instanceNames.end());
+
+			auto actualObject = it->second;
+
+			compareObject(actualObject, expectedObject);
+		}
 	}
 }