Merge pull request #4563 from SoundSSGood/vcmiScrolls

Improvements to the artifact creation

client/Client.h

@@ -184,7 +184,8 @@ public:
 
 	void removeAfterVisit(const CGObjectInstance * object) override {};
 	bool swapGarrisonOnSiege(ObjectInstanceID tid) override {return false;};
-	bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos) override {return false;}
+	bool giveHeroNewArtifact(const CGHeroInstance * h, const ArtifactID & artId, const ArtifactPosition & pos) override {return false;};
+	bool giveHeroNewScroll(const CGHeroInstance * h, const SpellID & spellId, const ArtifactPosition & pos) override {return false;};
 	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override {return false;};
 	void removeArtifact(const ArtifactLocation & al) override {};
 	bool moveArtifact(const PlayerColor & player, const ArtifactLocation & al1, const ArtifactLocation & al2) override {return false;};

lib/ArtifactUtils.cpp

@@ -14,7 +14,6 @@
 #include "GameSettings.h"
 #include "spells/CSpellHandler.h"
 
-#include "mapping/CMap.h"
 #include "mapObjects/CGHeroInstance.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
@@ -222,67 +221,47 @@ DLL_LINKAGE std::vector<const CArtifact*> ArtifactUtils::assemblyPossibilities(
 	return arts;
 }
 
-DLL_LINKAGE CArtifactInstance * ArtifactUtils::createScroll(const SpellID & sid)
+DLL_LINKAGE CArtifactInstance * ArtifactUtils::createScroll(const SpellID & spellId)
 {
-	auto ret = new CArtifactInstance(ArtifactID(ArtifactID::SPELL_SCROLL).toArtifact());
-	auto bonus = std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::SPELL,
-		BonusSource::ARTIFACT_INSTANCE, -1, BonusSourceID(ArtifactID(ArtifactID::SPELL_SCROLL)), BonusSubtypeID(sid));
-	ret->addNewBonus(bonus);
-	return ret;
+	return ArtifactUtils::createArtifact(ArtifactID::SPELL_SCROLL, spellId);
 }
 
-DLL_LINKAGE CArtifactInstance * ArtifactUtils::createNewArtifactInstance(const CArtifact * art)
+DLL_LINKAGE CArtifactInstance * ArtifactUtils::createArtifact(const ArtifactID & artId, const SpellID & spellId)
 {
-	assert(art);
-
-	auto * artInst = new CArtifactInstance(art);
-	if(art->isCombined())
-	{
-		for(const auto & part : art->getConstituents())
-			artInst->addPart(ArtifactUtils::createNewArtifactInstance(part), ArtifactPosition::PRE_FIRST);
-	}
-	if(art->isGrowing())
+	const std::function<CArtifactInstance*(const CArtifact*)> createArtInst =
+		[&createArtInst, &spellId](const CArtifact * art) -> CArtifactInstance*
 	{
-		auto bonus = std::make_shared<Bonus>();
-		bonus->type = BonusType::LEVEL_COUNTER;
-		bonus->val = 0;
-		artInst->addNewBonus(bonus);
-	}
-	return artInst;
-}
+		assert(art);
 
-DLL_LINKAGE CArtifactInstance * ArtifactUtils::createNewArtifactInstance(const ArtifactID & aid)
-{
-	return ArtifactUtils::createNewArtifactInstance(aid.toArtifact());
-}
-
-DLL_LINKAGE CArtifactInstance * ArtifactUtils::createArtifact(CMap * map, const ArtifactID & aid, SpellID spellID)
-{
-	CArtifactInstance * art = nullptr;
-	if(aid.getNum() >= 0)
-	{
-		if(spellID == SpellID::NONE)
+		auto * artInst = new CArtifactInstance(art);
+		if(art->isCombined())
 		{
-			art = ArtifactUtils::createNewArtifactInstance(aid);
+			for(const auto & part : art->getConstituents())
+				artInst->addPart(createArtInst(part), ArtifactPosition::PRE_FIRST);
 		}
-		else
+		if(art->isGrowing())
 		{
-			art = ArtifactUtils::createScroll(spellID);
+			auto bonus = std::make_shared<Bonus>();
+			bonus->type = BonusType::LEVEL_COUNTER;
+			bonus->val = 0;
+			artInst->addNewBonus(bonus);
 		}
-	}
-	else
+		if(art->isScroll())
+		{
+			artInst->addNewBonus(std::make_shared<Bonus>(BonusDuration::PERMANENT, BonusType::SPELL,
+				BonusSource::ARTIFACT_INSTANCE, -1, BonusSourceID(ArtifactID(ArtifactID::SPELL_SCROLL)), BonusSubtypeID(spellId)));
+		}
+		return artInst;
+	};
+
+	if(artId.getNum() >= 0)
 	{
-		art = new CArtifactInstance(); // random, empty
+		return createArtInst(artId.toArtifact());
 	}
-	map->addNewArtifactInstance(art);
-	if(art->artType && art->isCombined())
+	else
 	{
-		for(auto & part : art->getPartsInfo())
-		{
-			map->addNewArtifactInstance(part.art);
-		}
+		return new CArtifactInstance(); // random, empty
 	}
-	return art;
 }
 
 DLL_LINKAGE void ArtifactUtils::insertScrrollSpellName(std::string & description, const SpellID & sid)

lib/ArtifactUtils.h

