fix: prevent requesting a new trade window with each "trade" (opentib…

…iabr#2700)

Identified Problems:
• Item Loop Performance: The loop in the ProtocolGame::sendShop function
performed key-value store (KV) access operations for each NPC item
during every iteration. This was highly inefficient for NPCs with many
items, such as "The Lootmonger," which has 1380 items.
• Lack of Database Caching: Currently, our database does not implement
caching for query results. This leads to redundant database accesses,
particularly noticeable when commands like "trade" are used repeatedly,
significantly increasing CPU usage and processing time.

Implemented Solutions:
• Optimization of the Item Loop: Necessary initialization has been moved
outside the loop in the ProtocolGame::sendShop method. This reduces
overhead by avoiding unnecessary repetitive database accesses.
• Removal of Unnecessary Map: Removed an extraneous map in npc.hpp and
simplified the use of the shop player cache.
These changes aim to streamline interactions and reduce the
computational load, especially during frequent trade requests with
item-rich NPCs like "The Lootmonger." Additional optimizations have been
made to enhance overall system performance.

data/npclib/npc_system/npc_handler.lua

@@ -480,7 +480,6 @@ if NpcHandler == nil then
 
 				-- If is npc shop, send shop window and parse default message (if not have callback on the npc)
 				if npc:isMerchant() then
-					npc:closeShopWindow(player)
 					npc:openShopWindow(player)
 					self:say(msg, npc, player)
 				end

src/creatures/npcs/npc.cpp

@@ -108,7 +108,7 @@ void Npc::onRemoveCreature(std::shared_ptr<Creature> creature, bool isLogout) {
 		spawnNpc->startSpawnNpcCheck();
 	}
 
-	shopPlayerMap.clear();
+	shopPlayers.clear();
 }
 
 void Npc::onCreatureMove(const std::shared_ptr<Creature> &creature, const std::shared_ptr<Tile> &newTile, const Position &newPos, const std::shared_ptr<Tile> &oldTile, const Position &oldPos, bool teleport) {
@@ -259,7 +259,7 @@ void Npc::onPlayerBuyItem(std::shared_ptr<Player> player, uint16_t itemId, uint8
 	}
 
 	uint32_t buyPrice = 0;
-	const std::vector<ShopBlock> &shopVector = getShopItemVector(player->getGUID());
+	const auto &shopVector = getShopItemVector(player->getGUID());
 	for (const ShopBlock &shopBlock : shopVector) {
 		if (itemType.id == shopBlock.itemId && shopBlock.itemBuyPrice != 0) {
 			buyPrice = shopBlock.itemBuyPrice;
@@ -372,7 +372,7 @@ void Npc::onPlayerSellItem(std::shared_ptr<Player> player, uint16_t itemId, uint
 
 	uint32_t sellPrice = 0;
 	const ItemType &itemType = Item::items[itemId];
-	const std::vector<ShopBlock> &shopVector = getShopItemVector(player->getGUID());
+	const auto &shopVector = getShopItemVector(player->getGUID());
 	for (const ShopBlock &shopBlock : shopVector) {
 		if (itemType.id == shopBlock.itemId && shopBlock.itemSellPrice != 0) {
 			sellPrice = shopBlock.itemSellPrice;
@@ -586,7 +586,7 @@ void Npc::setPlayerInteraction(uint32_t playerId, uint16_t topicId /*= 0*/) {
 void Npc::removePlayerInteraction(std::shared_ptr<Player> player) {
 	if (playerInteractions.contains(player->getID())) {
 		playerInteractions.erase(player->getID());
-		player->closeShopWindow(true);
+		player->closeShopWindow();
 	}
 }
 
@@ -643,30 +643,26 @@ bool Npc::getRandomStep(Direction &moveDirection) {
 	return false;
 }
 
-void Npc::addShopPlayer(const std::shared_ptr<Player> &player, const std::vector<ShopBlock> &shopItems /* = {}*/) {
-	if (!player) {
-		return;
-	}
-
-	shopPlayerMap.try_emplace(player->getGUID(), shopItems);
+bool Npc::isShopPlayer(uint32_t playerGUID) const {
+	return shopPlayers.find(playerGUID) != shopPlayers.end();
 }
 
-void Npc::removeShopPlayer(const std::shared_ptr<Player> &player) {
-	if (!player) {
-		return;
-	}
+void Npc::addShopPlayer(uint32_t playerGUID) {
+	shopPlayers.insert(playerGUID);
+}
 
-	shopPlayerMap.erase(player->getGUID());
+void Npc::removeShopPlayer(uint32_t playerGUID) {
+	shopPlayers.erase(playerGUID);
 }
 
 void Npc::closeAllShopWindows() {
-	for (const auto &[playerGUID, playerPtr] : shopPlayerMap) {
-		auto shopPlayer = g_game().getPlayerByGUID(playerGUID);
-		if (shopPlayer) {
-			shopPlayer->closeShopWindow();
+	for (const auto playerGUID : shopPlayers) {
+		const auto &player = g_game().getPlayerByGUID(playerGUID);
+		if (player) {
+			player->closeShopWindow();
 		}
 	}
-	shopPlayerMap.clear();
+	shopPlayers.clear();
 }
 
 void Npc::handlePlayerMove(std::shared_ptr<Player> player, const Position &newPos) {

src/creatures/npcs/npc.hpp

@@ -95,14 +95,7 @@ class Npc final : public Creature {
 		npcType->info.currencyId = currency;
 	}
 
-	std::vector<ShopBlock> getShopItemVector(uint32_t playerGUID) {
-		if (playerGUID != 0) {
-			auto it = shopPlayerMap.find(playerGUID);
-			if (it != shopPlayerMap.end() && !it->second.empty()) {
-				return it->second;
-			}
-		}
-
+	const std::vector<ShopBlock> &getShopItemVector(uint32_t playerGUID) const {
 		return npcType->info.shopItemVector;
 	}
 
@@ -165,8 +158,10 @@ class Npc final : public Creature {
 		internalLight = npcType->info.light;
 	}
 
-	void addShopPlayer(const std::shared_ptr<Player> &player, const std::vector<ShopBlock> &shopItems = {});
-	void removeShopPlayer(const std::shared_ptr<Player> &player);
+	bool isShopPlayer(uint32_t playerGUID) const;
+
+	void addShopPlayer(uint32_t playerGUID);
+	void removeShopPlayer(uint32_t playerGUID);
 	void closeAllShopWindows();
 
 	static uint32_t npcAutoID;
@@ -184,7 +179,7 @@ class Npc final : public Creature {
 
 	std::map<uint32_t, uint16_t> playerInteractions;
 
-	phmap::flat_hash_map<uint32_t, std::vector<ShopBlock>> shopPlayerMap;
+	std::unordered_set<uint32_t> shopPlayers;
 
 	std::shared_ptr<NpcType> npcType;
 	std::shared_ptr<SpawnNpc> spawnNpc;

src/creatures/players/player.cpp

@@ -1895,31 +1895,38 @@ void Player::onRemoveCreature(std::shared_ptr<Creature> creature, bool isLogout)
 }
 
 bool Player::openShopWindow(std::shared_ptr<Npc> npc) {
+	Benchmark brenchmark;
 	if (!npc) {
 		g_logger().error("[Player::openShopWindow] - Npc is wrong or nullptr");
 		return false;
 	}
 
+	if (npc->isShopPlayer(getGUID())) {
+		g_logger().debug("[Player::openShopWindow] - Player {} is already in shop window", getName());
+		return false;
+	}
+
+	npc->addShopPlayer(getGUID());
+
 	setShopOwner(npc);
 
 	sendShop(npc);
 	std::map<uint16_t, uint16_t> inventoryMap;
 	sendSaleItemList(getAllSaleItemIdAndCount(inventoryMap));
+
+	g_logger().debug("[Player::openShopWindow] - Player {} has opened shop window in {} ms", getName(), brenchmark.duration());
 	return true;
 }
 
-bool Player::closeShopWindow(bool sendCloseShopWindow /*= true*/) {
+bool Player::closeShopWindow() {
 	if (!shopOwner) {
 		return false;
 	}
 
-	shopOwner->removeShopPlayer(static_self_cast<Player>());
+	shopOwner->removeShopPlayer(getGUID());
 	setShopOwner(nullptr);
 
-	if (sendCloseShopWindow) {
-		sendCloseShop();
-	}
-
+	sendCloseShop();
 	return true;
 }
 
@@ -4293,7 +4300,7 @@ bool Player::hasShopItemForSale(uint16_t itemId, uint8_t subType) const {
 	}
 
 	const ItemType &itemType = Item::items[itemId];
-	std::vector<ShopBlock> shoplist = shopOwner->getShopItemVector(getGUID());
+	const auto &shoplist = shopOwner->getShopItemVector(getGUID());
 	return std::any_of(shoplist.begin(), shoplist.end(), [&](const ShopBlock &shopBlock) {
 		return shopBlock.itemId == itemId && shopBlock.itemBuyPrice != 0 && (!itemType.isFluidContainer() || shopBlock.itemSubType == subType);
 	});

src/creatures/players/player.hpp

@@ -851,7 +851,7 @@ class Player final : public Creature, public Cylinder, public Bankable {
 
 	void stopWalk();
 	bool openShopWindow(std::shared_ptr<Npc> npc);
-	bool closeShopWindow(bool sendCloseShopWindow = true);
+	bool closeShopWindow();
 	bool updateSaleShopList(std::shared_ptr<Item> item);
 	bool hasShopItemForSale(uint16_t itemId, uint8_t subType) const;
 

src/lua/functions/creatures/npc/npc_functions.cpp

@@ -354,7 +354,6 @@ int NpcFunctions::luaNpcOpenShopWindow(lua_State* L) {
 		return 1;
 	}
 
-	npc->addShopPlayer(player);
 	pushBoolean(L, player->openShopWindow(npc));
 	return 1;
 }
@@ -405,9 +404,6 @@ int NpcFunctions::luaNpcOpenShopWindowTable(lua_State* L) {
 	}
 	lua_pop(L, 3);
 
-	// Close any eventual other shop window currently open.
-	player->closeShopWindow(true);
-	npc->addShopPlayer(player, items);
 	pushBoolean(L, player->openShopWindow(npc));
 	return 1;
 }
@@ -429,7 +425,7 @@ int NpcFunctions::luaNpcCloseShopWindow(lua_State* L) {
 	}
 
 	if (player->getShopOwner() == npc) {
-		player->closeShopWindow(true);
+		player->closeShopWindow();
 	}
 
 	pushBoolean(L, true);
@@ -577,7 +573,7 @@ int NpcFunctions::luaNpcSellItem(lua_State* L) {
 	}
 
 	uint64_t pricePerUnit = 0;
-	const std::vector<ShopBlock> &shopVector = npc->getShopItemVector(player->getGUID());
+	const auto &shopVector = npc->getShopItemVector(player->getGUID());
 	for (ShopBlock shopBlock : shopVector) {
 		if (itemId == shopBlock.itemId && shopBlock.itemBuyPrice != 0) {
 			pricePerUnit = shopBlock.itemBuyPrice;

src/server/network/message/networkmessage.cpp

@@ -40,9 +40,9 @@ Position NetworkMessage::getPosition() {
 	return pos;
 }
 
-void NetworkMessage::addString(const std::string &value, const std::string &function) {
+void NetworkMessage::addString(const std::string &value, const std::string &function /* = ""*/) {
 	size_t stringLen = value.length();
-	if (value.empty()) {
+	if (value.empty() && !function.empty()) {
 		g_logger().debug("[NetworkMessage::addString] - Value string is empty, function '{}'", function);
 	}
 	if (!canAdd(stringLen + 2)) {

src/server/network/message/networkmessage.hpp

@@ -90,7 +90,20 @@ class NetworkMessage {
 	void addBytes(const char* bytes, size_t size);
 	void addPaddingBytes(size_t n);
 
-	void addString(const std::string &value, const std::string &function);
+	/**
+	 * Adds a string to the network message buffer.
+	 *
+	 * @param value The string value to be added to the message buffer.
+	 * @param function * @param function An optional parameter that specifies the function name from which `addString` is invoked.
+	 * Including this enhances logging by adding the function name to the debug and error log messages.
+	 * This helps in debugging by indicating the context when issues occur, such as attempting to add an
+	 * empty string or when there are message size errors.
+	 *
+	 * When the function parameter is used, it aids in identifying the context in log messages,
+	 * making it easier to diagnose issues related to network message construction,
+	 * especially in complex systems where the same method might be called from multiple places.
+	 */
+	void addString(const std::string &value, const std::string &function = "");
 
 	void addDouble(double value, uint8_t precision = 2);
 

src/server/network/protocol/protocolgame.cpp

@@ -4637,6 +4637,7 @@ void ProtocolGame::sendLootStats(std::shared_ptr<Item> item, uint8_t count) {
 }
 
 void ProtocolGame::sendShop(std::shared_ptr<Npc> npc) {
+	Benchmark brenchmark;
 	NetworkMessage msg;
 	msg.addByte(0x7A);
 	msg.addString(npc->getName(), "ProtocolGame::sendShop - npc->getName()");
@@ -4646,20 +4647,35 @@ void ProtocolGame::sendShop(std::shared_ptr<Npc> npc) {
 		msg.addString(std::string(), "ProtocolGame::sendShop - std::string()"); // Currency name
 	}
 
-	std::vector<ShopBlock> shoplist = npc->getShopItemVector(player->getGUID());
+	const auto &shoplist = npc->getShopItemVector(player->getGUID());
 	uint16_t itemsToSend = std::min<size_t>(shoplist.size(), std::numeric_limits<uint16_t>::max());
 	msg.add<uint16_t>(itemsToSend);
 
+	// Initialize before the loop to avoid database overload on each iteration
+	auto talkactionHidden = player->kv()->get("npc-shop-hidden-sell-item");
+	// Initialize the inventoryMap outside the loop to avoid creation on each iteration
+	std::map<uint16_t, uint16_t> inventoryMap;
+	player->getAllSaleItemIdAndCount(inventoryMap);
 	uint16_t i = 0;
 	for (const ShopBlock &shopBlock : shoplist) {
 		if (++i > itemsToSend) {
 			break;
 		}
 
+		// Hidden sell items from the shop if they are not in the player's inventory
+		if (talkactionHidden && talkactionHidden->get<bool>()) {
+			const auto &foundItem = inventoryMap.find(shopBlock.itemId);
+			if (foundItem == inventoryMap.end() && shopBlock.itemSellPrice > 0 && shopBlock.itemBuyPrice == 0) {
+				AddHiddenShopItem(msg);
+				continue;
+			}
+		}
+
 		AddShopItem(msg, shopBlock);
 	}
 
 	writeToOutputBuffer(msg);
+	g_logger().debug("ProtocolGame::sendShop - Time: {} ms, shop items: {}", brenchmark.duration(), shoplist.size());
 }
 
 void ProtocolGame::sendCloseShop() {
@@ -8101,7 +8117,7 @@ void ProtocolGame::AddHiddenShopItem(NetworkMessage &msg) {
 	// Empty bytes from AddShopItem
 	msg.add<uint16_t>(0);
 	msg.addByte(0);
-	msg.addString(std::string(), "ProtocolGame::AddHiddenShopItem - std::string()");
+	msg.addString(std::string());
 	msg.add<uint32_t>(0);
 	msg.add<uint32_t>(0);
 	msg.add<uint32_t>(0);
@@ -8114,18 +8130,6 @@ void ProtocolGame::AddShopItem(NetworkMessage &msg, const ShopBlock &shopBlock)
 		return;
 	}
 
-	// Hidden sell items from the shop if they are not in the player's inventory
-	auto talkactionHidden = player->kv()->get("npc-shop-hidden-sell-item");
-	if (talkactionHidden && talkactionHidden->get<BooleanType>() == true) {
-		std::map<uint16_t, uint16_t> inventoryMap;
-		player->getAllSaleItemIdAndCount(inventoryMap);
-		auto inventoryItems = inventoryMap.find(shopBlock.itemId);
-		if (inventoryItems == inventoryMap.end() && shopBlock.itemSellPrice > 0 && shopBlock.itemBuyPrice == 0) {
-			AddHiddenShopItem(msg);
-			return;
-		}
-	}
-
 	const ItemType &it = Item::items[shopBlock.itemId];
 	msg.add<uint16_t>(shopBlock.itemId);
 	if (it.isSplash() || it.isFluidContainer()) {