Client-side prediction of inventory changes, and some inventory menu fixes

8 files changed, 210 insertions, 57 deletions

diff --git a/src/client.cpp b/src/client.cpp
index 602f0cf84..0463aa81c 100644
--- a/src/client.cpp
+++ b/src/client.cpp
@@ -225,6 +225,8 @@ Client::Client(
 	m_server_ser_ver(SER_FMT_VER_INVALID),
 	m_playeritem(0),
 	m_inventory_updated(false),
+	m_inventory_from_server(NULL),
+	m_inventory_from_server_age(0.0),
 	m_time_of_day(0),
 	m_map_seed(0),
 	m_password(password),
@@ -269,6 +271,8 @@ Client::~Client()
 	m_mesh_update_thread.setRun(false);
 	while(m_mesh_update_thread.IsRunning())
 		sleep_ms(100);
+
+	delete m_inventory_from_server;
 }
 
 void Client::connect(Address address)
@@ -657,6 +661,30 @@ void Client::step(float dtime)
 			}
 		}
 	}
+
+	/*
+		If the server didn't update the inventory in a while, revert
+		the local inventory (so the player notices the lag problem
+		and knows something is wrong).
+	*/
+	if(m_inventory_from_server)
+	{
+		float interval = 10.0;
+		float count_before = floor(m_inventory_from_server_age / interval);
+
+		m_inventory_from_server_age += dtime;
+
+		float count_after = floor(m_inventory_from_server_age / interval);
+
+		if(count_after != count_before)
+		{
+			// Do this every <interval> seconds after TOCLIENT_INVENTORY
+			// Reset the locally changed inventory to the authoritative inventory
+			Player *player = m_env.getLocalPlayer();
+			player->inventory = *m_inventory_from_server;
+			m_inventory_updated = true;
+		}
+	}
 }
 
 // Virtual methods from con::PeerHandler
@@ -975,6 +1003,10 @@ void Client::ProcessData(u8 *data, u32 datasize, u16 sender_peer_id)
 
 			m_inventory_updated = true;
 
+			delete m_inventory_from_server;
+			m_inventory_from_server = new Inventory(player->inventory);
+			m_inventory_from_server_age = 0.0;
+
 			//infostream<<"Client got player inventory:"<<std::endl;
 			//player->inventory.print(infostream);
 		}
@@ -1931,7 +1963,15 @@ Inventory* Client::getInventory(const InventoryLocation &loc)
 }
 void Client::inventoryAction(InventoryAction *a)
 {
+	/*
+		Send it to the server
+	*/
 	sendInventoryAction(a);
+
+	/*
+		Predict some local inventory changes
+	*/
+	a->clientApply(this, this);
 }
 
 ClientActiveObject * Client::getSelectedActiveObject(
diff --git a/src/client.h b/src/client.h
index 6d5e5c525..efdf315f7 100644
--- a/src/client.h
+++ b/src/client.h
@@ -367,6 +367,8 @@ private:
 	u8 m_server_ser_ver;
 	u16 m_playeritem;
 	bool m_inventory_updated;
+	Inventory *m_inventory_from_server;
+	float m_inventory_from_server_age;
 	core::map<v3s16, bool> m_active_blocks;
 	PacketCounter m_packetcounter;
 	// Received from the server. 0-23999
diff --git a/src/guiInventoryMenu.cpp b/src/guiInventoryMenu.cpp
index a4b16b251..cd371d062 100644
--- a/src/guiInventoryMenu.cpp
+++ b/src/guiInventoryMenu.cpp
@@ -537,6 +537,11 @@ bool GUIInventoryMenu::OnEvent(const SEvent& event)
 				s.i = -1;  // make it invalid again
 		}
 
+		bool identical = (m_selected_item != NULL) && s.isValid() &&
+			(inv_selected == inv_s) &&
+			(m_selected_item->listname == s.listname) &&
+			(m_selected_item->i == s.i);
+
 		// buttons: 0 = left, 1 = right, 2 = middle
 		// up/down: 0 = down (press), 1 = up (release), 2 = unknown event
 		int button = 0;