@@ -21,7 +21,6 @@ class CGHeroInstance;
 class CArtifactSet;
 class CArtifactInstance;
 struct ArtSlotInfo;
-class CMap;
 
 namespace ArtifactUtils
 {
@@ -40,10 +39,8 @@ namespace ArtifactUtils
 	DLL_LINKAGE bool isSlotEquipment(const ArtifactPosition & slot);
 	DLL_LINKAGE bool isBackpackFreeSlots(const CArtifactSet * target, const size_t reqSlots = 1);
 	DLL_LINKAGE std::vector<const CArtifact*> assemblyPossibilities(const CArtifactSet * artSet, const ArtifactID & aid, const bool onlyEquiped = false);
-	DLL_LINKAGE CArtifactInstance * createScroll(const SpellID & sid);
-	DLL_LINKAGE CArtifactInstance * createNewArtifactInstance(const CArtifact * art);
-	DLL_LINKAGE CArtifactInstance * createNewArtifactInstance(const ArtifactID & aid);
-	DLL_LINKAGE CArtifactInstance * createArtifact(CMap * map, const ArtifactID & aid, SpellID spellID = SpellID::NONE);
+	DLL_LINKAGE CArtifactInstance * createScroll(const SpellID & spellId);
+	DLL_LINKAGE CArtifactInstance * createArtifact(const ArtifactID & artId, const SpellID & spellId = SpellID::NONE);
 	DLL_LINKAGE void insertScrrollSpellName(std::string & description, const SpellID & sid);
 }
 

lib/CArtHandler.cpp

@@ -964,7 +964,7 @@ void CArtifactSet::artDeserializationFix(CBonusSystemNode *node)
 			node->attachTo(*elem.second.artifact);
 }
 
