Client-Server Item Desyncronization Bugs

[Uleat]

Ok, sadly it appears that EQ Editor needs alot more work... I finally got to a corpse that had ALL of my equip and
it reported all slots and itemids as '0' as well...

Anyways... Back to trying to find out what is crashing the client.

[Uleat]

I gave up short term on the blob data..was taking too much time.

Instead, I just added a command to view the server data of a particular corpse.


What I found was (I left my cursor item prior to crash in place):

- item: 13201 -> equipslot: 30 -> lootslot: 27
- item: 13202 -> equipslot: 8000 -> lootslot: 65535
- item: 13203 -> equipslot: 8001 -> lootslot: 65535
- item: 13204 -> equipslot: 8002 -> lootslot: 65535


With my testing with my 'zopp' command, I saw that the client does not recognize the 8000-series of slots.
When a packet is sent in that range, the client just ignores it..meaning moving/deleting the cursor item
will not 'bump' the next item up if you assume that 8001 is the next client slot.

With that being obvious, 'MoveItemToCorpse' is calling 'DeleteItemInInventory.' This function, among other
things, sends a client delete item packet. I'm not sure if the equipslot or lootslot ID is causing the crash at this
point, but I'm still working on it.


If anyone has any idea on why the lootitem slots are 65535 and/or the possibility of the slot 8000 range
causing client crashes, you could save me a whole bunch of time. Every time I find something new, I have to
stop to find and learn why something is causing that behavior...

[Uleat]

I believe the bugged corpse issue can be resolved soon.

I found what needed to be changed to allow ALL items to be seen by the client and now all of my corpses are
COMPLETELY lootable.


However... There is a duplication problem that this fix brings to light.

Currently, when a player logs back in after the crash (dying with queued cursor items,) those items are back on the cursor.

Now, when a player loots their corpse, those items are there again as well. This is what was going on behind the scenes
with the current code, but was being blocked as well.


Now I need to pin-down the client crash (I have, but need to ensure no other calls are affected) and the duplication cause.

In addition, I need to resolve any issues with a queued cursor bag that has contents..I'll probably just push them to the
cursor so they are not lost (rememeber, queued cursor slots do not have queued bag slots.)

[Caryatis]

I'm sure eventually you will actually produce something useful but seriously man, 8 posts by you in a row like an hour apart. If you want to start a twitter feed to track your rambling around this problem, feel free but nobody needs to know that you havent used Georges tools and just got an out of date program to kinda work but then turns out it wasnt needed at all because that was a circuitous route around the actual issue.

You might take offense to this but you don't seem experienced enough to be tackling this all at once. I don't see why you don't break out each bit into its own diff and finalize those. You got some test code here, other code there and your scope keeps growing every post, at this rate you will never finish.

The other thing to take into account is that this thread doesn't really inspire confidence in your final product, so if you dump a 10k line diff at the end, that is going to have to be combed over like crazy before a commit since this is a very exploit prone area and nobody likes to go through a huge ass diff like that, shit it can take like a month to get a multi line diff added around here.

[Uleat]

I appreciate your candor Caryatis, and you are probably right on all counts. I am mature enough not to take criticism
personally, though.


I have devised a fairly decent system of keeping track of this mess. Any function that I modify is considered a 'Change.'
When changes must be deployed together, it becomes a 'Patch.'


As far as patches in this thread, it is not my intent that they be implemented. I will start a new one over in server code
submission specifically for a finalized product. Anything here is mainly for review and suggestions.

I will post a 'support' patch soon. The other issues, like bandolier swaps, arrow stacking issues, etc... will come later since
they are the most intrusive into the kernel.


I do tend to get a little 'manic' at times, so I'll try to do a better job of monitoring my 'tweet' rate!
</points to my signature block>

[Uleat]

Ok, I think I have got a working fix for the bugged corpses.

I fixed the 'invisible item' (bugged corpses) and client crash issues with queued cursor items.

I have even somewhat fixed the new item duplication bug that arose from that.


I am now looking for a way to clear the client-side cursor queue when your dead. I can remove the immediate cursor item,
but when a player is dead, the queue doesn't update and I end up with 'phantom' items when a player is revived.
Logging out and back in resets the client, so there is no problem there.