@@ -602,13 +607,26 @@ bool GUIInventoryMenu::OnEvent(const SEvent& event)
 
 				if(s.isValid())
 				{
-					// Clicked another slot: move
+					// Clicked a slot: move
 					if(button == 1)  // right
 						move_amount = 1;
 					else if(button == 2)  // middle
 						move_amount = MYMIN(m_selected_amount, 10);
 					else  // left
 						move_amount = m_selected_amount;
+					dstream << "move_amount=" << move_amount<<"\n";
+					dstream << "m_selected_amount=" << m_selected_amount<<"\n";
+
+					if(identical)
+					{
+						if(move_amount >= m_selected_amount)
+							m_selected_amount = 0;
+						else
+							m_selected_amount -= move_amount;
+						move_amount = 0;
+					}
+					dstream << "move_amount=" << move_amount<<"\n";
+					dstream << "m_selected_amount=" << m_selected_amount<<"\n";
 				}
 				else if(getAbsoluteClippingRect().isPointInside(m_pointer))
 				{
@@ -636,9 +654,7 @@ bool GUIInventoryMenu::OnEvent(const SEvent& event)
 
 			if(m_selected_item != NULL && m_selected_dragging && s.isValid())
 			{
-				if((inv_selected != inv_s) ||
-					(m_selected_item->listname != s.listname) ||
-					(m_selected_item->i != s.i))
+				if(!identical)
 				{
 					// Dragged to different slot: move all selected
 					move_amount = m_selected_amount;
@@ -675,18 +691,19 @@ bool GUIInventoryMenu::OnEvent(const SEvent& event)
 			if(leftover.count == stack_from.count)
 			{
 				// Swap the stacks
+				m_selected_amount -= stack_to.count;
 			}
 			else if(leftover.empty())
 			{
 				// Item fits
+				m_selected_amount -= move_amount;
 			}
 			else
 			{
 				// Item only fits partially
 				move_amount -= leftover.count;
+				m_selected_amount -= move_amount;
 			}
-			assert(move_amount > 0 && move_amount <= m_selected_amount);
-			m_selected_amount -= move_amount;
 
 			infostream<<"Handing IACTION_MOVE to manager"<<std::endl;
 			IMoveAction *a = new IMoveAction();
@@ -741,6 +758,7 @@ bool GUIInventoryMenu::OnEvent(const SEvent& event)
 		{
 			delete m_selected_item;
 			m_selected_item = NULL;
+			m_selected_amount = 0;
 			m_selected_dragging = false;
 		}
 	}
diff --git a/src/inventory.cpp b/src/inventory.cpp
index ebd0b9c23..66101b831 100644
--- a/src/inventory.cpp
+++ b/src/inventory.cpp
@@ -733,6 +733,49 @@ ItemStack InventoryList::peekItem(u32 i, u32 peekcount) const
 	return m_items[i].peekItem(peekcount);
 }
 
+void InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i, u32 count)
+{
+	if(this == dest && i == dest_i)
+		return;
+
+	// Take item from source list
+	ItemStack item1;
+	if(count == 0)
+		item1 = changeItem(i, ItemStack());
+	else
+		item1 = takeItem(i, count);
+
+	if(item1.empty())
+		return;
+
+	// Try to add the item to destination list
+	u32 oldcount = item1.count;
+	item1 = dest->addItem(dest_i, item1);
+
+	// If something is returned, the item was not fully added
+	if(!item1.empty())
+	{
+		// If olditem is returned, nothing was added.
+		bool nothing_added = (item1.count == oldcount);
+
+		// If something else is returned, part of the item was left unadded.
+		// Add the other part back to the source item
+		addItem(i, item1);
+
+		// If olditem is returned, nothing was added.
+		// Swap the items
+		if(nothing_added)
+		{
+			// Take item from source list
+			item1 = changeItem(i, ItemStack());
+			// Adding was not possible, swap the items.
+			ItemStack item2 = dest->changeItem(dest_i, item1);
+			// Put item from destination list to the source list
+			changeItem(i, item2);
+		}
+	}
+}
+
 /*
 	Inventory
 */
diff --git a/src/inventory.h b/src/inventory.h
index 3f5d83589..fcac5f647 100644
--- a/src/inventory.h
+++ b/src/inventory.h
@@ -233,6 +233,10 @@ public:
 	// Similar to takeItem, but keeps the slot intact.
 	ItemStack peekItem(u32 i, u32 peekcount) const;
 
