Merge pull request #1394 from SoundSSGood/art-move-fix

Artifact movement refactoring

client/widgets/CArtifactHolder.cpp

@@ -278,13 +278,12 @@ void CHeroArtPlace::select ()
 	if (locked)
 		return;
 
-	selectSlot(true);
 	pickSlot(true);
 	if(ourArt->canBeDisassembled() && slotID < GameConstants::BACKPACK_START) //worn combined artifact -> locks have to disappear
 	{
-		for(int i = 0; i < GameConstants::BACKPACK_START; i++)
+		for(auto slot : ArtifactUtils::constituentWornSlots())
 		{
-			auto ap = ourOwner->getArtPlace(i);
+			auto ap = ourOwner->getArtPlace(slot);
 			if(ap)//getArtPlace may return null
 				ap->pickSlot(ourArt->isPart(ap->ourArt));
 		}
@@ -309,9 +308,9 @@ void CHeroArtPlace::deselect ()
 	pickSlot(false);
 	if(ourArt && ourArt->canBeDisassembled()) //combined art returned to its slot -> restore locks
 	{
-		for(int i = 0; i < GameConstants::BACKPACK_START; i++)
+		for(auto slot : ArtifactUtils::constituentWornSlots())
 		{
-			auto place = ourOwner->getArtPlace(i);
+			auto place = ourOwner->getArtPlace(slot);
 
 			if(nullptr != place)//getArtPlace may return null
 				place->pickSlot(false);
@@ -670,6 +669,16 @@ CArtifactsOfHero::CArtifactsOfHero(const Point & position, bool createCommonPart
 CArtifactsOfHero::~CArtifactsOfHero()
 {
 	dispose();
+	// Artifact located in artifactsTransitionPos should be returned
+	if(!curHero->artifactsTransitionPos.empty())
+	{
+		auto artPlace = getArtPlace(
+			ArtifactUtils::getArtifactDstPosition(curHero->artifactsTransitionPos.begin()->artifact, curHero, curHero->bearerType()));
+		assert(artPlace);
+		assert(artPlace->ourOwner);
+		artPlace->setMeAsDest();
+		artPlace->ourOwner->realizeCurrentTransaction();
+	}
 }
 
 void CArtifactsOfHero::updateParentWindow()
@@ -716,85 +725,76 @@ void CArtifactsOfHero::realizeCurrentTransaction()
 								ArtifactLocation(commonInfo->dst.AOH->curHero, commonInfo->dst.slotID));
 }
 
-void CArtifactsOfHero::artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst)
+void CArtifactsOfHero::artifactMoved(const ArtifactLocation & src, const ArtifactLocation & dst)
 {
 	bool isCurHeroSrc = src.isHolder(curHero),
 		isCurHeroDst = dst.isHolder(curHero);
-	if(isCurHeroSrc && src.slot >= GameConstants::BACKPACK_START)
+	if(isCurHeroSrc && ArtifactUtils::isSlotBackpack(src.slot))
 		updateSlot(src.slot);
-	if(isCurHeroDst && dst.slot >= GameConstants::BACKPACK_START)
+	if(isCurHeroDst && ArtifactUtils::isSlotBackpack(dst.slot))
 		updateSlot(dst.slot);
-	if(isCurHeroSrc  ||  isCurHeroDst) //we need to update all slots, artifact might be combined and affect more slots
+	// We need to update all slots, artifact might be combined and affect more slots
+	if(isCurHeroSrc || isCurHeroDst)
 		updateWornSlots(false);
 
-	if (!src.isHolder(curHero) && !isCurHeroDst)
+	if(!isCurHeroSrc && !isCurHeroDst)
 		return;
 
-	if(commonInfo->src == src) //artifact was taken from us
+	// When moving one artifact onto another it leads to two art movements: dst->TRANSITION_POS; src->dst
+	// however after first movement we pick the art from TRANSITION_POS and the second movement coming when
+	// we have a different artifact may look surprising... but it's valid.
+
+	// Used when doing dragAndDrop and artifact swap multiple times
+	if(src.slot == ArtifactPosition::TRANSITION_POS && 
+		commonInfo->src.slotID == ArtifactPosition::TRANSITION_POS &&
+		commonInfo->dst.slotID == ArtifactPosition::PRE_FIRST && 
+		isCurHeroDst)
+	{
+		auto art = curHero->getArt(ArtifactPosition::TRANSITION_POS);
+		assert(art);
+		CCS->curh->dragAndDropCursor(std::make_unique<CAnimImage>("artifact", art->artType->getIconIndex()));
+		markPossibleSlots(art);
+
+		commonInfo->src.art = art;
+		commonInfo->src.slotID = src.slot;
+	}
+	// Artifact was taken from us
+	else if(commonInfo->src == src)
 	{
-		assert(commonInfo->dst == dst  //expected movement from slot ot slot
-			||  dst.slot == dst.getHolderArtSet()->artifactsInBackpack.size() + GameConstants::BACKPACK_START //artifact moved back to backpack (eg. to make place for art we are moving)
+		// Expected movement from slot ot slot
+		assert(commonInfo->dst == dst
+			// Artifact moved back to backpack (eg. to make place for art we are moving)
+			||  dst.slot == dst.getHolderArtSet()->artifactsInBackpack.size() + GameConstants::BACKPACK_START
 			|| dst.getHolderArtSet()->bearerType() != ArtBearer::HERO);
 		commonInfo->reset();
 		unmarkSlots();
 	}
-	else if(commonInfo->dst == src) //the dest artifact was moved -> we are picking it
+	// The dest artifact was moved after the swap -> we are picking it
+	else if(commonInfo->dst == src)
 	{
-		assert(dst.slot >= GameConstants::BACKPACK_START);
+		assert(dst.slot == ArtifactPosition::TRANSITION_POS);
 		commonInfo->reset();
 
-		CArtifactsOfHero::ArtPlacePtr ap;
-		for(CArtifactsOfHero *aoh : commonInfo->participants)
+		for(CArtifactsOfHero * aoh : commonInfo->participants)
 		{
 			if(dst.isHolder(aoh->curHero))
 			{
 				commonInfo->src.AOH = aoh;
-				if((ap = aoh->getArtPlace(dst.slot)))//getArtPlace may return null
-					break;
+				break;
 			}
 		}
 
-		if(ap)
-		{
-			ap->select();
-		}
-		else
-		{
-			commonInfo->src.art = dst.getArt();
-			commonInfo->src.slotID = dst.slot;
-			assert(commonInfo->src.AOH);
-			CCS->curh->dragAndDropCursor(std::make_unique<CAnimImage>("artifact", dst.getArt()->artType->getIconIndex()));
-			markPossibleSlots(dst.getArt());
-		}
-	}
-	else if(src.slot >= GameConstants::BACKPACK_START &&
-	        src.slot <  commonInfo->src.slotID &&
-			    src.isHolder(commonInfo->src.AOH->curHero)) //artifact taken from before currently picked one
-	{
-		//int fixedSlot = src.hero->getArtPos(commonInfo->src.art);
-		vstd::advance(commonInfo->src.slotID, -1);
-		assert(commonInfo->src.valid());
-	}
-	else
-	{
-		//when moving one artifact onto another it leads to two art movements: dst->backapck; src->dst
-		// however after first movement we pick the art from backpack and the second movement coming when
-		// we have a different artifact may look surprising... but it's valid.
+		commonInfo->src.art = dst.getArt();
+		commonInfo->src.slotID = dst.slot;
+		assert(commonInfo->src.AOH);
+		CCS->curh->dragAndDropCursor(std::make_unique<CAnimImage>("artifact", dst.getArt()->artType->getIconIndex()));
 	}
 
 	updateParentWindow();
-	int shift = 0;
-// 	if(dst.slot >= Arts::BACKPACK_START && dst.slot - Arts::BACKPACK_START < backpackPos)
-// 		shift++;
-//
-	if(src.slot < GameConstants::BACKPACK_START  &&  dst.slot - GameConstants::BACKPACK_START < backpackPos)
-		shift++;
-	if(dst.slot < GameConstants::BACKPACK_START  &&  src.slot - GameConstants::BACKPACK_START < backpackPos)
-		shift--;
-
-	if( (isCurHeroSrc && src.slot >= GameConstants::BACKPACK_START)
-	 || (isCurHeroDst && dst.slot >= GameConstants::BACKPACK_START) )
-		scrollBackpack(shift); //update backpack slots
+	// If backpack is changed, update it
+	if((isCurHeroSrc && ArtifactUtils::isSlotBackpack(src.slot))
+	 || (isCurHeroDst && ArtifactUtils::isSlotBackpack(dst.slot)))
+		scrollBackpack(0);
 }
 
 void CArtifactsOfHero::artifactRemoved(const ArtifactLocation &al)
@@ -808,11 +808,15 @@ void CArtifactsOfHero::artifactRemoved(const ArtifactLocation &al)
 	}
 }
 