-void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName, CMap * map)
+void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName)
 {
 	//todo: creature and commander artifacts
 	if(handler.saving && artifactsInBackpack.empty() && artifactsWorn.empty())
@@ -972,7 +972,6 @@ void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const s
 
 	if(!handler.saving)
 	{
-		assert(map);
 		artifactsInBackpack.clear();
 		artifactsWorn.clear();
 	}
@@ -982,13 +981,13 @@ void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const s
 	switch(bearerType())
 	{
 	case ArtBearer::HERO:
-		serializeJsonHero(handler, map);
+		serializeJsonHero(handler);
 		break;
 	case ArtBearer::CREATURE:
-		serializeJsonCreature(handler, map);
+		serializeJsonCreature(handler);
 		break;
 	case ArtBearer::COMMANDER:
-		serializeJsonCommander(handler, map);
+		serializeJsonCommander(handler);
 		break;
 	default:
 		assert(false);
@@ -996,11 +995,11 @@ void CArtifactSet::serializeJsonArtifacts(JsonSerializeFormat & handler, const s
 	}
 }
 
-void CArtifactSet::serializeJsonHero(JsonSerializeFormat & handler, CMap * map)
+void CArtifactSet::serializeJsonHero(JsonSerializeFormat & handler)
 {
 	for(const auto & slot : ArtifactUtils::allWornSlots())
 	{
-		serializeJsonSlot(handler, slot, map);
+		serializeJsonSlot(handler, slot);
 	}
 
 	std::vector<ArtifactID> backpackTemp;
@@ -1016,7 +1015,7 @@ void CArtifactSet::serializeJsonHero(JsonSerializeFormat & handler, CMap * map)
 	{
 		for(const ArtifactID & artifactID : backpackTemp)
 		{
-			auto * artifact = ArtifactUtils::createArtifact(map, artifactID);
+			auto * artifact = ArtifactUtils::createArtifact(artifactID);
 			auto slot = ArtifactPosition::BACKPACK_START + artifactsInBackpack.size();
 			if(artifact->artType->canBePutAt(this, slot))
 			{
@@ -1027,17 +1026,17 @@ void CArtifactSet::serializeJsonHero(JsonSerializeFormat & handler, CMap * map)
 	}
 }
 
-void CArtifactSet::serializeJsonCreature(JsonSerializeFormat & handler, CMap * map)
+void CArtifactSet::serializeJsonCreature(JsonSerializeFormat & handler)
 {
 	logGlobal->error("CArtifactSet::serializeJsonCreature not implemented");
 }
 
-void CArtifactSet::serializeJsonCommander(JsonSerializeFormat & handler, CMap * map)
+void CArtifactSet::serializeJsonCommander(JsonSerializeFormat & handler)
 {
 	logGlobal->error("CArtifactSet::serializeJsonCommander not implemented");
 }
 
-void CArtifactSet::serializeJsonSlot(JsonSerializeFormat & handler, const ArtifactPosition & slot, CMap * map)
+void CArtifactSet::serializeJsonSlot(JsonSerializeFormat & handler, const ArtifactPosition & slot)
 {
 	ArtifactID artifactID;
 
@@ -1057,7 +1056,7 @@ void CArtifactSet::serializeJsonSlot(JsonSerializeFormat & handler, const Artifa
 
 		if(artifactID != ArtifactID::NONE)
 		{
-			auto * artifact = ArtifactUtils::createArtifact(map, artifactID.toEnum());
+			auto * artifact = ArtifactUtils::createArtifact(artifactID.toEnum());
 
 			if(artifact->artType->canBePutAt(this, slot))
 			{

lib/CArtHandler.h

@@ -25,7 +25,6 @@ class CArtHandler;
 class CGHeroInstance;
 class CArtifactSet;
 class CArtifactInstance;
-class CMap;
 class JsonSerializeFormat;
 
 #define ART_BEARER_LIST \
@@ -233,16 +232,16 @@ public:
 
 	void artDeserializationFix(CBonusSystemNode *node);
 
-	void serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName, CMap * map);
+	void serializeJsonArtifacts(JsonSerializeFormat & handler, const std::string & fieldName);
 protected:
 	std::pair<const CArtifactInstance *, const CArtifactInstance *> searchForConstituent(const ArtifactID & aid) const;
 
 private:
-	void serializeJsonHero(JsonSerializeFormat & handler, CMap * map);
-	void serializeJsonCreature(JsonSerializeFormat & handler, CMap * map);
-	void serializeJsonCommander(JsonSerializeFormat & handler, CMap * map);
+	void serializeJsonHero(JsonSerializeFormat & handler);
+	void serializeJsonCreature(JsonSerializeFormat & handler);
+	void serializeJsonCommander(JsonSerializeFormat & handler);
 
-	void serializeJsonSlot(JsonSerializeFormat & handler, const ArtifactPosition & slot, CMap * map);//normal slots
+	void serializeJsonSlot(JsonSerializeFormat & handler, const ArtifactPosition & slot);//normal slots
 };
 
 // Used to try on artifacts before the claimed changes have been applied

lib/IGameCallback.h

@@ -118,7 +118,8 @@ public:
 
 	virtual void removeAfterVisit(const CGObjectInstance *object) = 0; //object will be destroyed when interaction is over. Do not call when interaction is not ongoing!
 
-	virtual bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos) = 0;
+	virtual bool giveHeroNewArtifact(const CGHeroInstance * h, const ArtifactID & artId, const ArtifactPosition & pos) = 0;
+	virtual bool giveHeroNewScroll(const CGHeroInstance * h, const SpellID & spellId, const ArtifactPosition & pos) = 0;
 	virtual bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble = std::nullopt) = 0;
 	virtual void removeArtifact(const ArtifactLocation& al) = 0;
 	virtual bool moveArtifact(const PlayerColor & player, const ArtifactLocation & al1, const ArtifactLocation & al2) = 0;

lib/campaign/CampaignState.cpp

@@ -429,7 +429,10 @@ CGHeroInstance * CampaignState::crossoverDeserialize(const JsonNode & node, CMap
 	hero->ID = Obj::HERO;
 	hero->serializeJsonOptions(handler);
 	if (map)
-		hero->serializeJsonArtifacts(handler, "artifacts", map);
+	{
+		hero->serializeJsonArtifacts(handler, "artifacts");
+		map->addNewArtifactInstance(*hero);
+	}
 	return hero;
 }
 

lib/gameState/CGameState.cpp

@@ -1628,7 +1628,7 @@ void CGameState::attachArmedObjects()
 
 bool CGameState::giveHeroArtifact(CGHeroInstance * h, const ArtifactID & aid)
 {
-	 CArtifactInstance * ai = ArtifactUtils::createNewArtifactInstance(aid);
+	 CArtifactInstance * ai = ArtifactUtils::createArtifact(aid);
 	 map->addNewArtifactInstance(ai);
 	 auto slot = ArtifactUtils::getArtAnyPosition(h, aid);
 	 if(ArtifactUtils::isSlotEquipment(slot) || ArtifactUtils::isSlotBackpack(slot))

lib/mapObjects/CBank.cpp

@@ -292,7 +292,7 @@ void CBank::doVisit(const CGHeroInstance * hero) const
 			iw.components.emplace_back(ComponentType::ARTIFACT, elem);
 			loot.appendRawString("%s");
 			loot.replaceName(elem);
-			cb->giveHeroNewArtifact(hero, elem.toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+			cb->giveHeroNewArtifact(hero, elem, ArtifactPosition::FIRST_AVAILABLE);
 		}
 		//display loot
 		if (!iw.components.empty())

lib/mapObjects/CGCreature.cpp

@@ -605,7 +605,7 @@ void CGCreature::giveReward(const CGHeroInstance * h) const
 
 	if(gainedArtifact != ArtifactID::NONE)
 	{
-		cb->giveHeroNewArtifact(h, gainedArtifact.toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+		cb->giveHeroNewArtifact(h, gainedArtifact, ArtifactPosition::FIRST_AVAILABLE);
 		iw.components.emplace_back(ComponentType::ARTIFACT, gainedArtifact);
 	}
 

lib/mapObjects/CGHeroInstance.cpp

@@ -343,7 +343,7 @@ void CGHeroInstance::initHero(vstd::RNG & rand)
 		// hero starts with default spellbook presence status
 		if(!getArt(ArtifactPosition::SPELLBOOK) && type->haveSpellBook)
 		{
-			auto artifact = ArtifactUtils::createNewArtifactInstance(ArtifactID::SPELLBOOK);
+			auto artifact = ArtifactUtils::createArtifact(ArtifactID::SPELLBOOK);
 			putArtifact(ArtifactPosition::SPELLBOOK, artifact);
 		}
 	}
@@ -352,7 +352,7 @@ void CGHeroInstance::initHero(vstd::RNG & rand)
 
 	if(!getArt(ArtifactPosition::MACH4))
 	{
-		auto artifact = ArtifactUtils::createNewArtifactInstance(ArtifactID::CATAPULT);
+		auto artifact = ArtifactUtils::createArtifact(ArtifactID::CATAPULT);
 		putArtifact(ArtifactPosition::MACH4, artifact); //everyone has a catapult
 	}
 
@@ -468,7 +468,7 @@ void CGHeroInstance::initArmy(vstd::RNG & rand, IArmyDescriptor * dst)
 
 				if(!getArt(slot))
 				{
-					auto artifact = ArtifactUtils::createNewArtifactInstance(aid);
+					auto artifact = ArtifactUtils::createArtifact(aid);
 					putArtifact(slot, artifact);
 				}
 				else
@@ -1686,7 +1686,7 @@ void CGHeroInstance::serializeCommonOptions(JsonSerializeFormat & handler)
 	handler.serializeIdArray("spellBook", spells);
 
 	if(handler.saving)
-		CArtifactSet::serializeJsonArtifacts(handler, "artifacts", nullptr);
+		CArtifactSet::serializeJsonArtifacts(handler, "artifacts");
 }
 
 void CGHeroInstance::serializeJsonOptions(JsonSerializeFormat & handler)

lib/mapObjects/CQuest.cpp

@@ -163,7 +163,7 @@ void CQuest::completeQuest(IGameCallback * cb, const CGHeroInstance *h) const
 			for(const auto & ci : parts)
 			{
 				if(ci.art->getTypeId() != elem)
-					cb->giveHeroNewArtifact(h, ci.art->artType, ArtifactPosition::BACKPACK_START);
+					cb->giveHeroNewArtifact(h, ci.art->getTypeId(), ArtifactPosition::BACKPACK_START);
 			}
 		}
 	}

lib/mapping/CMap.cpp

@@ -519,10 +519,26 @@ void CMap::checkForObjectives()
 	}
 }
 
