Merge pull request #3161 from SoundSSGood/art-swap-optimization

Artifacts swap optimization

client/Client.h

@@ -188,8 +188,7 @@ 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 giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos) override {return false;}
-	void putArtifact(const ArtifactLocation & al, const CArtifactInstance * a) override {};
+	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override {return false;};
 	void removeArtifact(const ArtifactLocation & al) override {};
 	bool moveArtifact(const ArtifactLocation & al1, const ArtifactLocation & al2) override {return false;};
 

client/NetPacksClient.cpp

@@ -301,7 +301,7 @@ void ApplyClientNetPackVisitor::visitBulkMoveArtifacts(BulkMoveArtifacts & pack)
 		{
 			auto srcLoc = ArtifactLocation(pack.srcArtHolder, slotToMove.srcPos);
 			auto dstLoc = ArtifactLocation(pack.dstArtHolder, slotToMove.dstPos);
-			MoveArtifact ma(&srcLoc, &dstLoc, false);
+			MoveArtifact ma(&srcLoc, &dstLoc, pack.askAssemble);
 			visitMoveArtifact(ma);
 		}
 	};

lib/ArtifactUtils.cpp

@@ -33,10 +33,11 @@ DLL_LINKAGE ArtifactPosition ArtifactUtils::getArtAnyPosition(const CArtifactSet
 DLL_LINKAGE ArtifactPosition ArtifactUtils::getArtBackpackPosition(const CArtifactSet * target, const ArtifactID & aid)
 {
 	const auto * art = aid.toArtifact();
-	if(art->canBePutAt(target, ArtifactPosition::BACKPACK_START))
-	{
-		return ArtifactPosition::BACKPACK_START;
-	}
+	if(target->bearerType() == ArtBearer::HERO)
+		if(art->canBePutAt(target, ArtifactPosition::BACKPACK_START))
+		{
+			return ArtifactPosition::BACKPACK_START;
+		}
 	return ArtifactPosition::PRE_FIRST;
 }
 

lib/IGameCallback.h

@@ -107,8 +107,7 @@ 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 giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos) = 0;
-	virtual void putArtifact(const ArtifactLocation &al, const CArtifactInstance *a) = 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 ArtifactLocation &al1, const ArtifactLocation &al2) = 0;
 

lib/mapObjects/MiscObjects.cpp

@@ -881,7 +881,7 @@ void CGArtifact::onHeroVisit(const CGHeroInstance * h) const
 
 void CGArtifact::pick(const CGHeroInstance * h) const
 {
-	if(cb->giveHeroArtifact(h, storedArtifact, ArtifactPosition::FIRST_AVAILABLE))
+	if(cb->putArtifact(ArtifactLocation(h->id, ArtifactPosition::FIRST_AVAILABLE), storedArtifact))
 		cb->removeObject(this, h->getOwner());
 }
 

lib/networkPacks/ArtifactLocation.h

@@ -31,6 +31,12 @@ struct ArtifactLocation
 		, creature(std::nullopt)
 	{
 	}
+	ArtifactLocation(const ObjectInstanceID id, const std::optional<SlotID> creatureSlot)
+		: artHolder(id)
+		, slot(ArtifactPosition::PRE_FIRST)
+		, creature(creatureSlot)
+	{
+	}
 
 	template <typename Handler> void serialize(Handler & h, const int version)
 	{

lib/networkPacks/NetPacksLib.cpp

@@ -1837,13 +1837,13 @@ void BulkMoveArtifacts::applyGs(CGameState * gs)
 			switch(operation)
 			{
 			case EBulkArtsOp::BULK_MOVE:
-				art->move(artSet, srcPos, *gs->getHero(dstArtHolder), slot.dstPos);
+				art->move(artSet, srcPos, *gs->getArtSet(ArtifactLocation(dstArtHolder, dstCreature)), slot.dstPos);
 				break;
 			case EBulkArtsOp::BULK_REMOVE:
 				art->removeFrom(artSet, srcPos);
 				break;
 			case EBulkArtsOp::BULK_PUT:
-				art->putAt(*gs->getHero(srcArtHolder), slot.dstPos);
+				art->putAt(*gs->getArtSet(ArtifactLocation(srcArtHolder, srcCreature)), slot.dstPos);
 				break;
 			default:
 				break;
@@ -1856,11 +1856,11 @@ void BulkMoveArtifacts::applyGs(CGameState * gs)
 		}
 	};
 	
