Merge pull request #1615 from SoundSSGood/CArtifactsOfHero-refactoring

Optimization and cleanup CArtifactHolder

client/CPlayerInterface.cpp

@@ -143,6 +143,7 @@ CPlayerInterface::CPlayerInterface(PlayerColor Player)
 
 	duringMovement = false;
 	ignoreEvents = false;
+	numOfMovedArts = 0;
 }
 
 CPlayerInterface::~CPlayerInterface()
@@ -2149,21 +2150,28 @@ void CPlayerInterface::artifactMoved(const ArtifactLocation &src, const Artifact
 {
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	adventureInt->infoBar->showSelection();
+
+	bool redraw = true;
+	// If a bulk transfer has arrived, then redrawing only the last art movement.
+	if(numOfMovedArts != 0)
+	{
+		numOfMovedArts--;
+		if(numOfMovedArts != 0)
+			redraw = false;
+	}
+
 	for(auto isa : GH.listInt)
 	{
 		auto artWin = dynamic_cast<CArtifactHolder*>(isa.get());
 		if (artWin)
-			artWin->artifactMoved(src, dst);
+			artWin->artifactMoved(src, dst, redraw);
 	}
-	if(!GH.objsToBlit.empty())
-		GH.objsToBlit.back()->redraw();
-
 	waitWhileDialog();
 }
 
-void CPlayerInterface::artifactPossibleAssembling(const ArtifactLocation & dst)
+void CPlayerInterface::bulkArtMovementStart(size_t numOfArts)
 {
-	askToAssembleArtifact(dst);
+	numOfMovedArts = numOfArts;
 }
 
 void CPlayerInterface::artifactAssembled(const ArtifactLocation &al)

client/CPlayerInterface.h

@@ -131,8 +131,9 @@ public:
 	void artifactPut(const ArtifactLocation &al) override;
 	void artifactRemoved(const ArtifactLocation &al) override;
 	void artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst) override;
+	void bulkArtMovementStart(size_t numOfArts) override;
 	void artifactAssembled(const ArtifactLocation &al) override;
-	void artifactPossibleAssembling(const ArtifactLocation & dst) override;
+	void askToAssembleArtifact(const ArtifactLocation & dst) override;
 	void artifactDisassembled(const ArtifactLocation &al) override;
 
 	void heroVisit(const CGHeroInstance * visitor, const CGObjectInstance * visitedObj, bool start) override;
@@ -276,10 +277,10 @@ private:
 
 	bool duringMovement;
 	bool ignoreEvents;
+	size_t numOfMovedArts;
 
 	void doMoveHero(const CGHeroInstance *h, CGPath path);
 	void setMovementStatus(bool value);
-	void askToAssembleArtifact(const ArtifactLocation &al);
 };
 
 extern CPlayerInterface * LOCPLINT;

client/NetPacksClient.cpp

@@ -268,7 +268,7 @@ void ApplyClientNetPackVisitor::visitMoveArtifact(MoveArtifact & pack)
 	{
 		callInterfaceIfPresent(cl, player, &IGameEventsReceiver::artifactMoved, pack.src, pack.dst);
 		if(pack.askAssemble)
-			callInterfaceIfPresent(cl, player, &IGameEventsReceiver::artifactPossibleAssembling, pack.dst);
+			callInterfaceIfPresent(cl, player, &IGameEventsReceiver::askToAssembleArtifact, pack.dst);
 	};
 
 	moveArtifact(pack.src.owningPlayer());
@@ -284,12 +284,14 @@ void ApplyClientNetPackVisitor::visitBulkMoveArtifacts(BulkMoveArtifacts & pack)
 		{
 			auto srcLoc = ArtifactLocation(pack.srcArtHolder, slotToMove.srcPos);
 			auto dstLoc = ArtifactLocation(pack.dstArtHolder, slotToMove.dstPos);
-			callInterfaceIfPresent(cl, srcLoc.owningPlayer(), &IGameEventsReceiver::artifactMoved, srcLoc, dstLoc);
+			MoveArtifact ma(&srcLoc, &dstLoc, false);
-			if(srcLoc.owningPlayer() != dstLoc.owningPlayer())
+			visitMoveArtifact(ma);
-				callInterfaceIfPresent(cl, dstLoc.owningPlayer(), &IGameEventsReceiver::artifactMoved, srcLoc, dstLoc);
 		}
 	};
 