+void CMap::addNewArtifactInstance(CArtifactSet & artSet)
+{
+	for(const auto & [slot, slotInfo] : artSet.artifactsWorn)
+	{
+		if(!slotInfo.locked && slotInfo.getArt())
+			addNewArtifactInstance(slotInfo.artifact);
+	}
+	for(const auto & slotInfo : artSet.artifactsInBackpack)
+		addNewArtifactInstance(slotInfo.artifact);
+}
+
 void CMap::addNewArtifactInstance(ConstTransitivePtr<CArtifactInstance> art)
 {
+	assert(art);
+	assert(art->getId() == -1);
 	art->setId(static_cast<ArtifactInstanceID>(artInstances.size()));
 	artInstances.emplace_back(art);
+		
+	for(const auto & partInfo : art->getPartsInfo())
+		addNewArtifactInstance(partInfo.art);
 }
 
 void CMap::eraseArtifactInstance(CArtifactInstance * art)

lib/mapping/CMap.h

@@ -20,6 +20,7 @@
 VCMI_LIB_NAMESPACE_BEGIN
 
 class CArtifactInstance;
+class CArtifactSet;
 class CGObjectInstance;
 class CGHeroInstance;
 class CCommanderInstance;
@@ -102,6 +103,7 @@ public:
 	void removeBlockVisTiles(CGObjectInstance * obj, bool total = false);
 	void calculateGuardingGreaturePositions();
 
+	void addNewArtifactInstance(CArtifactSet & artSet);
 	void addNewArtifactInstance(ConstTransitivePtr<CArtifactInstance> art);
 	void eraseArtifactInstance(CArtifactInstance * art);
 

lib/mapping/MapFormatH3M.cpp

@@ -957,14 +957,15 @@ bool CMapLoaderH3M::loadArtifactToSlot(CGHeroInstance * hero, int slot)
 	// H3 bug workaround - Enemy hero on 3rd scenario of Good1.h3c campaign ("Long Live The Queen")
 	// He has Shackles of War (normally - MISC slot artifact) in LEFT_HAND slot set in editor
 	// Artifact seems to be missing in game, so skip artifacts that don't fit target slot
-	auto * artifact = ArtifactUtils::createArtifact(map, artifactID);
-	if(artifact->canBePutAt(hero, ArtifactPosition(slot)))
+	if(ArtifactID(artifactID).toArtifact()->canBePutAt(hero, ArtifactPosition(slot)))
 	{
+		auto * artifact = ArtifactUtils::createArtifact(artifactID);
 		artifact->putAt(*hero, ArtifactPosition(slot));
+		map->addNewArtifactInstance(artifact);
 	}
 	else
 	{
-		logGlobal->warn("Map '%s': Artifact '%s' can't be put at the slot %d", mapName, artifact->artType->getNameTranslated(), slot);
+		logGlobal->warn("Map '%s': Artifact '%s' can't be put at the slot %d", mapName, ArtifactID(artifactID).toArtifact()->getNameTranslated(), slot);
 		return false;
 	}
 
@@ -1308,7 +1309,8 @@ CGObjectInstance * CMapLoaderH3M::readArtifact(const int3 & mapPosition, std::sh
 		artID = ArtifactID(objectTemplate->subid);
 	}
 
-	object->storedArtifact = ArtifactUtils::createArtifact(map, artID, spellID.getNum());
+	object->storedArtifact = ArtifactUtils::createArtifact(artID, spellID.getNum());
+	map->addNewArtifactInstance(object->storedArtifact);
 	return object;
 }
 

lib/mapping/MapFormatJson.cpp