-	auto * leftSet = gs->getArtSet(ArtifactLocation(srcArtHolder));
+	auto * leftSet = gs->getArtSet(ArtifactLocation(srcArtHolder, srcCreature));
 	if(swap)
 	{
 		// Swap
-		auto * rightSet = gs->getArtSet(ArtifactLocation(dstArtHolder));
+		auto * rightSet = gs->getArtSet(ArtifactLocation(dstArtHolder, dstCreature));
 		CArtifactFittingSet artFittingSet(leftSet->bearerType());
 
 		artFittingSet.artifactsWorn = rightSet->artifactsWorn;

lib/networkPacks/PacksForClient.h

@@ -975,13 +975,13 @@ struct DLL_LINKAGE CArtifactOperationPack : CPackForClient
 struct DLL_LINKAGE PutArtifact : CArtifactOperationPack
 {
 	PutArtifact() = default;
-	explicit PutArtifact(ArtifactLocation * dst, bool askAssemble = true)
-		: al(*dst), askAssemble(askAssemble)
+	explicit PutArtifact(ArtifactLocation & dst, bool askAssemble = true)
+		: al(dst), askAssemble(askAssemble)
 	{
 	}
 
 	ArtifactLocation al;
-	bool askAssemble = false;
+	bool askAssemble;
 	ConstTransitivePtr<CArtifactInstance> art;
 
 	void applyGs(CGameState * gs);
@@ -1064,17 +1064,25 @@ struct DLL_LINKAGE BulkMoveArtifacts : CArtifactOperationPack
 
 	ObjectInstanceID srcArtHolder;
 	ObjectInstanceID dstArtHolder;
+	std::optional<SlotID> srcCreature;
+	std::optional<SlotID> dstCreature;
 
 	BulkMoveArtifacts()
 		: srcArtHolder(ObjectInstanceID::NONE)
 		, dstArtHolder(ObjectInstanceID::NONE)
 		, swap(false)
+		, askAssemble(false)
+		, srcCreature(std::nullopt)
+		, dstCreature(std::nullopt)
 	{
 	}
 	BulkMoveArtifacts(const ObjectInstanceID srcArtHolder, const ObjectInstanceID dstArtHolder, bool swap)
 		: srcArtHolder(std::move(srcArtHolder))
 		, dstArtHolder(std::move(dstArtHolder))
 		, swap(swap)
+		, askAssemble(false)
+		, srcCreature(std::nullopt)
+		, dstCreature(std::nullopt)
 	{
 	}
 
@@ -1083,6 +1091,7 @@ struct DLL_LINKAGE BulkMoveArtifacts : CArtifactOperationPack
 	std::vector<LinkedSlots> artsPack0;
 	std::vector<LinkedSlots> artsPack1;
 	bool swap;
+	bool askAssemble;
 
 	void visitTyped(ICPackVisitor & visitor) override;
 
@@ -1092,7 +1101,10 @@ struct DLL_LINKAGE BulkMoveArtifacts : CArtifactOperationPack
 		h & artsPack1;
 		h & srcArtHolder;
 		h & dstArtHolder;
+		h & srcCreature;
+		h & dstCreature;
 		h & swap;
+		h & askAssemble;
 	}
 };
 

server/CGameHandler.cpp