Any ideas on how to do this? I don't want to 'Kick()' them everytime they die with items on their cursor...

[Uleat]

Got a couple of questions, if anyone happens to know an answer or two...


With what I can gleen from from the code, it appears the server player profile is based on the Titanium model with translations
(Encode/Decode) and adaptations applied per client.

Are there any instances where this is not true?


I've also been playing around with the bugged corpse issue and have had some luck. There are two specific things I can't
deduce and wonder if different clients are handling things differently.


I've added code to basically allow any item that is not assigned to a bag slot to show up in the corpse list..but not necessarily
add them when the corpse is created.

Should the trader slots be a part of corpse creation? My testing shows that trade items are put back into the player's
inventory after the corpse is processed. (The server code does not currently process trader slots to a corpse.)


I'm also having trouble clearing the cursor of a dead player client-side. I can clear the server and database with no issues,
but the client won't release them until revived.

Do all clients have this behavior? SoF does, but the server IS currently coded to move cursor queue items to the corpse. (source of bugged corpses)
I can do a work-around, but I need to know what the desired behavior should be. (Leave queue items on cursor, or force
zone change event on rez/revive..like Evac does.)


Thanks!

[sorvani]

you are forgetting that the death hover window now exists. it is generally turned off because it was causing issues itself. but one would think that since all the clients are designed for the hover, that may be one reason some things do not clean up (client side) like you are thinking.

test trades. make a second toon, open a trade window then #kill it.

[Uleat]

Yeah, cancelled trades are definitely processed after the corpse is created. I see the item pop back into my inventory about
a half-a-second after I use #kill.

Setting 'Hover' mode to false definitely eliminates the issue since forcing a zone change event is what I was wanting to
use to correct the situation. The only problem is trying to figure out how to handle the CSD associated with it when
'Hover' is set to true. It's more cosmetic than anything since trying to place the 'phantom' items in your inventory results
in the server deleting them anyways.


I'll still leave that question out there in case someone has a decent idea of how to handle that.

I want to add a bit more code to let the player know if the corpse contains 'bugged' items and then tidy it up a
little bit, then I'll post an alpha patch here to let you guys see where this is going.

[Uleat]

Ok, here is an ALPHA patch for bugged corpses. It may still need a little work, but most issues should be ironed out:

[CSD Patch 4]

Index: inventory.cpp
===================================================================
--- inventory.cpp	(revision 2175)
+++ inventory.cpp	(working copy)
@@ -397,9 +405,11 @@
 		LogFile->write(EQEMuLog::Debug, "DeleteItemInInventory(%i, %i, %s)", slot_id, quantity, (client_update) ? "true":"false");
 	#endif
 
