Inventory: Send dirty lists where appropriate (#8742)

This change reduces the amount of sent data towards clients. Inventory lists that are already known to the player are skipped, saving quite some data over time. Raises protocol version to 38 to ensure correct backwards-compatible code.
minetest · Aug 24, 2019 · 0b4f424 · 0b4f424
1 parent 008b80f
commit 0b4f424
Show file tree

Hide file tree

Showing 16 changed files with 193 additions and 160 deletions.
diff --git a/src/client/client.cpp b/src/client/client.cpp
@@ -557,8 +557,8 @@ void Client::step(float dtime)
 		if (count_after != count_before) {
 			// Do this every <interval> seconds after TOCLIENT_INVENTORY
 			// Reset the locally changed inventory to the authoritative inventory
-			m_env.getLocalPlayer()->inventory = *m_inventory_from_server;
-			m_inventory_updated = true;
+			player->inventory = *m_inventory_from_server;
+			m_update_wielded_item = true;
 		}
 	}
 
@@ -1331,28 +1331,30 @@ void Client::setPlayerControl(PlayerControl &control)
 void Client::setPlayerItem(u16 item)
 {
 	m_env.getLocalPlayer()->setWieldIndex(item);
-	m_inventory_updated = true;
+	m_update_wielded_item = true;
 
 	NetworkPacket pkt(TOSERVER_PLAYERITEM, 2);
 	pkt << item;
 	Send(&pkt);
 }
 
-// Returns true if the inventory of the local player has been
-// updated from the server. If it is true, it is set to false.
-bool Client::getLocalInventoryUpdated()
+// Returns true once after the inventory of the local player
+// has been updated from the server.
+bool Client::updateWieldedItem()
 {
-	bool updated = m_inventory_updated;
-	m_inventory_updated = false;
-	return updated;
-}
+	if (!m_update_wielded_item)
+		return false;
+
+	m_update_wielded_item = false;
 
-// Copies the inventory of the local player to parameter
-void Client::getLocalInventory(Inventory &dst)
-{
 	LocalPlayer *player = m_env.getLocalPlayer();
 	assert(player);
-	dst = player->inventory;
+	if (auto *list = player->inventory.getList("main"))
+		list->setModified(false);
+	if (auto *list = player->inventory.getList("hand"))
+		list->setModified(false);
+
+	return true;
 }
 
 Inventory* Client::getInventory(const InventoryLocation &loc)

diff --git a/src/client/client.h b/src/client/client.h
@@ -274,9 +274,7 @@ class Client : public con::PeerHandler, public InventoryManager, public IGameDef
 
 	// Returns true if the inventory of the local player has been
 	// updated from the server. If it is true, it is set to false.
-	bool getLocalInventoryUpdated();
-	// Copies the inventory of the local player to parameter
-	void getLocalInventory(Inventory &dst);
+	bool updateWieldedItem();
 
 	/* InventoryManager interface */
 	Inventory* getInventory(const InventoryLocation &loc) override;
@@ -504,7 +502,7 @@ class Client : public con::PeerHandler, public InventoryManager, public IGameDef
 	// If 0, server init hasn't been received yet.
 	u16 m_proto_ver = 0;
 
-	bool m_inventory_updated = false;
+	bool m_update_wielded_item = false;
 	Inventory *m_inventory_from_server = nullptr;
 	float m_inventory_from_server_age = 0.0f;
 	PacketCounter m_packetcounter;

