Inventory: Properly revert client predictions (#8945)

Caused by incremental inventory sending Previously everything was overwritten by serializing the entire inventory
minetest · Sep 18, 2019 · 94a5df7 · 94a5df7
1 parent 05a7da6
commit 94a5df7
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 11 deletions.
diff --git a/src/inventory.cpp b/src/inventory.cpp
@@ -785,17 +785,17 @@ Inventory::~Inventory()
 
 void Inventory::clear()
 {
-	m_dirty = true;
 	for (auto &m_list : m_lists) {
 		delete m_list;
 	}
 	m_lists.clear();
+	setModified();
 }
 
 Inventory::Inventory(IItemDefManager *itemdef)
 {
-	m_dirty = false;
 	m_itemdef = itemdef;
+	setModified();
 }
 
 Inventory::Inventory(const Inventory &other)
@@ -808,12 +808,12 @@ Inventory & Inventory::operator = (const Inventory &other)
 	// Gracefully handle self assignment
 	if(this != &other)
 	{
-		m_dirty = true;
 		clear();
 		m_itemdef = other.m_itemdef;
 		for (InventoryList *list : other.m_lists) {
 			m_lists.push_back(new InventoryList(*list));
 		}
+		setModified();
 	}
 	return *this;
 }
@@ -833,6 +833,7 @@ bool Inventory::operator == (const Inventory &other) const
 
 void Inventory::serialize(std::ostream &os, bool incremental) const
 {
+	//std::cout << "Serialize " << (int)incremental << ", n=" << m_lists.size() << std::endl;
 	for (const InventoryList *list : m_lists) {
 		if (!incremental || list->checkModified()) {
 			os << "List " << list->getName() << " " << list->getSize() << "\n";
@@ -867,7 +868,7 @@ void Inventory::deSerialize(std::istream &is)
 
 				delete list;
 				list = nullptr;
-				m_dirty = true;
+				setModified();
 			}
 			m_lists.erase(std::remove(m_lists.begin(), m_lists.end(),
 					nullptr), m_lists.end());
@@ -920,7 +921,7 @@ void Inventory::deSerialize(std::istream &is)
 
 InventoryList * Inventory::addList(const std::string &name, u32 size)
 {
-	m_dirty = true;
+	setModified();
 	s32 i = getListIndex(name);
 	if(i != -1)
 	{
@@ -966,7 +967,8 @@ bool Inventory::deleteList(const std::string &name)
 	s32 i = getListIndex(name);
 	if(i == -1)
 		return false;
-	m_dirty = true;
+
+	setModified();
 	delete m_lists[i];
 	m_lists.erase(m_lists.begin() + i);
 	return true;

diff --git a/src/inventory.h b/src/inventory.h
@@ -323,11 +323,14 @@ class Inventory
 		return false;
 	}
 
-	inline void setModified(bool dirty)
+	inline void setModified(bool dirty = true)
 	{
 		m_dirty = dirty;
-		for (const auto &list : m_lists)
-			list->setModified(dirty);
+		// Set all as handled
+		if (!dirty) {
+			for (const auto &list : m_lists)
+				list->setModified(dirty);
+		}
 	}
 private:
 	// -1 if not found

diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp
@@ -348,6 +348,13 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 
 	/* If no items will be moved, don't go further */
 	if (count == 0) {
+		// Undo client prediction. See 'clientApply'
+		if (from_inv.type == InventoryLocation::PLAYER)
+			list_from->setModified();
+
+		if (to_inv.type == InventoryLocation::PLAYER)
+			list_to->setModified();
+
 		infostream<<"IMoveAction::apply(): move was completely disallowed:"
 				<<" count="<<old_count
 				<<" from inv=\""<<from_inv.dump()<<"\""
@@ -658,8 +665,10 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 
 		if (actually_dropped_count == 0) {
 			infostream<<"Actually dropped no items"<<std::endl;
-			// Revert client prediction
-			mgr->setInventoryModified(from_inv);
+
+			// Revert client prediction. See 'clientApply'
+			if (from_inv.type == InventoryLocation::PLAYER)
+				list_from->setModified();
 			return;
 		}