@@ -1107,13 +1107,15 @@ void CMapLoaderJson::MapObjectLoader::configure()
 			artID = art->getArtifact();
 		}
 
-		art->storedArtifact = ArtifactUtils::createArtifact(owner->map, artID, spellID.getNum());
+		art->storedArtifact = ArtifactUtils::createArtifact(artID, spellID.getNum());
+		owner->map->addNewArtifactInstance(art->storedArtifact);
 	}
 
 	if(auto * hero = dynamic_cast<CGHeroInstance *>(instance))
 	{
 		auto o = handler.enterStruct("options");
-		hero->serializeJsonArtifacts(handler, "artifacts", owner->map);
+		hero->serializeJsonArtifacts(handler, "artifacts");
+		owner->map->addNewArtifactInstance(*hero);
 	}
 }
 

lib/networkPacks/NetPacksLib.cpp

@@ -1472,17 +1472,11 @@ void NewObject::applyGs(CGameState *gs)
 
 void NewArtifact::applyGs(CGameState *gs)
 {
-	assert(!vstd::contains(gs->map->artInstances, art));
-	assert(!art->getParentNodes().size());
-	assert(art->artType);
-
-	art->setType(art->artType);
-	if(art->isCombined())
-	{
-		for(const auto & part : art->artType->getConstituents())
-			art->addPart(ArtifactUtils::createNewArtifactInstance(part), ArtifactPosition::PRE_FIRST);
-	}
+	auto art = ArtifactUtils::createArtifact(artId, spellId);
 	gs->map->addNewArtifactInstance(art);
+	PutArtifact pa(ArtifactLocation(artHolder, pos), false);
+	pa.art = art;
+	pa.applyGs(gs);
 }
 
 const CStackInstance * StackLocation::getStack()
@@ -1700,6 +1694,7 @@ void PutArtifact::applyGs(CGameState *gs)
 	auto hero = gs->getHero(al.artHolder);
 	assert(hero);
 	assert(art && art->canBePutAt(hero, al.slot));
+	assert(ArtifactUtils::checkIfSlotValid(*hero, al.slot));
 	art->putAt(*hero, al.slot);
 }
 

lib/networkPacks/PacksForClient.h