diff --git a/src/client/game.cpp b/src/client/game.cpp
@@ -599,7 +599,6 @@ struct GameRunData {
 	bool dig_instantly;
 	bool digging_blocked;
 	bool left_punch;
-	bool update_wielded_item_trigger;
 	bool reset_jump_timer;
 	float nodig_delay_timer;
 	float dig_time;
@@ -1018,7 +1017,6 @@ bool Game::startup(bool *kill,
 	// Reinit runData
 	runData = GameRunData();
 	runData.time_from_last_punch = 10.0;
-	runData.update_wielded_item_trigger = true;
 
 	m_game_ui->initFlags();
 
@@ -3736,19 +3734,11 @@ void Game::updateFrame(ProfilerGraph *graph, RunStats *stats, f32 dtime,
 	if (player->getWieldIndex() != runData.new_playeritem)
 		client->setPlayerItem(runData.new_playeritem);
 
-	// Update local inventory if it has changed
-	if (client->getLocalInventoryUpdated()) {
-		//infostream<<"Updating local inventory"<<std::endl;
-		runData.update_wielded_item_trigger = true;
-	}
-
-	if (runData.update_wielded_item_trigger) {
+	if (client->updateWieldedItem()) {
 		// Update wielded tool
 		ItemStack selected_item, hand_item;
 		ItemStack &tool_item = player->getWieldedItem(&selected_item, &hand_item);
 		camera->wield(tool_item);
-
-		runData.update_wielded_item_trigger = false;
 	}
 
 	/*

diff --git a/src/inventory.cpp b/src/inventory.cpp
@@ -20,6 +20,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "inventory.h"
 #include "serialization.h"
 #include "debug.h"
+#include <algorithm>
 #include <sstream>
 #include "log.h"
 #include "itemdef.h"
@@ -382,27 +383,32 @@ void InventoryList::clearItems()
 		m_items.emplace_back();
 	}
 
-	//setDirty(true);
+	setModified();
 }
 
 void InventoryList::setSize(u32 newsize)
 {
-	if(newsize != m_items.size())
-		m_items.resize(newsize);
+	if (newsize == m_items.size())
+		return;
+
+	m_items.resize(newsize);
 	m_size = newsize;
+	setModified();
 }
 
 void InventoryList::setWidth(u32 newwidth)
 {
 	m_width = newwidth;
+	setModified();
 }
 
 void InventoryList::setName(const std::string &name)
 {
 	m_name = name;
+	setModified();
 }
 
-void InventoryList::serialize(std::ostream &os) const
+void InventoryList::serialize(std::ostream &os, bool incremental) const
 {
 	//os.imbue(std::locale("C"));
 
@@ -415,6 +421,9 @@ void InventoryList::serialize(std::ostream &os) const
 			os<<"Item ";
 			item.serialize(os);
 		}
+		// TODO: Implement this:
+		// if (!incremental || item.checkModified())
+		// os << "Keep";
 		os<<"\n";
 	}
 
@@ -424,8 +433,8 @@ void InventoryList::serialize(std::ostream &os) const
 void InventoryList::deSerialize(std::istream &is)
 {
 	//is.imbue(std::locale("C"));
+	setModified();
 
-	clearItems();
 	u32 item_i = 0;
 	m_width = 0;
 
@@ -439,12 +448,12 @@ void InventoryList::deSerialize(std::istream &is)
 		std::string name;
 		std::getline(iss, name, ' ');
 
-		if (name == "EndInventoryList")
-			return;
-
-		// This is a temporary backwards compatibility fix
-		if (name == "end")
+		if (name == "EndInventoryList" || name == "end") {
+			// If partial incremental: Clear leftover items (should not happen!)
+			for (size_t i = item_i; i < m_items.size(); ++i)
+				m_items[i].clear();
 			return;
+		}
 
 		if (name == "Width") {
 			iss >> m_width;
@@ -464,6 +473,8 @@ void InventoryList::deSerialize(std::istream &is)
 			if(item_i > getSize() - 1)
 				throw SerializationError("too many items");
 			m_items[item_i++].clear();
+		} else if (name == "Keep") {
+			++item_i; // Unmodified item
 		}
 	}
 
@@ -557,14 +568,15 @@ ItemStack InventoryList::changeItem(u32 i, const ItemStack &newitem)
 
 	ItemStack olditem = m_items[i];
 	m_items[i] = newitem;
-	//setDirty(true);
+	setModified();
 	return olditem;
 }
 
 void InventoryList::deleteItem(u32 i)
 {
 	assert(i < m_items.size()); // Pre-condition
 	m_items[i].clear();
+	setModified();
 }
 
 ItemStack InventoryList::addItem(const ItemStack &newitem_)
@@ -612,8 +624,8 @@ ItemStack InventoryList::addItem(u32 i, const ItemStack &newitem)
 		return newitem;
 
 	ItemStack leftover = m_items[i].addItem(newitem, m_itemdef);
-	//if(leftover != newitem)
-	//	setDirty(true);
+	if (leftover != newitem)
+		setModified();
 	return leftover;
 }
 
@@ -682,8 +694,8 @@ ItemStack InventoryList::takeItem(u32 i, u32 takecount)
 		return ItemStack();
 
 	ItemStack taken = m_items[i].takeItem(takecount);
-	//if(!taken.empty())
-	//	setDirty(true);
+	if (!taken.empty())
+		setModified();
 	return taken;
 }
 
@@ -788,16 +800,6 @@ void Inventory::clear()
 	m_lists.clear();
 }
 
-void Inventory::clearContents()
-{
-	m_dirty = true;
-	for (InventoryList *list : m_lists) {
-		for (u32 j=0; j<list->getSize(); j++) {
-			list->deleteItem(j);
-		}
-	}
-}
-
 Inventory::Inventory(IItemDefManager *itemdef)
 {
 	m_dirty = false;
@@ -807,7 +809,6 @@ Inventory::Inventory(IItemDefManager *itemdef)
 Inventory::Inventory(const Inventory &other)
 {
 	*this = other;
-	m_dirty = false;
 }
 
 Inventory & Inventory::operator = (const Inventory &other)
@@ -838,19 +839,24 @@ bool Inventory::operator == (const Inventory &other) const
 	return true;
 }
 
-void Inventory::serialize(std::ostream &os) const
+void Inventory::serialize(std::ostream &os, bool incremental) const
 {
-	for (InventoryList *list : m_lists) {
-		os<<"List "<<list->getName()<<" "<<list->getSize()<<"\n";
-		list->serialize(os);
+	for (const InventoryList *list : m_lists) {
+		if (!incremental || list->checkModified()) {
+			os << "List " << list->getName() << " " << list->getSize() << "\n";
+			list->serialize(os, incremental);
+		} else {
+			os << "KeepList " << list->getName() << "\n";
+		}
 	}
 
 	os<<"EndInventory\n";
 }
 
 void Inventory::deSerialize(std::istream &is)
 {
-	clear();
+	std::vector<InventoryList *> new_lists;
+	new_lists.reserve(m_lists.size());
 
 	while (is.good()) {
 		std::string line;
@@ -861,12 +867,20 @@ void Inventory::deSerialize(std::istream &is)
 		std::string name;
 		std::getline(iss, name, ' ');
 
-		if (name == "EndInventory")
-			return;
+		if (name == "EndInventory" || name == "end") {
+			// Remove all lists that were not sent
+			for (auto &list : m_lists) {
+				if (std::find(new_lists.begin(), new_lists.end(), list) != new_lists.end())
+					continue;
 
-		// This is a temporary backwards compatibility fix
-		if (name == "end")
+				delete list;
+				list = nullptr;
+				m_dirty = true;
+			}
+			m_lists.erase(std::remove(m_lists.begin(), m_lists.end(),
+					nullptr), m_lists.end());
 			return;
+		}
 
 		if (name == "List") {
 			std::string listname;
@@ -875,15 +889,33 @@ void Inventory::deSerialize(std::istream &is)
 			std::getline(iss, listname, ' ');
 			iss>>listsize;
 
-			InventoryList *list = new InventoryList(listname, listsize, m_itemdef);
+			InventoryList *list = getList(listname);
+			bool create_new = !list;
+			if (create_new)
+				list = new InventoryList(listname, listsize, m_itemdef);
+			else
+				list->setSize(listsize);
 			list->deSerialize(is);
 
-			m_lists.push_back(list);
-		}
-		else
-		{
-			throw SerializationError("invalid inventory specifier: " + name);
+			new_lists.push_back(list);
+			if (create_new)
+				m_lists.push_back(list);
+
+		} else if (name == "KeepList") {
+			// Incrementally sent list
+			std::string listname;
+			std::getline(iss, listname, ' ');
+
+			InventoryList *list = getList(listname);
+			if (list) {
+				new_lists.push_back(list);
+			} else {
+				errorstream << "Inventory::deSerialize(): Tried to keep list '" <<
+					listname << "' which is non-existent." << std::endl;
+			}
 		}
+		// Any additional fields will throw errors when received by a client
+		// older than PROTOCOL_VERSION 38
 	}
 
 	// Contents given to deSerialize() were not terminated properly: throw error.