+	// Move an item to a different list (or a different stack in the same list)
+	// count is the maximum number of items to move (0 for everything)
+	void moveItem(u32 i, InventoryList *dest, u32 dest_i, u32 count = 0);
+
 private:
 	std::vector<ItemStack> m_items;
 	u32 m_size;
diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp
index b42a80673..b04a1c177 100644
--- a/src/inventorymanager.cpp
+++ b/src/inventorymanager.cpp
@@ -133,6 +133,10 @@ InventoryAction * InventoryAction::deSerialize(std::istream &is)
 	return a;
 }
 
+/*
+	IMoveAction
+*/
+
 IMoveAction::IMoveAction(std::istream &is)
 {
 	std::string ts;
@@ -193,59 +197,13 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 				<<", to_list=\""<<to_list<<"\""<<std::endl;
 		return;
 	}
-	if(list_from->getItem(from_i).empty())
-	{
-		infostream<<"IMoveAction::apply(): FAIL: source item not found: "
-				<<"from_inv=\""<<from_inv.dump()<<"\""
-				<<", from_list=\""<<from_list<<"\""
-				<<" from_i="<<from_i<<std::endl;
-		return;
-	}
 	/*
-		If the source and the destination slots are the same
-	*/
-	if(inv_from == inv_to && list_from == list_to && from_i == to_i)
-	{
-		infostream<<"IMoveAction::apply(): FAIL: source and destination slots "
-				<<"are the same: inv=\""<<from_inv.dump()
-				<<"\" list=\""<<from_list
-				<<"\" i="<<from_i<<std::endl;
-		return;
-	}
-	
-	// Take item from source list
-	ItemStack item1;
-	if(count == 0)
-		item1 = list_from->changeItem(from_i, ItemStack());
-	else
-		item1 = list_from->takeItem(from_i, count);
+		This performs the actual movement
 
-	// Try to add the item to destination list
-	int oldcount = item1.count;
-	item1 = list_to->addItem(to_i, item1);
-
-	// If something is returned, the item was not fully added
-	if(!item1.empty())
-	{
-		// If olditem is returned, nothing was added.
-		bool nothing_added = (item1.count == oldcount);
-		
-		// If something else is returned, part of the item was left unadded.
-		// Add the other part back to the source item
-		list_from->addItem(from_i, item1);
-
-		// If olditem is returned, nothing was added.
-		// Swap the items
-		if(nothing_added)
-		{
-			// Take item from source list
-			item1 = list_from->changeItem(from_i, ItemStack());
-			// Adding was not possible, swap the items.
-			ItemStack item2 = list_to->changeItem(to_i, item1);
-			// Put item from destination list to the source list
-			list_from->changeItem(from_i, item2);
-		}
-	}
+		If something is wrong (source item is empty, destination is the
+		same as source), nothing happens
+	*/
+	list_from->moveItem(from_i, list_to, to_i, count);
 
 	mgr->setInventoryModified(from_inv);
 	if(inv_from != inv_to)
@@ -262,6 +220,38 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 			<<std::endl;
 }
 
+void IMoveAction::clientApply(InventoryManager *mgr, IGameDef *gamedef)
+{
+	// Optional InventoryAction operation that is run on the client
+	// to make lag less apparent.
+
+	Inventory *inv_from = mgr->getInventory(from_inv);
+	Inventory *inv_to = mgr->getInventory(to_inv);
+	if(!inv_from || !inv_to)
+		return;
+
+	InventoryLocation current_player;
+	current_player.setCurrentPlayer();
+	Inventory *inv_player = mgr->getInventory(current_player);
+	if(inv_from != inv_player || inv_to != inv_player)
+		return;
+
+	InventoryList *list_from = inv_from->getList(from_list);
+	InventoryList *list_to = inv_to->getList(to_list);
+	if(!list_from || !list_to)
+		return;
+
+	list_from->moveItem(from_i, list_to, to_i, count);
+
+	mgr->setInventoryModified(from_inv);
+	if(inv_from != inv_to)
+		mgr->setInventoryModified(to_inv);
+}
+
+/*
+	IDropAction
+*/
+
 IDropAction::IDropAction(std::istream &is)
 {
 	std::string ts;
@@ -338,6 +328,37 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 			<<std::endl;
 }
 