@@ -965,7 +965,7 @@ struct DLL_LINKAGE CArtifactOperationPack : CPackForClient
 struct DLL_LINKAGE PutArtifact : CArtifactOperationPack
 {
 	PutArtifact() = default;
-	explicit PutArtifact(ArtifactLocation & dst, bool askAssemble = true)
+	explicit PutArtifact(const ArtifactLocation & dst, bool askAssemble = true)
 		: al(dst), askAssemble(askAssemble)
 	{
 	}
@@ -987,14 +987,20 @@ struct DLL_LINKAGE PutArtifact : CArtifactOperationPack
 
 struct DLL_LINKAGE NewArtifact : public CArtifactOperationPack
 {
-	ConstTransitivePtr<CArtifactInstance> art;
+	ObjectInstanceID artHolder;
+	ArtifactID artId;
+	SpellID spellId;
+	ArtifactPosition pos;
 
 	void applyGs(CGameState * gs) override;
 	void visitTyped(ICPackVisitor & visitor) override;
 
 	template <typename Handler> void serialize(Handler & h)
 	{
-		h & art;
+		h & artHolder;
+		h & artId;
+		h & spellId;
+		h & pos;
 	}
 };
 

lib/rewardable/Interface.cpp

@@ -157,7 +157,7 @@ void Rewardable::Interface::grantRewardAfterLevelup(IGameCallback * cb, const Re
 	}
 
 	for(const ArtifactID & art : info.reward.artifacts)
-		cb->giveHeroNewArtifact(hero, art.toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+		cb->giveHeroNewArtifact(hero, art, ArtifactPosition::FIRST_AVAILABLE);
 
 	if(!info.reward.spells.empty())
 	{

mapeditor/mapcontroller.cpp

@@ -165,7 +165,7 @@ void MapController::repairMap(CMap * map) const
 			{
 				nih->removeSpellFromSpellbook(SpellID::SPELLBOOK_PRESET);
 				if(!nih->getArt(ArtifactPosition::SPELLBOOK) && type->haveSpellBook)
-					nih->putArtifact(ArtifactPosition::SPELLBOOK, ArtifactUtils::createNewArtifactInstance(ArtifactID::SPELLBOOK));
+					nih->putArtifact(ArtifactPosition::SPELLBOOK, ArtifactUtils::createArtifact(ArtifactID::SPELLBOOK));
 			}
 			
 		}

server/CGameHandler.cpp

@@ -2267,7 +2267,7 @@ bool CGameHandler::recruitCreatures(ObjectInstanceID objid, ObjectInstanceID dst
 		COMPLAIN_RET_FALSE_IF(artId == ArtifactID::CATAPULT, "Catapult cannot be recruited!");
 		COMPLAIN_RET_FALSE_IF(nullptr == art, "Invalid war machine artifact");
 
-		return giveHeroNewArtifact(hero, art);
+		return giveHeroNewArtifact(hero, artId, ArtifactPosition::FIRST_AVAILABLE);
 	}
 	else
 	{
@@ -2502,7 +2502,7 @@ bool CGameHandler::moveArtifact(const PlayerColor & player, const ArtifactLocati
 
 	auto hero = getHero(dst.artHolder);
 	if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->artType->getId(), dstSlot))
-		giveHeroNewArtifact(hero, ArtifactID(ArtifactID::SPELLBOOK).toArtifact(), ArtifactPosition::SPELLBOOK);
+		giveHeroNewArtifact(hero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 
 	ma.artsPack0.push_back(BulkMoveArtifacts::LinkedSlots(src.slot, dstSlot));
 	if(src.artHolder != dst.artHolder)
@@ -2543,7 +2543,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 			if(auto dstHero = getHero(dstId))
 			{
 				if(ArtifactUtils::checkSpellbookIsNeeded(dstHero, artifact->getTypeId(), dstSlot))
-					giveHeroNewArtifact(dstHero, ArtifactID(ArtifactID::SPELLBOOK).toArtifact(), ArtifactPosition::SPELLBOOK);
+					giveHeroNewArtifact(dstHero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 			}
 		}
 	};
@@ -2736,7 +2736,7 @@ bool CGameHandler::assembleArtifacts(ObjectInstanceID heroID, ArtifactPosition a
 		}
 		
 		if(ArtifactUtils::checkSpellbookIsNeeded(hero, assembleTo, artifactSlot))
-			giveHeroNewArtifact(hero, ArtifactID(ArtifactID::SPELLBOOK).toArtifact(), ArtifactPosition::SPELLBOOK);
+			giveHeroNewArtifact(hero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 
 		AssembledArtifact aa;
 		aa.al = dstLoc;
@@ -2793,7 +2793,7 @@ bool CGameHandler::buyArtifact(ObjectInstanceID hid, ArtifactID aid)
 			return false;
 
 		giveResource(hero->getOwner(),EGameResID::GOLD,-GameConstants::SPELLBOOK_GOLD_COST);
-		giveHeroNewArtifact(hero, ArtifactID(ArtifactID::SPELLBOOK).toArtifact(), ArtifactPosition::SPELLBOOK);
+		giveHeroNewArtifact(hero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 		assert(hero->getArt(ArtifactPosition::SPELLBOOK));
 		giveSpells(town,hero);
 		return true;
@@ -2821,7 +2821,7 @@ bool CGameHandler::buyArtifact(ObjectInstanceID hid, ArtifactID aid)
 			}
 
 			giveResource(hero->getOwner(),EGameResID::GOLD,-price);
-			return giveHeroNewArtifact(hero, art);
+			return giveHeroNewArtifact(hero, aid, ArtifactPosition::FIRST_AVAILABLE);
 		}
 		else
 			COMPLAIN_RET("This machine is unavailable here!");
@@ -2874,7 +2874,7 @@ bool CGameHandler::buyArtifact(const IMarket *m, const CGHeroInstance *h, GameRe
 		COMPLAIN_RET("Cannot find selected artifact on the list");
 
 	sendAndApply(&saa);
-	giveHeroNewArtifact(h, aid.toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+	giveHeroNewArtifact(h, aid, ArtifactPosition::FIRST_AVAILABLE);
 	return true;
 }
 
@@ -3416,7 +3416,7 @@ bool CGameHandler::dig(const CGHeroInstance *h)
 		iw.text.appendLocalString(EMetaText::GENERAL_TXT, 58); //"Congratulations! After spending many hours digging here, your hero has uncovered the " ...
 		iw.text.appendName(grail); // ... " The Grail"
 		iw.soundID = soundBase::ULTIMATEARTIFACT;
-		giveHeroNewArtifact(h, grail.toArtifact(), ArtifactPosition::FIRST_AVAILABLE); //give grail
+		giveHeroNewArtifact(h, grail, ArtifactPosition::FIRST_AVAILABLE); //give grail
 		sendAndApply(&iw);
 
 		iw.soundID = soundBase::invalid;
@@ -3756,13 +3756,21 @@ bool CGameHandler::putArtifact(const ArtifactLocation & al, const CArtifactInsta
 	}
 }
 
-bool CGameHandler::giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos)
+bool CGameHandler::giveHeroNewArtifact(
+	const CGHeroInstance * h, const CArtifact * artType, const SpellID & spellId, const ArtifactPosition & pos)
 {
 	assert(artType);
 
+	NewArtifact na;
+	na.artHolder = h->id;
+	na.artId = artType->getId();
+	na.spellId = spellId;
+	na.pos = pos;
+
 	if(pos == ArtifactPosition::FIRST_AVAILABLE)
 	{
-		if(!artType->canBePutAt(h, ArtifactUtils::getArtAnyPosition(h, artType->getId())))
+		na.pos = ArtifactUtils::getArtAnyPosition(h, artType->getId());
+		if(!artType->canBePutAt(h, na.pos))
 			COMPLAIN_RET("Cannot put artifact in that slot!");
 	}
 	else if(ArtifactUtils::isSlotBackpack(pos))
@@ -3774,18 +3782,18 @@ bool CGameHandler::giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact
 	{
 		COMPLAIN_RET_FALSE_IF(!artType->canBePutAt(h, pos, false), "Cannot put artifact in that slot!");
 	}
+	sendAndApply(&na);
+	return true;
+}
 
-	auto * newArtInst = new CArtifactInstance();
-	newArtInst->artType = artType; // *NOT* via settype -> all bonus-related stuff must be done by NewArtifact apply
-
-	NewArtifact na;
-	na.art = newArtInst;
-	sendAndApply(&na); // -> updates newArtInst!!!
+bool CGameHandler::giveHeroNewArtifact(const CGHeroInstance * h, const ArtifactID & artId, const ArtifactPosition & pos)
+{
+	return giveHeroNewArtifact(h, artId.toArtifact(), SpellID::NONE, pos);
+}
 
-	if(putArtifact(ArtifactLocation(h->id, pos), newArtInst, false))
-		return true;
-	else
-		return false;
+bool CGameHandler::giveHeroNewScroll(const CGHeroInstance * h, const SpellID & spellId, const ArtifactPosition & pos)
+{
+	return giveHeroNewArtifact(h, ArtifactID(ArtifactID::SPELL_SCROLL).toArtifact(), spellId, pos);
 }
 
 void CGameHandler::spawnWanderingMonsters(CreatureID creatureID)

server/CGameHandler.h

@@ -133,7 +133,9 @@ public:
 
 	void removeAfterVisit(const CGObjectInstance *object) override;
 
-	bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos = ArtifactPosition::FIRST_AVAILABLE) override;
+	bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, const SpellID & spellId, const ArtifactPosition & pos);
+	bool giveHeroNewArtifact(const CGHeroInstance * h, const ArtifactID & artId, const ArtifactPosition & pos) override;
+	bool giveHeroNewScroll(const CGHeroInstance * h, const SpellID & spellId, const ArtifactPosition & pos) override;
 	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override;
 	void removeArtifact(const ArtifactLocation &al) override;
 	void removeArtifact(const ObjectInstanceID & srcId, const std::vector<ArtifactPosition> & slotsPack);