@@ -2667,19 +2667,18 @@ bool CGameHandler::garrisonSwap(ObjectInstanceID tid)
 // Function moves artifact from src to dst. If dst is not a backpack and is already occupied, old dst art goes to backpack and is replaced.
 bool CGameHandler::moveArtifact(const ArtifactLocation & src, const ArtifactLocation & dst)
 {
-	ArtifactLocation srcLoc = src, dstLoc = dst;
-	const auto srcArtSet = getArtSet(srcLoc);
-	const auto dstArtSet = getArtSet(dstLoc);
+	const auto srcArtSet = getArtSet(src);
+	const auto dstArtSet = getArtSet(dst);
 	assert(srcArtSet);
 	assert(dstArtSet);
 
 	// Make sure exchange is even possible between the two heroes.
-	if(!isAllowedExchange(srcLoc.artHolder, dstLoc.artHolder))
+	if(!isAllowedExchange(src.artHolder, dst.artHolder))
 		COMPLAIN_RET("That heroes cannot make any exchange!");
 
-	const auto srcArtifact = srcArtSet->getArt(srcLoc.slot);
-	const auto dstArtifact = dstArtSet->getArt(dstLoc.slot);
-	const bool isDstSlotBackpack = dstArtSet->bearerType() == ArtBearer::HERO ? ArtifactUtils::isSlotBackpack(dstLoc.slot) : false;
+	const auto srcArtifact = srcArtSet->getArt(src.slot);
+	const auto dstArtifact = dstArtSet->getArt(dst.slot);
+	const bool isDstSlotBackpack = dstArtSet->bearerType() == ArtBearer::HERO ? ArtifactUtils::isSlotBackpack(dst.slot) : false;
 
 	if(srcArtifact == nullptr)
 		COMPLAIN_RET("No artifact to move!");
@@ -2688,55 +2687,47 @@ bool CGameHandler::moveArtifact(const ArtifactLocation & src, const ArtifactLoca
 
 	// Check if src/dest slots are appropriate for the artifacts exchanged.
 	// Moving to the backpack is always allowed.
-	if((!srcArtifact || !isDstSlotBackpack) && srcArtifact && !srcArtifact->canBePutAt(dstArtSet, dstLoc.slot, true))
+	if((!srcArtifact || !isDstSlotBackpack) && srcArtifact && !srcArtifact->canBePutAt(dstArtSet, dst.slot, true))
 		COMPLAIN_RET("Cannot move artifact!");
 
-	auto srcSlotInfo = srcArtSet->getSlot(srcLoc.slot);
-	auto dstSlotInfo = dstArtSet->getSlot(dstLoc.slot);
+	auto srcSlotInfo = srcArtSet->getSlot(src.slot);
+	auto dstSlotInfo = dstArtSet->getSlot(dst.slot);
 
 	if((srcSlotInfo && srcSlotInfo->locked) || (dstSlotInfo && dstSlotInfo->locked))
 		COMPLAIN_RET("Cannot move artifact locks.");
 
 	if(isDstSlotBackpack && srcArtifact->artType->isBig())
 		COMPLAIN_RET("Cannot put big artifacts in backpack!");
-	if(srcLoc.slot == ArtifactPosition::MACH4 || dstLoc.slot == ArtifactPosition::MACH4)
+	if(src.slot == ArtifactPosition::MACH4 || dst.slot == ArtifactPosition::MACH4)
 		COMPLAIN_RET("Cannot move catapult!");
+	if(isDstSlotBackpack && !ArtifactUtils::isBackpackFreeSlots(dstArtSet))
+		COMPLAIN_RET("Backpack is full!");
 
-	if(isDstSlotBackpack)
-	{
-		if(!ArtifactUtils::isBackpackFreeSlots(dstArtSet))
-			COMPLAIN_RET("Backpack is full!");
-		vstd::amin(dstLoc.slot, ArtifactPosition::BACKPACK_START + dstArtSet->artifactsInBackpack.size());
-	}
+	auto dstSlot = std::min(dst.slot, ArtifactPosition(ArtifactPosition::BACKPACK_START + dstArtSet->artifactsInBackpack.size()));
 
-	if(!(srcLoc.slot == ArtifactPosition::TRANSITION_POS && dstLoc.slot == ArtifactPosition::TRANSITION_POS))
+	if(src.slot == dstSlot && src.artHolder == dst.artHolder)
+		COMPLAIN_RET("Won't move artifact: Dest same as source!");
+	
+	BulkMoveArtifacts ma(src.artHolder, dst.artHolder, false);
+	ma.srcCreature = src.creature;
+	ma.dstCreature = dst.creature;
+	
+	// Check if dst slot is occupied
+	if(!isDstSlotBackpack && dstArtifact)
 	{
-		if(srcLoc.slot == dstLoc.slot && srcLoc.artHolder == dstLoc.artHolder)
-			COMPLAIN_RET("Won't move artifact: Dest same as source!");
-
-		// Check if dst slot is occupied
-		if(!isDstSlotBackpack && dstArtifact)
-		{
-			// Previous artifact must be removed first
-			moveArtifact(dstLoc, ArtifactLocation(dstLoc.artHolder, ArtifactPosition::TRANSITION_POS));
-		}
+		// Previous artifact must be removed
+		ma.artsPack1.push_back(BulkMoveArtifacts::LinkedSlots(dstSlot, src.slot));
+		ma.swap = true;
+	}
 
-		try
-		{
-			auto hero = getHero(dst.artHolder);
-			if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->artType->getId(), dstLoc.slot))
-				giveHeroNewArtifact(hero, VLC->arth->objects[ArtifactID::SPELLBOOK], ArtifactPosition::SPELLBOOK);
-		}
-		catch(const std::bad_variant_access &)
-		{
-			// object other than hero received an art - ignore
-		}
+	auto hero = getHero(dst.artHolder);
+	if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->artType->getId(), dstSlot))
+		giveHeroNewArtifact(hero, VLC->arth->objects[ArtifactID::SPELLBOOK], ArtifactPosition::SPELLBOOK);
 