-	if(!m_inv[slot_id]) {
+	// Added 'IsSlotValid(slot_id)' check to both segments of client packet processing.
+	// Cursor queue slots were slipping through and crashing client
+	if(!m_inv[slot_id]) { 
 		// Make sure the client deletes anything in this slot to match the server.
-		if(client_update) {
+		if(client_update && IsValidSlot(slot_id)) {
 			EQApplicationPacket* outapp;
 			outapp = new EQApplicationPacket(OP_DeleteItem, sizeof(DeleteItem_Struct));
 			DeleteItem_Struct* delitem	= (DeleteItem_Struct*)outapp->pBuffer;
@@ -427,7 +437,7 @@
 		    database.SaveInventory(character_id, inst, slot_id);
 	}
 
-	if(client_update) {
+	if(client_update && IsValidSlot(slot_id)) { 
 		EQApplicationPacket* outapp;
 		if(inst) {
 			if(!inst->IsStackable() && !isDeleted) 
@@ -437,13 +447,13 @@
 				// Stackable, arrows, etc ? Delete one from the stack
 				outapp = new EQApplicationPacket(OP_DeleteItem, sizeof(MoveItem_Struct));
 
-			DeleteItem_Struct* delitem	= (DeleteItem_Struct*)outapp->pBuffer;
-			delitem->from_slot			= slot_id;
-			delitem->to_slot			= 0xFFFFFFFF;
-			delitem->number_in_stack	= 0xFFFFFFFF;
-			for(int loop=0;loop<quantity;loop++)
-				QueuePacket(outapp);
-			safe_delete(outapp);
+				DeleteItem_Struct* delitem	= (DeleteItem_Struct*)outapp->pBuffer;
+				delitem->from_slot			= slot_id;
+				delitem->to_slot			= 0xFFFFFFFF;
+				delitem->number_in_stack	= 0xFFFFFFFF;
+				for(int loop=0;loop<quantity;loop++)
+					QueuePacket(outapp);
+				safe_delete(outapp);
 		}
 		else {
 			outapp = new EQApplicationPacket(OP_MoveItem, sizeof(MoveItem_Struct));
Index: PlayerCorpse.cpp
===================================================================
--- PlayerCorpse.cpp	(revision 2175)
+++ PlayerCorpse.cpp	(working copy)
@@ -352,13 +352,20 @@
 		pp->silver = 0;
 		pp->gold = 0;
 		pp->platinum = 0;
+
+		// need to tell client that cash has changed..could be RespawnFromHover=true..need to check
+		// issue looks like a money duplication error, but server value is actually correct
 	
 		// get their tints
 		memcpy(item_tint, &client->GetPP().item_tint, sizeof(item_tint));
 	
 		// solar: TODO soulbound items need not be added to corpse, but they need
 		// to go into the regular slots on the player, out of bags
-	
+		
+		// personal and cursor bag slots (251-340) are moved to corpse..should be deleting db entries too.
+		// reworked code to return and merge a list for the query builder instead of adding MoveItemToCorpse
+		// code to each loop.
+
 		// worn + inventory + cursor
         std::list<uint32> removed_list;
         bool cursor = false;
@@ -367,23 +374,27 @@
 			item = client->GetInv().GetItem(i);
 			if((item && (!client->IsBecomeNPC())) || (item && client->IsBecomeNPC() && !item->GetItem()->NoRent))
 			{
-				MoveItemToCorpse(client, item, i);
-                removed_list.push_back(i);
+				removed_list.merge(MoveItemToCorpse(client, item, i));   
 			}
 		}
 
 		// cursor queue
+		// bumped starting assignment to 8001 because any in-memory 'slot 8000' item was moved above as 'slot 30'
+		// this was mainly for client profile state reflection..should match db player inventory entries now.
 		iter_queue it;
-		for(it=client->GetInv().cursor_begin(),i=8000; it!=client->GetInv().cursor_end(); it++,i++) {
+		for(it=client->GetInv().cursor_begin(),i=8001; it!=client->GetInv().cursor_end(); it++,i++) {
 			item = *it;
 			if((item && (!client->IsBecomeNPC())) || (item && client->IsBecomeNPC() && !item->GetItem()->NoRent))
 			{
-				MoveItemToCorpse(client, item, i);
+				removed_list.merge(MoveItemToCorpse(client, item, i));
                 cursor = true;
 			}
 		}
 		
-        if(removed_list.size() != 0) {
+		// client->DeleteItemInInventory is memory only..update_db is passed as false..not sure why unless it's a
+		// sync issue... client->Save() alone doesn't appear to handle inventory deletion with &inv[slot_id] being
+		// null. player inventory db entries did not reflect client.inv states after 'client->Save()' alone.
+		if(removed_list.size() != 0) {
             std::stringstream ss("");
             ss << "DELETE FROM inventory WHERE charid=" << client->CharacterID();
             ss << " AND (";
@@ -401,14 +412,40 @@
             ss << ")";
             database.RunQuery(ss.str().c_str(), ss.str().length());
         }
-        
+
         if(cursor) {
-            std::list<ItemInst*>::const_iterator start = client->GetInv().cursor_begin();
-            std::list<ItemInst*>::const_iterator finish = client->GetInv().cursor_end();
-            database.SaveCursor(client->CharacterID(), 
-                start, finish);
+			// cycle cursor to clear memory queue..will delete server queue, but not client queue with hover enabled...
+			// if can figure out client issue, change argument 3 (client_update) to false if handled elsewhere.
+			// might could solve by forcing zone entry event to reset client inventory or sending client clear and
+			// resending bulkinventory. don't want to send client delete cursor on revive..might cause item loss...
+
+			while(!client->GetInv().CursorEmpty()) {
+				if (RuleB(Character, RespawnFromHover)) {
+					client->Message(0, "Attempting to delete %s from cursor.", client->GetInv().GetItem(SLOT_CURSOR)->GetItem()->Name);
+					client->DeleteItemInInventory(SLOT_CURSOR, 0, true, false);
+				}
+				else {
+					client->DeleteItemInInventory(SLOT_CURSOR, 0, false, false); // no need to update client without hover
+				}
+			}
+
+			if (RuleB(Character, RespawnFromHover)) {
+				client->Message(13, "Warning: Your cursor may contain duplicate items not found on the server!");
+				client->Message(0, "Place these items into an empty inventory slot to resolve this problem.");
+			}
+
+			// this code didn't appear to accomplish anything db-wise. it looks like it just re-saved the cursor
+			// queue data. with this code in-place, the cursor queue was re-saved and prevented lost items..which
+			// were the items on the corpses causing the 'bugged' issue in the first place. this code can be
+			// deleted after testing and verification of code submission.
+
+            //std::list<ItemInst*>::const_iterator start = client->GetInv().cursor_begin();
+            //std::list<ItemInst*>::const_iterator finish = client->GetInv().cursor_end();
+            //database.SaveCursor(client->CharacterID(), 
+            //    start, finish);
         }
 
+		client->CalcBonuses(); // shouldn't matter to a corpse, but hey..at least Magelo will update correctly =)
 		client->Save();
 	} //end "not leaving naked corpses"
 	
@@ -417,28 +454,35 @@
 }
 
 // solar: helper function for client corpse constructor
-void Corpse::MoveItemToCorpse(Client *client, ItemInst *item, sint16 equipslot)
+std::list<uint32> Corpse::MoveItemToCorpse(Client *client, ItemInst *item, sint16 equipslot)
 {
 	int bagindex;
 	sint16 interior_slot;
 	ItemInst *interior_item;
+	std::list<uint32> returnlist;
 
 	AddItem(item->GetItem()->ID, item->GetCharges(), equipslot, item->GetAugmentItemID(0), item->GetAugmentItemID(1), item->GetAugmentItemID(2), item->GetAugmentItemID(3), item->GetAugmentItemID(4));
-	if(item->IsType(ItemClassContainer))
+	returnlist.push_back(equipslot);
+
+	// Qualified bag slot iterations. processing bag slots that don't exist is not a good idea.
+	if(item->IsType(ItemClassContainer) && ((equipslot >= 22 && equipslot <=30))) // Limit the bag check to inventory and cursor slots.
 	{
 		for(bagindex = 0; bagindex <= 10; bagindex++)
 		{
+			// For empty bags in cursor queue, slot was being resolved as SLOT_INVALID (-1)
 			interior_slot = Inventory::CalcSlotId(equipslot, bagindex);
 			interior_item = client->GetInv().GetItem(interior_slot);
 
 			if(interior_item)
 			{
 				AddItem(interior_item->GetItem()->ID, interior_item->GetCharges(), interior_slot, interior_item->GetAugmentItemID(0), interior_item->GetAugmentItemID(1), interior_item->GetAugmentItemID(2), interior_item->GetAugmentItemID(3), interior_item->GetAugmentItemID(4));
+				returnlist.push_back(Inventory::CalcSlotId(equipslot, bagindex));
 				client->DeleteItemInInventory(interior_slot, 0, true, false);
 			}
 		}
-	}
+	} // */
 	client->DeleteItemInInventory(equipslot, 0, true, false);
+	return returnlist;
 }
 
 // To be called from LoadFromDBData
@@ -981,19 +1025,43 @@
 		ItemList::iterator cur,end;
 		cur = itemlist.begin();
 		end = itemlist.end();
+
+		// Observed some odd behavior concerning the last corpse slot as coded (SoF client in this case.)
+		// Clicking index 29 (zero-based) with an item in index 28 results in retrieving index 28 item.
+		// Clicking index 29 with no item in index 28 results in the closing of the corpse loot window.
+
+		int corpselootlimit = 30; // 30 is the original value
+		/* need actual corpse limit values per client (or client range)..if always 30, then these con checks are unneeded
+		// enumeration shouldn't be needed unless someone finds a use for this info elsewhere
+		if (client->GetClientVersion()>=EQClientVoA)
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClientHoT)
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClientUnderfoot)
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClientSoD)
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClientSoF) // SoF has 32 visible slots..change untested
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClientTitanium)
+			corpselootlimit=30;
+		else if (client->GetClientVersion()>=EQClient62)
+			corpselootlimit=30;
+		else
+			corpselootlimit=30; // */
+
 		for(; cur != end; cur++) {
 			ServerLootItem_Struct* item_data = *cur;
 			item_data->lootslot = 0xFFFF;
+			
+			// Dont display the item if it's in a bag
 
-			// Dont display the item if it's in a bag
-			if(!IsPlayerCorpse() || item_data->equipSlot <= 30 || tCanLoot>=3)
+			// added cursor queue slots to corpse item visibility list. Nothing else should be making it to corpse.
+			if(!IsPlayerCorpse() || item_data->equipSlot <= 30 || tCanLoot>=3 ||
+				(item_data->equipSlot >= 8000 && item_data->equipSlot <= 8999))
 			{
-				if (i >= 30)
+				if (i < corpselootlimit) // < 30 (0 - 29) 
 				{
-						Message(13, "Warning: Too many items to display. Loot some then re-loot the corpse to see the rest");
-				}
-				else
-				{
 					item = database.GetItem(item_data->item_id);
 					if (client && item)
 					{
@@ -1006,9 +1074,24 @@
 						item_data->lootslot = i;
 					}
 				}
+				else if (i == corpselootlimit) // = 30
+				{
+					client->Message(13, "Warning: This corpse contains more items than can be displayed!");
+					client->Message(0, "Remove items and re-loot corpse to access remaining inventory.");
+				}
 				i++;
 			}
 		}
+		if (i > corpselootlimit) // > 30 (remember 'i' is increased again after the last iteration, so no '=')
+			client->Message(0, "(%s contains %i additional %s.)", GetName(), (i-corpselootlimit), (i-corpselootlimit)==1?"item":"items");
+
+		if (IsPlayerCorpse() && i == 0 && itemlist.size() > 0) { // somehow, corpse contains items, but client doesn't see them...
+			client->Message(13, "Warning: This corpse contains items that you do not permission to access!");
+			client->Message(13, "Contact a GM for assistance to determine if item replacement is necessary.");
+			client->Message(0, "BUGGED CORPSE [DBID: %i, Name: %s, Item Count: %i]", GetDBID(), GetName(), itemlist.size());
+			// if needed/wanted - create log dump->iterate corpse list..need pointer to log file
+			// could add code to check for owning client and give list of bugged items on corpse
+		}
 	}
 	
 	// Disgrace: Client seems to require that we send the packet back...
Index: PlayerCorpse.h
===================================================================
--- PlayerCorpse.h	(revision 2175)
+++ PlayerCorpse.h	(working copy)
@@ -111,7 +111,7 @@
 	inline int GetRezzExp() { return rezzexp; }
 
 protected:
-	void MoveItemToCorpse(Client *client, ItemInst *item, sint16 equipslot);
+	std::list<uint32> MoveItemToCorpse(Client *client, ItemInst *item, sint16 equipslot);
 
 private:
 	bool		p_PlayerCorpse;

Remember, this patch is NOT commit ready.

[Uleat]

I ran across a couple of client bugs that can be mentioned, but don't really need a new thread.


First, there's a odd issue with my client (SoF) where with a full corpse (30 items), clicking index 29 (zero-based) will
loot index 28. Clicking index 29 again, with 28 empty, will cause the loot window to close.

Second, with the submitted bugged corpse patch, and with the RespawnFromHover = false, dying outside of your
binding zone with certain items loaded on your cursor, will cause a glitch when you reload. The icon from one of the
cursor items will appear when you load into zone. If you die in your binding zone after this, but before leaving, this
issue doesn't appear.


[Random Items - no issue]
17005 (first item on cursor)
8005
13201
17003
1619

[Sequential Items, all have the same icon - issue arises]
13201 (first item on cursor)
13202
13203
13204
13205


I think it is a graphic glitch as putting this 'phantom' item in your inventory results in no server message. If you
use the #zopp command (slot 30, item id) and place it in your inventory, you get a server item deletion message.

[Uleat]

Now that I've confused, flustered and maddened everyone with my code submissions so far, let's move on to something more productive.


Here's what I'm seeing at first glance with the 'Bandolier' issue.


inventory.cpp::Client::SetBandolier(const EQApplicationPacket *app)

slot = m_inv.HasItem(m_pp.bandoliers[bss->number].items[BandolierSlot].item_id, 1, invWhereWorn|invWherePersonal|invWhereCursor);

Item.cpp::Inventory::HasItem(uint32 item_id, uint8 quantity, uint8 where)

if(where & invWhereCursor) {
	// Check cursor queue
	slot_id = _HasItem(m_cursor, item_id, quantity);
	if (slot_id != SLOT_INVALID)
		return slot_id;
}

The following internal function is overloaded allowing two argument sets. The checks in Inventory::HasItem all use the first definition with
the exception of the last check that uses the second one, which is m_cursor.


Item.cpp::Inventory::_HasItem(ItemInstQueue& iqueue, uint32 item_id, uint8 quantity)

for (it=iqueue.begin(); it!=iqueue.end(); it++) {
	ItemInst* inst = *it;
	if (inst)
	{
		if (inst->GetID() == item_id) {
			quantity_found += (inst->GetCharges()<=0) ? 1 : inst->GetCharges();
			if (quantity_found >= quantity)
					return SLOT_CURSOR;
		}

Notice how 'it' is iterated. If an item is found, it returns SLOT_CURSOR. MAJOR PROBLEM there unless it's found at slot 8000...

SetBandolier assigns the weapon slot based on 'HasItem.' Say, for instance, the item being sought is in the fourth queue position. The actual
item is in slot 8003, but the Bandolier function thinks it is in slot 30 (queue slot 8000) because of 'return SLOT_CURSOR;' This is creating a
CSD.

We can't change that to slot_id because the client doesn't allow cursor queue manipulation. We should consider only checking the queue
for items, but not allowing their removal unless all items up to that item are also removed, and possibly pushed back to the cursor.


I think this is one of the causes of the bandolier issue, and possibly the tradeskill cursor issue as well.


If you think my logic is flawed, your input is welcome. I need to understand this if I'm to stand any chance of fixing it.

[sorvani]

having done no research into this code at all, but reading your post and knowing some history of the issue, I wonder if things get broke because it is using the cursor slots?

What if someone hits forage then hits a bandolier swap right as the forage is placed on the cursor? or some other event that returns an item to the cursor?

[Uleat]

The testing I've done appears to be ok with only using slot 30. Anytime a bandolier item is found further back in the queue,
the first item is taken..explains why a 'Box of Abu-Kar' wound up as my primary weapon...

I imagine that any items in it would also go poof...


Also, the support function doesn't check the cursor for stacking before trying to move stackable items.

When moving an arrow out of worn inventory, the client adds it to the cursor stack, but the server puts it in an empty bag slot.
Further manual swaps show the CSD condition by making items disappear.


I'm going to try dropping the 'invWhereCursor' and add a single cursor check in addition to bumping up the stackable range
check to 30 from 29 and see where that takes us.


In regards to foraging, or any other timer-based action, we could just throw in a timer check in the beginning and exit out if
any of them are active..maybe throw a "You must finish your current action before using the bandolier" message to the client.


I also scanned through the tradeskill combine functions. They appear to only use the 'm_inv' list, but I'm trying to find where
the relationship is defined between list and slot to see if the cursor is also being checked. I found the index declarations.

[Uleat]

Both of the bandolier issue fixes appear to work so far. I'm at 100% success on items going where they're suppose to. (I'm not
checking the database on this yet since that part of the code should work. It was only where the items were going that was
the issue. I will check though before this gets posted down the road.)

This does not mean that issues won't still arise once the client becomes de-sync'd due to another reason. It just means that
the bandolier shouldn't be the cause of the de-sync.


As far as the timer possibility, I don't know how feasible that really is. There are no client packets sent in the SetBandolier
function, so all switching is handled client-side. Cancelling the process would create a major de-sync. We'll have to watch this.