server/processors/PlayerMessageProcessor.cpp

@@ -29,6 +29,7 @@
 #include "../../lib/networkPacks/PacksForClient.h"
 #include "../../lib/networkPacks/StackLocation.h"
 #include "../../lib/serializer/Connection.h"
+#include "../../lib/spells/CSpellHandler.h"
 #include "../lib/VCMIDirs.h"
 
 PlayerMessageProcessor::PlayerMessageProcessor(CGameHandler * gameHandler)
@@ -343,7 +344,7 @@ void PlayerMessageProcessor::cheatGiveSpells(PlayerColor player, const CGHeroIns
 
 	///Give hero spellbook
 	if (!hero->hasSpellbook())
-		gameHandler->giveHeroNewArtifact(hero, ArtifactID(ArtifactID::SPELLBOOK).toArtifact(), ArtifactPosition::SPELLBOOK);
+		gameHandler->giveHeroNewArtifact(hero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 
 	///Give all spells with bonus (to allow banned spells)
 	GiveBonus giveBonus(GiveBonus::ETarget::OBJECT);
@@ -421,11 +422,11 @@ void PlayerMessageProcessor::cheatGiveMachines(PlayerColor player, const CGHeroI
 		return;
 
 	if (!hero->getArt(ArtifactPosition::MACH1))
-		gameHandler->giveHeroNewArtifact(hero, ArtifactID(ArtifactID::BALLISTA).toArtifact(), ArtifactPosition::MACH1);
+		gameHandler->giveHeroNewArtifact(hero, ArtifactID::BALLISTA, ArtifactPosition::MACH1);
 	if (!hero->getArt(ArtifactPosition::MACH2))
-		gameHandler->giveHeroNewArtifact(hero, ArtifactID(ArtifactID::AMMO_CART).toArtifact(), ArtifactPosition::MACH2);
+		gameHandler->giveHeroNewArtifact(hero, ArtifactID::AMMO_CART, ArtifactPosition::MACH2);
 	if (!hero->getArt(ArtifactPosition::MACH3))
-		gameHandler->giveHeroNewArtifact(hero, ArtifactID(ArtifactID::FIRST_AID_TENT).toArtifact(), ArtifactPosition::MACH3);
+		gameHandler->giveHeroNewArtifact(hero, ArtifactID::FIRST_AID_TENT, ArtifactPosition::MACH3);
 }
 
 void PlayerMessageProcessor::cheatGiveArtifacts(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words)
@@ -439,7 +440,7 @@ void PlayerMessageProcessor::cheatGiveArtifacts(PlayerColor player, const CGHero
 		{
 			auto artID = VLC->identifiers()->getIdentifier(ModScope::scopeGame(), "artifact", word, false);
 			if(artID &&  VLC->arth->objects[*artID])
-				gameHandler->giveHeroNewArtifact(hero, ArtifactID(*artID).toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+				gameHandler->giveHeroNewArtifact(hero, ArtifactID(*artID), ArtifactPosition::FIRST_AVAILABLE);
 		}
 	}
 	else
@@ -447,11 +448,23 @@ void PlayerMessageProcessor::cheatGiveArtifacts(PlayerColor player, const CGHero
 		for(int g = 7; g < VLC->arth->objects.size(); ++g) //including artifacts from mods
 		{
 			if(VLC->arth->objects[g]->canBePutAt(hero))
-				gameHandler->giveHeroNewArtifact(hero, ArtifactID(g).toArtifact(), ArtifactPosition::FIRST_AVAILABLE);
+				gameHandler->giveHeroNewArtifact(hero, ArtifactID(g), ArtifactPosition::FIRST_AVAILABLE);
 		}
 	}
 }
 
+void PlayerMessageProcessor::cheatGiveScrolls(PlayerColor player, const CGHeroInstance * hero)
+{
+	if(!hero)
+		return;
+
+	for(const auto & spell : VLC->spellh->objects)
+		if(gameHandler->gameState()->isAllowed(spell->getId()) && !spell->isSpecial())
+		{
+			gameHandler->giveHeroNewScroll(hero, spell->getId(), ArtifactPosition::FIRST_AVAILABLE);
+		}
+}
+
 void PlayerMessageProcessor::cheatLevelup(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words)
 {
 	if (!hero)
@@ -675,7 +688,8 @@ bool PlayerMessageProcessor::handleCheatCode(const std::string & cheat, PlayerCo
 		"vcmiolorin",              "vcmiexp",
 		"vcmiluck",                                   "nwcfollowthewhiterabbit", 
 		"vcmimorale",                                 "nwcmorpheus",
-		"vcmigod",                                    "nwctheone"
+		"vcmigod",                                    "nwctheone",
+		"vcmiscrolls"
 	};
 
 	if (!vstd::contains(townTargetedCheats, cheatName) && !vstd::contains(playerTargetedCheats, cheatName) && !vstd::contains(heroTargetedCheats, cheatName))
@@ -752,6 +766,7 @@ void PlayerMessageProcessor::executeCheatCode(const std::string & cheatName, Pla
 	const auto & doCheatRevealPuzzle = [&]() { cheatPuzzleReveal(player); };
 	const auto & doCheatMaxLuck = [&]() { cheatMaxLuck(player, hero); };
 	const auto & doCheatMaxMorale = [&]() { cheatMaxMorale(player, hero); };
+	const auto & doCheatGiveScrolls = [&]() { cheatGiveScrolls(player, hero); };
 	const auto & doCheatTheOne = [&]()
 	{
 		if(!hero)
@@ -820,6 +835,7 @@ void PlayerMessageProcessor::executeCheatCode(const std::string & cheatName, Pla
 		{"nwcmorpheus",             doCheatMaxMorale      },
 		{"vcmigod",                 doCheatTheOne         },
 		{"nwctheone",               doCheatTheOne         },
+		{"vcmiscrolls",             doCheatGiveScrolls    },
 	};
 
 	assert(callbacks.count(cheatName));

server/processors/PlayerMessageProcessor.h

@@ -46,6 +46,7 @@ class PlayerMessageProcessor
 	void cheatGiveArmy(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words);
 	void cheatGiveMachines(PlayerColor player, const CGHeroInstance * hero);
 	void cheatGiveArtifacts(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words);
+	void cheatGiveScrolls(PlayerColor player, const CGHeroInstance * hero);
 	void cheatLevelup(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words);
 	void cheatExperience(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words);
 	void cheatMovement(PlayerColor player, const CGHeroInstance * hero, std::vector<std::string> words);

test/game/CGameStateTest.cpp

@@ -230,17 +230,11 @@ TEST_F(CGameStateTest, DISABLED_issue2765)
 	ASSERT_NE(attacker->tempOwner, defender->tempOwner);
 
 	{
-		CArtifactInstance * a = new CArtifactInstance();
-		a->artType = const_cast<CArtifact *>(ArtifactID(ArtifactID::BALLISTA).toArtifact());
-
 		NewArtifact na;
-		na.art = a;
+		na.artHolder = defender->id;
+		na.artId = ArtifactID::BALLISTA;
+		na.pos = ArtifactPosition::MACH1;
 		gameCallback->sendAndApply(&na);
-
-		PutArtifact pack;
-		pack.al = ArtifactLocation(defender->id, ArtifactPosition::MACH1);
-		pack.art = a;
-		gameCallback->sendAndApply(&pack);
 	}
 
 	startTestBattle(attacker, defender);
@@ -324,17 +318,11 @@ TEST_F(CGameStateTest, DISABLED_battleResurrection)
 	attacker->mana = attacker->manaLimit();
 
 	{
-		CArtifactInstance * a = new CArtifactInstance();
-		a->artType = const_cast<CArtifact *>(ArtifactID(ArtifactID::SPELLBOOK).toArtifact());
-
 		NewArtifact na;
-		na.art = a;
+		na.artHolder = attacker->id;
+		na.artId = ArtifactID::SPELLBOOK;
+		na.pos = ArtifactPosition::SPELLBOOK;
 		gameCallback->sendAndApply(&na);
-
-		PutArtifact pack;
-		pack.al = ArtifactLocation(attacker->id, ArtifactPosition::SPELLBOOK);
-		pack.art = a;
-		gameCallback->sendAndApply(&pack);
 	}
 
 	startTestBattle(attacker, defender);

test/mock/mock_IGameCallback.h

@@ -70,7 +70,8 @@ public:
 
 	void removeAfterVisit(const CGObjectInstance *object) override {} //object will be destroyed when interaction is over. Do not call when interaction is not ongoing!
 
-	bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos) override {return false;}
+	bool giveHeroNewArtifact(const CGHeroInstance * h, const ArtifactID & artId, const ArtifactPosition & pos) override {return false;}
+	bool giveHeroNewScroll(const CGHeroInstance * h, const SpellID & spellId, const ArtifactPosition & pos) override {return false;}
 	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override {return false;}
 	void removeArtifact(const ArtifactLocation &al) override {}
 	bool moveArtifact(const PlayerColor & player, const ArtifactLocation & al1, const ArtifactLocation & al2) override {return false;}