+	// Begin a session of bulk movement of arts. It is not necessary but useful for the client optimization.
+	callInterfaceIfPresent(cl, cl.getCurrentPlayer(), &IGameEventsReceiver::bulkArtMovementStart, 
+		pack.artsPack0.size() + pack.artsPack1.size());
 	applyMove(pack.artsPack0);
 	if(pack.swap)
 		applyMove(pack.artsPack1);

client/widgets/CArtifactHolder.cpp

@@ -471,13 +471,14 @@ void CArtifactsOfHero::scrollBackpack(int dir)
  *
  * @param art Artifact checked against.
  */
-void CArtifactsOfHero::markPossibleSlots(const CArtifactInstance* art)
+void CArtifactsOfHero::markPossibleSlots(const CArtifactInstance* art, bool withRedraw)
 {
 	for(CArtifactsOfHero *aoh : commonInfo->participants)
 		for(auto p : aoh->artWorn)
 			p.second->selectSlot(art->canBePutAt(ArtifactLocation(aoh->curHero, p.second->slotID), true));
 
-	safeRedraw();
+	if(withRedraw)
+		safeRedraw();
 }
 
 /**
@@ -489,7 +490,7 @@ void CArtifactsOfHero::unmarkSlots(bool withRedraw)
 		for(CArtifactsOfHero *aoh : commonInfo->participants)
 			aoh->unmarkLocalSlots(false);
 	else
-		unmarkLocalSlots(false);\
+		unmarkLocalSlots(false);
 
 	if(withRedraw)
 		safeRedraw();
@@ -546,7 +547,6 @@ CArtifactsOfHero::CArtifactsOfHero(ArtPlaceMap ArtWorn, std::vector<ArtPlacePtr>
 	backpack(Backpack),
 	backpackPos(0),
 	commonInfo(nullptr),
-	updateState(false),
 	leftArtRoll(leftScroll),
 	rightArtRoll(rightScroll),
 	allowedAssembling(true),
@@ -580,7 +580,6 @@ CArtifactsOfHero::CArtifactsOfHero(const Point & position, bool createCommonPart
 	: curHero(nullptr),
 	backpackPos(0),
 	commonInfo(nullptr),
-	updateState(false),
 	allowedAssembling(true),
 	highlightModeCallback(nullptr)
 {
@@ -644,26 +643,13 @@ CArtifactsOfHero::~CArtifactsOfHero()
 
 void CArtifactsOfHero::updateParentWindow()
 {
-	if (CHeroWindow* chw = dynamic_cast<CHeroWindow*>(GH.topInt().get()))
+	if(CHeroWindow * chw = dynamic_cast<CHeroWindow*>(GH.topInt().get()))
 	{
-		if(updateState)
+		chw->update(curHero, true);
-			chw->curHero = curHero;
-		else
-			chw->update(curHero, true);
 	}
-	else if(CExchangeWindow* cew = dynamic_cast<CExchangeWindow*>(GH.topInt().get()))
+	else if(CExchangeWindow * cew = dynamic_cast<CExchangeWindow*>(GH.topInt().get()))
 	{
-		//use our copy of hero to draw window
+		cew->updateWidgets();
-		if(cew->heroInst[0]->id == curHero->id)
-			cew->heroInst[0] = curHero;
-		else
-			cew->heroInst[1] = curHero;
-
-		if(!updateState)
-		{
-			cew->updateWidgets();
-			cew->redraw();
-		}
 	}
 }
 
@@ -686,7 +672,7 @@ 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 withUIUpdate)
 {
 	bool isCurHeroSrc = src.isHolder(curHero),
 		isCurHeroDst = dst.isHolder(curHero);
@@ -714,7 +700,8 @@ void CArtifactsOfHero::artifactMoved(const ArtifactLocation & src, const Artifac
 		auto art = curHero->getArt(ArtifactPosition::TRANSITION_POS);
 		assert(art);
 		CCS->curh->dragAndDropCursor("artifact", art->artType->getIconIndex());
-		markPossibleSlots(art);
+		if(withUIUpdate)
+			markPossibleSlots(art);
 
 		commonInfo->src.art = art;
 		commonInfo->src.slotID = src.slot;
@@ -752,17 +739,20 @@ void CArtifactsOfHero::artifactMoved(const ArtifactLocation & src, const Artifac
 			assert(commonInfo->src.AOH);
 			CCS->curh->dragAndDropCursor("artifact", dst.getArt()->artType->getIconIndex());
 		}
-		if(!curHero->artifactsTransitionPos.empty())
+		if(!curHero->artifactsTransitionPos.empty() && withUIUpdate)
 		{
 			markPossibleSlots(curHero->getArt(ArtifactPosition::TRANSITION_POS));
 		}
 	}
 
-	updateParentWindow();
+	if(withUIUpdate)
-	// If backpack is changed, update it
+	{
-	if((isCurHeroSrc && ArtifactUtils::isSlotBackpack(src.slot))
+		updateParentWindow();
-	 || (isCurHeroDst && ArtifactUtils::isSlotBackpack(dst.slot)))
+		// If backpack is changed, update it
-		scrollBackpack(0);
+		if((isCurHeroSrc && ArtifactUtils::isSlotBackpack(src.slot))
+			|| (isCurHeroDst && ArtifactUtils::isSlotBackpack(dst.slot)))
+			scrollBackpack(0);
+	}
 }
 
 void CArtifactsOfHero::artifactRemoved(const ArtifactLocation &al)
@@ -806,9 +796,9 @@ void CArtifactsOfHero::artifactUpdateSlots(const ArtifactLocation & al)
 	if(al.isHolder(curHero))
 	{
 		if(ArtifactUtils::isSlotBackpack(al.slot))
-			updateBackpackSlots();
+			updateBackpackSlots(true);
 		else
-			updateWornSlots();
+			updateWornSlots(true);
 	}
 }
 
@@ -872,7 +862,7 @@ void CWindowWithArtifacts::artifactRemoved(const ArtifactLocation &artLoc)
 	}
 }
 
-void CWindowWithArtifacts::artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc)
+void CWindowWithArtifacts::artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc, bool withRedraw)
 {
 	CArtifactsOfHero * destaoh = nullptr;
 
@@ -881,8 +871,7 @@ void CWindowWithArtifacts::artifactMoved(const ArtifactLocation &artLoc, const A
 		std::shared_ptr<CArtifactsOfHero> realPtr = artSetWeak.lock();
 		if(realPtr)
 		{
-			realPtr->artifactMoved(artLoc, destLoc);
+			realPtr->artifactMoved(artLoc, destLoc, withRedraw);
-			realPtr->redraw();
 			if(destLoc.isHolder(realPtr->getHero()))
 				destaoh = realPtr.get();
 		}
@@ -922,17 +911,12 @@ void CArtifactsOfHero::SCommonPart::Artpos::clear()
 	art = nullptr;
 }
 
-CArtifactsOfHero::SCommonPart::Artpos::Artpos()
-{
-	clear();
-}
-
 void CArtifactsOfHero::SCommonPart::Artpos::setTo(const CHeroArtPlace *place, bool dontTakeBackpack)
 {
 	slotID = place->slotID;
 	AOH = place->ourOwner;
 
-	if(slotID >= 19 && dontTakeBackpack)
+	if(ArtifactUtils::isSlotBackpack(slotID) && dontTakeBackpack)
 		art = nullptr;
 	else
 		art = place->ourArt;

client/widgets/CArtifactHolder.h

@@ -27,7 +27,7 @@ public:
 	CArtifactHolder();
 
 	virtual void artifactRemoved(const ArtifactLocation &artLoc)=0;
-	virtual void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc)=0;
+	virtual void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc, bool withRedraw)=0;
 	virtual void artifactDisassembled(const ArtifactLocation &artLoc)=0;
 	virtual void artifactAssembled(const ArtifactLocation &artLoc)=0;
 };
@@ -114,7 +114,6 @@ public:
 			const CArtifactsOfHero *AOH;
 			const CArtifactInstance *art;
 
-			Artpos();
 			void clear();
 			void setTo(const CHeroArtPlace *place, bool dontTakeBackpack);
 			bool valid();
@@ -127,8 +126,6 @@ public:
 	};
 	std::shared_ptr<SCommonPart> commonInfo; //when we have more than one CArtifactsOfHero in one window with exchange possibility, we use this (eg. in exchange window); to be provided externally
 
-	bool updateState; // Whether the commonInfo should be updated on setHero or not.
-
 	std::shared_ptr<CButton> leftArtRoll;
 	std::shared_ptr<CButton> rightArtRoll;
 	bool allowedAssembling;
@@ -137,7 +134,7 @@ public:
 	std::function<void(CHeroArtPlace*)> highlightModeCallback; //if set, clicking on art place doesn't pick artifact but highlights the slot and calls this function
 
 	void realizeCurrentTransaction(); //calls callback with parameters stored in commonInfo
-	void artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst);
+	void artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst, bool withUIUpdate);
 	void artifactRemoved(const ArtifactLocation &al);
 	void artifactUpdateSlots(const ArtifactLocation &al);
 	ArtPlacePtr getArtPlace(ArtifactPosition slot);//may return null
@@ -151,11 +148,11 @@ public:
 	void deactivate() override;
 
 	void safeRedraw();
-	void markPossibleSlots(const CArtifactInstance* art);
+	void markPossibleSlots(const CArtifactInstance * art, bool withRedraw = false);
-	void unmarkSlots(bool withRedraw = true); //unmarks slots in all visible AOHs
+	void unmarkSlots(bool withRedraw = false); //unmarks slots in all visible AOHs
-	void unmarkLocalSlots(bool withRedraw = true); //unmarks slots in that particular AOH
+	void unmarkLocalSlots(bool withRedraw = false); //unmarks slots in that particular AOH
-	void updateWornSlots(bool redrawParent = true);
+	void updateWornSlots(bool redrawParent = false);
-	void updateBackpackSlots(bool redrawParent = true);
+	void updateBackpackSlots(bool redrawParent = false);
 
 	void updateSlot(ArtifactPosition i);
 
@@ -189,7 +186,7 @@ public:
 	std::shared_ptr<CArtifactsOfHero::SCommonPart> getCommonPart();
 
 	void artifactRemoved(const ArtifactLocation &artLoc) override;
-	void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc) override;
+	void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc, bool withRedraw) override;
 	void artifactDisassembled(const ArtifactLocation &artLoc) override;
 	void artifactAssembled(const ArtifactLocation &artLoc) override;
 };

client/windows/CKingdomInterface.cpp

@@ -668,10 +668,10 @@ void CKingdomInterface::artifactDisassembled(const ArtifactLocation& artLoc)
 		arts->artifactDisassembled(artLoc);
 }
 
-void CKingdomInterface::artifactMoved(const ArtifactLocation& artLoc, const ArtifactLocation& destLoc)
+void CKingdomInterface::artifactMoved(const ArtifactLocation& artLoc, const ArtifactLocation& destLoc, bool withRedraw)
 {
 	if(auto arts = std::dynamic_pointer_cast<CArtifactHolder>(tabArea->getItem()))
-		arts->artifactMoved(artLoc, destLoc);
+		arts->artifactMoved(artLoc, destLoc, withRedraw);
 }
 
 void CKingdomInterface::artifactRemoved(const ArtifactLocation& artLoc)

client/windows/CKingdomInterface.h

@@ -251,7 +251,7 @@ public:
 	void townChanged(const CGTownInstance *town);
 	void updateGarrisons() override;
 	void artifactRemoved(const ArtifactLocation &artLoc) override;
-	void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc) override;
+	void artifactMoved(const ArtifactLocation &artLoc, const ArtifactLocation &destLoc, bool withRedraw) override;
 	void artifactDisassembled(const ArtifactLocation &artLoc) override;
 	void artifactAssembled(const ArtifactLocation &artLoc) override;
 };

lib/IGameEventsReceiver.h

@@ -90,7 +90,8 @@ public:
 	virtual void artifactAssembled(const ArtifactLocation &al){};
 	virtual void artifactDisassembled(const ArtifactLocation &al){};
 	virtual void artifactMoved(const ArtifactLocation &src, const ArtifactLocation &dst){};
-	virtual void artifactPossibleAssembling(const ArtifactLocation & dst) {};
+	virtual void bulkArtMovementStart(size_t numOfArts) {};
+	virtual void askToAssembleArtifact(const ArtifactLocation & dst) {};
 
 	virtual void heroVisit(const CGHeroInstance *visitor, const CGObjectInstance *visitedObj, bool start){};
 	virtual void heroCreated(const CGHeroInstance*){};