+void IDropAction::clientApply(InventoryManager *mgr, IGameDef *gamedef)
+{
+	// Optional InventoryAction operation that is run on the client
+	// to make lag less apparent.
+
+	Inventory *inv_from = mgr->getInventory(from_inv);
+	if(!inv_from)
+		return;
+
+	InventoryLocation current_player;
+	current_player.setCurrentPlayer();
+	Inventory *inv_player = mgr->getInventory(current_player);
+	if(inv_from != inv_player)
+		return;
+
+	InventoryList *list_from = inv_from->getList(from_list);
+	if(!list_from)
+		return;
+
+	if(count == 0)
+		list_from->changeItem(from_i, ItemStack());
+	else
+		list_from->takeItem(from_i, count);
+
+	mgr->setInventoryModified(from_inv);
+}
+
+/*
+	ICraftAction
+*/
+
 ICraftAction::ICraftAction(std::istream &is)
 {
 	std::string ts;
@@ -412,6 +433,12 @@ void ICraftAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGam
 			<<std::endl;
 }
 
+void ICraftAction::clientApply(InventoryManager *mgr, IGameDef *gamedef)
+{
+	// Optional InventoryAction operation that is run on the client
+	// to make lag less apparent.
+}
+
 
 // Crafting helper
 bool getCraftingResult(Inventory *inv, ItemStack& result,
diff --git a/src/inventorymanager.h b/src/inventorymanager.h
index 890d05168..55e8f8402 100644
--- a/src/inventorymanager.h
+++ b/src/inventorymanager.h
@@ -102,6 +102,7 @@ struct InventoryAction
 	virtual void serialize(std::ostream &os) const = 0;
 	virtual void apply(InventoryManager *mgr, ServerActiveObject *player,
 			IGameDef *gamedef) = 0;
+	virtual void clientApply(InventoryManager *mgr, IGameDef *gamedef) = 0;
 };
 
 struct IMoveAction : public InventoryAction
@@ -142,6 +143,8 @@ struct IMoveAction : public InventoryAction
 	}
 
 	void apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef);
+
+	void clientApply(InventoryManager *mgr, IGameDef *gamedef);
 };
 
 struct IDropAction : public InventoryAction
@@ -175,6 +178,8 @@ struct IDropAction : public InventoryAction
 	}
 
 	void apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef);
+
+	void clientApply(InventoryManager *mgr, IGameDef *gamedef);
 };
 
 struct ICraftAction : public InventoryAction
@@ -203,6 +208,8 @@ struct ICraftAction : public InventoryAction
 	}
 
 	void apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef);
+
+	void clientApply(InventoryManager *mgr, IGameDef *gamedef);
 };
 
 // Crafting helper
diff --git a/src/server.cpp b/src/server.cpp
index 177551881..a0c8a0092 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -2373,6 +2373,11 @@ void Server::ProcessData(u8 *data, u32 datasize, u16 peer_id)
 		}
 
 		/*
+			Note: Always set inventory not sent, to repair cases
+			where the client made a bad prediction.
+		*/
+
+		/*
 			Handle restrictions and special cases of the move action
 		*/
 		if(a->getType() == IACTION_MOVE)
@@ -2382,6 +2387,9 @@ void Server::ProcessData(u8 *data, u32 datasize, u16 peer_id)
 			ma->from_inv.applyCurrentPlayer(player->getName());
 			ma->to_inv.applyCurrentPlayer(player->getName());
 
+			setInventoryModified(ma->from_inv);
+			setInventoryModified(ma->to_inv);
+
 			bool from_inv_is_current_player =
 				(ma->from_inv.type == InventoryLocation::PLAYER) &&
 				(ma->from_inv.name == player->getName());
@@ -2461,6 +2469,8 @@ void Server::ProcessData(u8 *data, u32 datasize, u16 peer_id)
 
 			da->from_inv.applyCurrentPlayer(player->getName());
 
+			setInventoryModified(da->from_inv);
+
 			// Disallow dropping items if not allowed to interact
 			if((getPlayerPrivs(player) & PRIV_INTERACT) == 0)
 			{
@@ -2490,6 +2500,8 @@ void Server::ProcessData(u8 *data, u32 datasize, u16 peer_id)
 
 			ca->craft_inv.applyCurrentPlayer(player->getName());
 
+			setInventoryModified(ca->craft_inv);
+
 			//bool craft_inv_is_current_player =
 			//	(ca->craft_inv.type == InventoryLocation::PLAYER) &&
 			//	(ca->craft_inv.name == player->getName());