-		MoveArtifact ma(&srcLoc, &dstLoc);
-		if(srcLoc.artHolder == dstLoc.artHolder)
-			ma.askAssemble = false;
-		sendAndApply(&ma);
-	}
+	ma.artsPack0.push_back(BulkMoveArtifacts::LinkedSlots(src.slot, dstSlot));
+	if(src.artHolder != dst.artHolder)
+		ma.askAssemble = true;
+	sendAndApply(&ma);
 	return true;
 }
 
@@ -3961,38 +3952,46 @@ bool CGameHandler::swapStacks(const StackLocation & sl1, const StackLocation & s
 	}
 }
 
-bool CGameHandler::giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos)
+bool CGameHandler::putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble)
 {
-	assert(a->artType);
-	ArtifactLocation al(h->id, ArtifactPosition::PRE_FIRST);
+	assert(art && art->artType);
+	ArtifactLocation dst(al.artHolder, ArtifactPosition::PRE_FIRST);
+	dst.creature = al.creature;
+	auto putTo = getArtSet(al);
+	assert(putTo);
 
-	if(pos == ArtifactPosition::FIRST_AVAILABLE)
+	if(al.slot == ArtifactPosition::FIRST_AVAILABLE)
 	{
-		al.slot = ArtifactUtils::getArtAnyPosition(h, a->getTypeId());
+		dst.slot = ArtifactUtils::getArtAnyPosition(putTo, art->getTypeId());
 	}
-	else if(ArtifactUtils::isSlotBackpack(pos))
+	else if(ArtifactUtils::isSlotBackpack(al.slot) && !al.creature.has_value())
 	{
-		al.slot = ArtifactUtils::getArtBackpackPosition(h, a->getTypeId());
+		dst.slot = ArtifactUtils::getArtBackpackPosition(putTo, art->getTypeId());
 	}
 	else
 	{
-		al.slot = pos;
+		dst.slot = al.slot;
+	}
+
+	if(!askAssemble.has_value())
+	{
+		if(!dst.creature.has_value() && ArtifactUtils::isSlotEquipment(dst.slot))
+			askAssemble = true;
+		else
+			askAssemble = false;
 	}
 
-	if(a->canBePutAt(h, al.slot))
-		putArtifact(al, a);
+	if(art->canBePutAt(putTo, dst.slot))
+	{
+		PutArtifact pa(dst, askAssemble.value());
+		pa.art = art;
+		sendAndApply(&pa);
+		return true;
+	}
 	else
+	{
 		return false;
-
-	return true;
-}
-
-void CGameHandler::putArtifact(const ArtifactLocation &al, const CArtifactInstance *a)
-{
-	PutArtifact pa;
-	pa.art = a;
-	pa.al = al;
-	sendAndApply(&pa);
+	}
 }
 
 bool CGameHandler::giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos)
@@ -4021,7 +4020,7 @@ bool CGameHandler::giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact
 	na.art = newArtInst;
 	sendAndApply(&na); // -> updates newArtInst!!!
 
-	if(giveHeroArtifact(h, newArtInst, pos))
+	if(putArtifact(ArtifactLocation(h->id, pos), newArtInst, false))
 		return true;
 	else
 		return false;

server/CGameHandler.h

@@ -127,8 +127,7 @@ public:
 	void removeAfterVisit(const CGObjectInstance *object) override;
 
 	bool giveHeroNewArtifact(const CGHeroInstance * h, const CArtifact * artType, ArtifactPosition pos = ArtifactPosition::FIRST_AVAILABLE) override;
-	bool giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos) override;
-	void putArtifact(const ArtifactLocation &al, const CArtifactInstance *a) override;
+	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override;
 	void removeArtifact(const ArtifactLocation &al) override;
 	bool moveArtifact(const ArtifactLocation & src, const ArtifactLocation & dst) override;
 	bool bulkMoveArtifacts(ObjectInstanceID srcHero, ObjectInstanceID dstHero, bool swap, bool equipped, bool backpack);

test/mock/mock_IGameCallback.h

@@ -67,8 +67,7 @@ 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 giveHeroArtifact(const CGHeroInstance * h, const CArtifactInstance * a, ArtifactPosition pos) override {return false;}
-	void putArtifact(const ArtifactLocation &al, const CArtifactInstance *a) override {}
+	bool putArtifact(const ArtifactLocation & al, const CArtifactInstance * art, std::optional<bool> askAssemble) override {return false;}
 	void removeArtifact(const ArtifactLocation &al) override {}
 	bool moveArtifact(const ArtifactLocation &al1, const ArtifactLocation &al2) override {return false;}