-CArtifactsOfHero::ArtPlacePtr CArtifactsOfHero::getArtPlace(int slot)
+CArtifactsOfHero::ArtPlacePtr CArtifactsOfHero::getArtPlace(ArtifactPosition slot)
 {
+	if(slot == ArtifactPosition::TRANSITION_POS)
+	{
+		return nullptr;
+	}
 	if(slot < GameConstants::BACKPACK_START)
 	{
-		if(artWorn.find(ArtifactPosition(slot)) == artWorn.end())
+		if(artWorn.find(slot) == artWorn.end())
 		{
 			logGlobal->error("CArtifactsOfHero::getArtPlace: invalid slot %d", slot);
 			return nullptr;

client/widgets/CArtifactHolder.h

@@ -141,7 +141,7 @@ public:
 	void artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst);
 	void artifactRemoved(const ArtifactLocation &al);
 	void artifactUpdateSlots(const ArtifactLocation &al);
-	ArtPlacePtr getArtPlace(int slot);//may return null
+	ArtPlacePtr getArtPlace(ArtifactPosition slot);//may return null
 
 	void setHero(const CGHeroInstance * hero);
 	const CGHeroInstance *getHero() const;

client/windows/GUIClasses.cpp

@@ -1247,12 +1247,6 @@ CExchangeWindow::CExchangeWindow(ObjectInstanceID hero1, ObjectInstanceID hero2,
 	updateWidgets();
 }
 
-CExchangeWindow::~CExchangeWindow()
-{
-	artifs[0]->commonInfo = nullptr;
-	artifs[1]->commonInfo = nullptr;
-}
-
 const CGarrisonSlot * CExchangeWindow::getSelectedSlotID() const
 {
 	return garr->getSelection();

client/windows/GUIClasses.h

@@ -379,7 +379,6 @@ public:
 	const CGarrisonSlot * getSelectedSlotID() const;
 
 	CExchangeWindow(ObjectInstanceID hero1, ObjectInstanceID hero2, QueryID queryID);
-	~CExchangeWindow();
 };
 
 /// Here you can buy ships

lib/CArtHandler.cpp

@@ -824,7 +824,12 @@ bool CArtifactInstance::canBePutAt(const ArtifactLocation & al, bool assumeDestR
 
 bool CArtifactInstance::canBePutAt(const CArtifactSet *artSet, ArtifactPosition slot, bool assumeDestRemoved) const
 {
-	if(slot >= GameConstants::BACKPACK_START)
+	if(slot == ArtifactPosition::TRANSITION_POS)
+	{
+		return true;
+	}
+
+	if(ArtifactUtils::isSlotBackpack(slot))
 	{
 		if(artType->isBig())
 			return false;
@@ -851,7 +856,7 @@ void CArtifactInstance::putAt(ArtifactLocation al)
 	assert(canBePutAt(al));
 
 	al.getHolderArtSet()->setNewArtSlot(al.slot, this, false);
-	if(!ArtifactUtils::isSlotBackpack(al.slot))
+	if(ArtifactUtils::isSlotEquipment(al.slot))
 		al.getHolderNode()->attachTo(*this);
 }
 
@@ -859,7 +864,7 @@ void CArtifactInstance::removeFrom(ArtifactLocation al)
 {
 	assert(al.getHolderArtSet()->getArt(al.slot) == this);
 	al.getHolderArtSet()->eraseArtSlot(al.slot);
-	if(!ArtifactUtils::isSlotBackpack(al.slot))
+	if(ArtifactUtils::isSlotEquipment(al.slot))
 		al.getHolderNode()->detachFrom(*this);
 }
 
@@ -998,6 +1003,8 @@ bool CArtifactInstance::isPart(const CArtifactInstance *supposedPart) const
 
 bool CCombinedArtifactInstance::canBePutAt(const CArtifactSet *artSet, ArtifactPosition slot, bool assumeDestRemoved) const
 {
+	if(slot == ArtifactPosition::TRANSITION_POS)
+		return true;
 	bool canMainArtifactBePlaced = CArtifactInstance::canBePutAt(artSet, slot, assumeDestRemoved);
 	if(!canMainArtifactBePlaced)
 		return false; //no is no...
@@ -1070,7 +1077,11 @@ void CCombinedArtifactInstance::addAsConstituent(CArtifactInstance *art, Artifac
 
 void CCombinedArtifactInstance::putAt(ArtifactLocation al)
 {
-	if(ArtifactUtils::isSlotBackpack(al.slot))
+	if(al.slot == ArtifactPosition::TRANSITION_POS)
+	{
+		CArtifactInstance::putAt(al);
+	}
+	else if(ArtifactUtils::isSlotBackpack(al.slot))
 	{
 		CArtifactInstance::putAt(al);
 		for(ConstituentInfo &ci : constituentsInfo)
@@ -1108,7 +1119,7 @@ void CCombinedArtifactInstance::putAt(ArtifactLocation al)
 
 void CCombinedArtifactInstance::removeFrom(ArtifactLocation al)
 {
-	if(ArtifactUtils::isSlotBackpack(al.slot))
+	if(ArtifactUtils::isSlotBackpack(al.slot) || al.slot == ArtifactPosition::TRANSITION_POS)
 	{
 		CArtifactInstance::removeFrom(al);
 	}
@@ -1329,6 +1340,12 @@ const CCombinedArtifactInstance *CArtifactSet::getAssemblyByConstituent(Artifact
 
 const ArtSlotInfo * CArtifactSet::getSlot(ArtifactPosition pos) const
 {
+	if(pos == ArtifactPosition::TRANSITION_POS)
+	{
+		// Always add to the end. Always take from the beginning.
+		assert(!artifactsTransitionPos.empty());
+		return &(*artifactsTransitionPos.begin());
+	}
 	if(vstd::contains(artifactsWorn, pos))
 		return &artifactsWorn.at(pos);
 	if(pos >= ArtifactPosition::AFTER_LAST )
@@ -1355,7 +1372,13 @@ ArtSlotInfo & CArtifactSet::retrieveNewArtSlot(ArtifactPosition slot)
 {
 	assert(!vstd::contains(artifactsWorn, slot));
 
-	if (!ArtifactUtils::isSlotBackpack(slot))
+	if(slot == ArtifactPosition::TRANSITION_POS)
+	{
+		// Always add to the end. Always take from the beginning.
+		artifactsTransitionPos.push_back(ArtSlotInfo());
+		return artifactsTransitionPos.back();
+	}
+	if(!ArtifactUtils::isSlotBackpack(slot))
 		return artifactsWorn[slot];
 
 	ArtSlotInfo newSlot;
@@ -1375,7 +1398,12 @@ void CArtifactSet::setNewArtSlot(ArtifactPosition slot, CArtifactInstance *art,
 
 void CArtifactSet::eraseArtSlot(ArtifactPosition slot)
 {
-	if(ArtifactUtils::isSlotBackpack(slot))
+	if(slot == ArtifactPosition::TRANSITION_POS)
+	{
+		assert(!artifactsTransitionPos.empty());
+		artifactsTransitionPos.erase(artifactsTransitionPos.begin());
+	}
+	else if(ArtifactUtils::isSlotBackpack(slot))
 	{
 		auto backpackSlot = ArtifactPosition(slot - GameConstants::BACKPACK_START);
 
@@ -1612,4 +1640,9 @@ DLL_LINKAGE bool ArtifactUtils::isSlotBackpack(ArtifactPosition slot)
 	return slot >= GameConstants::BACKPACK_START;
 }
 
+DLL_LINKAGE bool ArtifactUtils::isSlotEquipment(ArtifactPosition slot)
+{
+	return slot < GameConstants::BACKPACK_START && slot >= 0;
+}
+
 VCMI_LIB_NAMESPACE_END

lib/CArtHandler.h

@@ -317,6 +317,7 @@ class DLL_LINKAGE CArtifactSet
 public:
 	std::vector<ArtSlotInfo> artifactsInBackpack; //hero's artifacts from bag
 	std::map<ArtifactPosition, ArtSlotInfo> artifactsWorn; //map<position,artifact_id>; positions: 0 - head; 1 - shoulders; 2 - neck; 3 - right hand; 4 - left hand; 5 - torso; 6 - right ring; 7 - left ring; 8 - feet; 9 - misc1; 10 - misc2; 11 - misc3; 12 - misc4; 13 - mach1; 14 - mach2; 15 - mach3; 16 - mach4; 17 - spellbook; 18 - misc5
+	std::vector<ArtSlotInfo> artifactsTransitionPos; // Used as transition position for dragAndDrop artifact exchange
 
 	ArtSlotInfo & retrieveNewArtSlot(ArtifactPosition slot);
 	void setNewArtSlot(ArtifactPosition slot, CArtifactInstance *art, bool locked);
@@ -392,6 +393,7 @@ namespace ArtifactUtils
 	DLL_LINKAGE bool isArtRemovable(const std::pair<ArtifactPosition, ArtSlotInfo> & slot);
 	DLL_LINKAGE bool checkSpellbookIsNeeded(const CGHeroInstance * heroPtr, ArtifactID artID, ArtifactPosition slot);
 	DLL_LINKAGE bool isSlotBackpack(ArtifactPosition slot);
+	DLL_LINKAGE bool isSlotEquipment(ArtifactPosition slot);
 }
 
 VCMI_LIB_NAMESPACE_END

lib/GameConstants.h

@@ -983,6 +983,7 @@ class ArtifactPosition
 public:
 	enum EArtifactPosition
 	{
+		TRANSITION_POS = -3,
 		FIRST_AVAILABLE = -2,
 		PRE_FIRST = -1, //sometimes used as error, sometimes as first free in backpack
 		HEAD, SHOULDERS, NECK, RIGHT_HAND, LEFT_HAND, TORSO, //5

server/CGameHandler.cpp

@@ -3892,7 +3892,7 @@ bool CGameHandler::moveArtifact(const ArtifactLocation &al1, const ArtifactLocat
 
 	// Check if src/dest slots are appropriate for the artifacts exchanged.
 	// Moving to the backpack is always allowed.
-	if ((!srcArtifact || dst.slot < GameConstants::BACKPACK_START)
+	if ((!srcArtifact || !ArtifactUtils::isSlotBackpack(dst.slot))
 		&& srcArtifact && !srcArtifact->canBePutAt(dst, true))
 		COMPLAIN_RET("Cannot move artifact!");
 
@@ -3907,24 +3907,29 @@ bool CGameHandler::moveArtifact(const ArtifactLocation &al1, const ArtifactLocat
 	if (src.slot == ArtifactPosition::MACH4 || dst.slot == ArtifactPosition::MACH4)
 		COMPLAIN_RET("Cannot move catapult!");
 
-	if (dst.slot >= GameConstants::BACKPACK_START)
+	if(ArtifactUtils::isSlotBackpack(dst.slot))
 		vstd::amin(dst.slot, ArtifactPosition(GameConstants::BACKPACK_START + (si32)dst.getHolderArtSet()->artifactsInBackpack.size()));
 
-	if (src.slot == dst.slot  &&  src.artHolder == dst.artHolder)
-		COMPLAIN_RET("Won't move artifact: Dest same as source!");
-
-	if (dst.slot < GameConstants::BACKPACK_START  &&  destArtifact) //moving art to another slot
+	if(!(src.slot == ArtifactPosition::TRANSITION_POS && dst.slot == ArtifactPosition::TRANSITION_POS))
 	{
-		//old artifact must be removed first
-		moveArtifact(dst, ArtifactLocation(dst.artHolder, ArtifactPosition(
-			(si32)dst.getHolderArtSet()->artifactsInBackpack.size() + GameConstants::BACKPACK_START)));
-	}
-	auto hero = boost::get<ConstTransitivePtr<CGHeroInstance>>(dst.artHolder);
-	if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->artType->id, dst.slot))
-		giveHeroNewArtifact(hero, VLC->arth->objects[ArtifactID::SPELLBOOK], ArtifactPosition::SPELLBOOK);
+		if(src.slot == dst.slot && src.artHolder == dst.artHolder)
+			COMPLAIN_RET("Won't move artifact: Dest same as source!");
 
-	MoveArtifact ma(&src, &dst);
-	sendAndApply(&ma);
+		// Check if dst slot is occupied
+		if(!ArtifactUtils::isSlotBackpack(dst.slot) && destArtifact)
+		{
+			// Previous artifact must be removed first
+			moveArtifact(dst, ArtifactLocation(dst.artHolder, ArtifactPosition::TRANSITION_POS));
+		}
+		auto hero = boost::get<ConstTransitivePtr<CGHeroInstance>>(dst.artHolder);
+		if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->artType->id, dst.slot))
+			giveHeroNewArtifact(hero, VLC->arth->objects[ArtifactID::SPELLBOOK], ArtifactPosition::SPELLBOOK);
+
+		MoveArtifact ma(&src, &dst);
+		if(dst.slot == ArtifactPosition::TRANSITION_POS)
+			ma.askAssemble = false;
+		sendAndApply(&ma);
+	}
 	return true;
 }