Simplify blukMoveArmy logic, fix army transfer bugs

AI/Nullkiller/Helpers/ArmyFormation.cpp

@@ -21,7 +21,7 @@ void ArmyFormation::rearrangeArmyForWhirlpool(const CGHeroInstance * hero)
 
 void ArmyFormation::addSingleCreatureStacks(const CGHeroInstance * hero)
 {
-	auto freeSlots = hero->getFreeSlotsQueue();
+	auto freeSlots = hero->getFreeSlots();
 
 	while(!freeSlots.empty())
 	{
@@ -37,8 +37,8 @@ void ArmyFormation::addSingleCreatureStacks(const CGHeroInstance * hero)
 			break;
 		}
 
-		cb->splitStack(hero, hero, weakestCreature->first, freeSlots.front(), 1);
-		freeSlots.pop();
+		cb->splitStack(hero, hero, weakestCreature->first, freeSlots.back(), 1);
+		freeSlots.pop_back();
 	}
 }
 

lib/mapObjects/army/CCreatureSet.cpp

@@ -173,20 +173,6 @@ std::vector<SlotID> CCreatureSet::getFreeSlots(ui32 slotsAmount) const
 	return freeSlots;
 }
 
-std::queue<SlotID> CCreatureSet::getFreeSlotsQueue(ui32 slotsAmount) const
-{
-	std::queue<SlotID> freeSlots;
-
-	for(ui32 i = 0; i < slotsAmount; i++)
-	{
-		auto slot = SlotID(i);
-
-		if(!vstd::contains(stacks, slot))
-			freeSlots.push(slot);
-	}
-	return freeSlots;
-}
-
 TMapCreatureSlot CCreatureSet::getCreatureMap() const
 {
 	TMapCreatureSlot creatureMap;

lib/mapObjects/army/CCreatureSet.h

@@ -117,7 +117,6 @@ public:
 
 	SlotID getFreeSlot(ui32 slotsAmount = GameConstants::ARMY_SIZE) const; //returns first free slot
 	std::vector<SlotID> getFreeSlots(ui32 slotsAmount = GameConstants::ARMY_SIZE) const;
-	std::queue<SlotID> getFreeSlotsQueue(ui32 slotsAmount = GameConstants::ARMY_SIZE) const;
 
 	TMapCreatureSlot getCreatureMap() const;
 	TCreatureQueue getCreatureQueue(const SlotID & exclude) const;

server/CGameHandler.cpp

@@ -1714,83 +1714,68 @@ bool CGameHandler::bulkMoveArmy(ObjectInstanceID srcArmy, ObjectInstanceID destA
 	if(!srcSlot.validSlot() && complain(complainInvalidSlot))
 		return false;
 
+	if(!isAllowedExchange(srcArmy, destArmy))
+		COMPLAIN_RET("That heroes cannot make any exchange!");
+
 	const auto * armySrc = dynamic_cast<const CArmedInstance*>(gameInfo().getObjInstance(srcArmy));
-	const CCreatureSet & setSrc = *armySrc;
+	const auto * armyDest = dynamic_cast<const CArmedInstance*>(gameInfo().getObjInstance(destArmy));
 
-	if(!vstd::contains(setSrc.stacks, srcSlot) && complain(complainNoCreatures))
+	if(!vstd::contains(armySrc->stacks, srcSlot) && complain(complainNoCreatures))
 		return false;
 
-	const auto * armyDest = dynamic_cast<const CArmedInstance*>(gameInfo().getObjInstance(destArmy));
-	const CCreatureSet & setDest = *armyDest;
-	auto freeSlots = setDest.getFreeSlotsQueue();
-
-	std::map<SlotID, std::pair<SlotID, TQuantity>> moves;
+	auto freeSlots = armyDest->getFreeSlots();
+	bool allTroopsMoved = true;
 
-	auto srcQueue = setSrc.getCreatureQueue(srcSlot); // Exclude srcSlot, it should be moved last
-	auto slotsLeft = setSrc.stacksCount();
-	auto destMap = setDest.getCreatureMap();
-	TMapCreatureSlot::key_compare keyComp = destMap.key_comp();
+	BulkRebalanceStacks bulkRS;
 
-	while(!srcQueue.empty())
+	for (const auto & slot : armySrc->Slots())
 	{
-		auto pair = srcQueue.top();
-		srcQueue.pop();
-
-		const auto * currCreature = pair.first;
-		auto currSlot = pair.second;
-		const auto quantity = setSrc.getStackCount(currSlot);
+		auto targetSlot = armyDest->getSlotFor(slot.second->getCreature());
 
-		const auto lb = destMap.lower_bound(currCreature);
-		const bool alreadyExists = (lb != destMap.end() && !(keyComp(currCreature, lb->first)));
-
-		if(!alreadyExists)
+		if (armyDest->slotEmpty(targetSlot))
 		{
-			if(freeSlots.empty())
-				continue;
+			if (freeSlots.empty())
+			{
+				allTroopsMoved = false;
+				continue; // no more free slots, but we might still have units that are present in both armies
+			}
 
-			auto currFreeSlot = freeSlots.front();
-			freeSlots.pop();
-			destMap.insert(lb, TMapCreatureSlot::value_type(currCreature, currFreeSlot));
+			targetSlot = freeSlots.front();
+			freeSlots.erase(freeSlots.begin());
 		}
-		moves.insert(std::make_pair(currSlot, std::make_pair(destMap[currCreature], quantity)));
-		slotsLeft--;
+
+		RebalanceStacks rs;
+		rs.srcArmy = armySrc->id;
+		rs.dstArmy = armyDest->id;
+		rs.srcSlot = slot.first;
+		rs.dstSlot = targetSlot;
+		rs.count = slot.second->getCount();
+
+		bulkRS.moves.push_back(rs);
 	}
-	if(slotsLeft == 1)
+
+	// all troops were moved, but we can't leave source hero without troops - undo movement of 1 unit from srcSlot
+	if (allTroopsMoved)
 	{
-		const auto * lastCreature = setSrc.getCreature(srcSlot);
-		auto slotToMove = SlotID();
-		// Try to find a slot for last creature
-		if(destMap.find(lastCreature) == destMap.end())
+		if (armySrc->getStack(srcSlot).getCount() == 1)
 		{
-			if(!freeSlots.empty())
-				slotToMove = freeSlots.front();
+			// slot only had 1 unit - remove this move completely
+			vstd::erase_if(bulkRS.moves, [srcSlot](const RebalanceStacks & move)
+			{
+				return move.srcSlot == srcSlot;
+			});
 		}
 		else
 		{
-			slotToMove = destMap[lastCreature];
-		}
-
-		if(slotToMove != SlotID())
-		{
-			const bool needsLastStack = armySrc->needsLastStack();
-			const auto quantity = setSrc.getStackCount(srcSlot) - (needsLastStack ? 1 : 0);
-
-			if(quantity > 0) //0 may happen when we need last creature and we have exactly 1 amount of that creature - amount of "rest we can transfer" becomes 0
-				moves.insert(std::make_pair(srcSlot, std::make_pair(slotToMove, quantity)));
+			// slot has multiple units - move all but one
+			for (auto & move : bulkRS.moves)
+			{
+				if (move.srcSlot == srcSlot)
+					move.count -= 1;
+			}
 		}
 	}
-	BulkRebalanceStacks bulkRS;
 
-	for(const auto & move : moves)
-	{
-		RebalanceStacks rs;
-		rs.srcArmy = armySrc->id;
-		rs.dstArmy = armyDest->id;
-		rs.srcSlot = move.first;
-		rs.dstSlot = move.second.first;
-		rs.count = move.second.second;
-		bulkRS.moves.push_back(rs);
-	}
 	sendAndApply(bulkRS);
 	return true;
 }