Home » MODDING HQ 1.13 » v1.13 Bug Reports » [PATCH] Fix invalid slots in map screen inventory
[PATCH] Fix invalid slots in map screen inventory[message #336486] Tue, 07 October 2014 10:50 Go to next message
Ocular

 
Messages:24
Registered:September 2014
Hello again,

There is a problem in the map screen inventory arising from the reuse of the pInventoryPoolList vector, which is done for performance reasons.

To illustrate this problem, let's apply the following patch that:

  • Prints a slot (WORLDITEM) object's name and usFlags on left click
  • Prints a message when clearing an inventory slot for later reuse

diff --git a/Build/Strategic/Map Screen Interface Map Inventory.cpp b/Build/Strategic/Map Screen Interface Map Inventory.cpp
index 5c82af3..66acd7f 100644
--- Build/Strategic/Map Screen Interface Map Inventory.cpp	
+++ Build/Strategic/Map Screen Interface Map Inventory.cpp	
@@ -1388,6 +1388,23 @@ void MapInvenPoolSlots(MOUSE_REGION * pRegion, INT32 iReason )
 			return;	
 		}
 
+ScreenMsg(FONT_MCOLOR_LTYELLOW, MSG_INTERFACE,
+	  L"[%s] %s%s%s%s%s%s%s%s%s%s%s%s%s",
+	  ItemNames[ pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].object.usItem ],
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_DONTRENDER) ? L"WORLD_ITEM_DONTRENDER " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WOLRD_ITEM_FIND_SWEETSPOT_FROM_GRIDNO) ? L"WOLRD_ITEM_FIND_SWEETSPOT_FROM_GRIDNO " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_ARMED_BOMB) ? L"WORLD_ITEM_ARMED_BOMB " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_SCIFI_ONLY) ? L"WORLD_ITEM_SCIFI_ONLY " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_REALISTIC_ONLY) ? L"WORLD_ITEM_REALISTIC_ONLY " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_REACHABLE) ? L"WORLD_ITEM_REACHABLE " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_GRIDNO_NOT_SET_USE_ENTRY_POINT) ? L"WORLD_ITEM_GRIDNO_NOT_SET_USE_ENTRY_POINT " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_DROPPED_FROM_ENEMY) ? L"WORLD_ITEM_DROPPED_FROM_ENEMY " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_TABOO_FOR_MILITIA_EQ_ELITE) ? L"WORLD_ITEM_TABOO_FOR_MILITIA_EQ_ELITE " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_TABOO_FOR_MILITIA_EQ_BLUE) ? L"WORLD_ITEM_TABOO_FOR_MILITIA_EQ_BLUE " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_TABOO_FOR_MILITIA_EQ_GREEN) ? L"WORLD_ITEM_TABOO_FOR_MILITIA_EQ_GREEN " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_MOVE_ASSIGNMENT_IGNORE) ? L"WORLD_ITEM_MOVE_ASSIGNMENT_IGNORE " : L"",
+	  (pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].usFlags & WORLD_ITEM_TABOO_FOR_MILITIA_EQ_ALL) ? L"WORLD_ITEM_TABOO_FOR_MILITIA_EQ_ALL" : L"");
+
 		// check if item in cursor, if so, then swap, and no item in curor, pick up, if item in cursor but not box, put in box
 
 		if ( gpItemPointer == NULL )
@@ -2192,6 +2209,7 @@ void BuildStashForSelectedSector( INT16 sMapX, INT16 sMapY, INT16 sMapZ )
 					*pusi++ = *pipl;
 					uiNumberOfUnSeenItems++;
 				}
+				if (pipl->fExists) ScreenMsg(FONT_MCOLOR_LTYELLOW, MSG_INTERFACE, L"Clearing slot %d: %s", i, ItemNames[ pipl->object.usItem ]);
 				pipl->fExists = FALSE;
 				pipl->object.usItem = NONE;
 				pipl->object.ubNumberOfObjects = 0;
@@ -2203,6 +2221,7 @@ void BuildStashForSelectedSector( INT16 sMapX, INT16 sMapY, INT16 sMapZ )
 	pipl = &pInventoryPoolList[uiNumberOfSeenItems];
 	for(i=uiNumberOfSeenItems; ifExists) ScreenMsg(FONT_MCOLOR_LTYELLOW, MSG_INTERFACE, L"Clearing slot %d: %s", i, ItemNames[ pipl->object.usItem ]);
 		pipl->fExists = FALSE;
 		pipl->object.usItem = NONE;
 		pipl->object.ubNumberOfObjects = 0;


After rebuilding, we start a new game and cheat our way to the San Mona sector with the Shady Lady, which has multiple ACTION_ITEMs.

When we bring up the map screen inventory, we see:

http://i.imgur.com/ADXiV2d.png

The first two empty slots (indices 29 and 30) used to contain a cheap key and an action item respectively. Action items, of course, are essentially invisible booby-trapped items that have the WORLD_ITEM_ARMED_BOMB bit set.

If we click on the first empty slot (index 29) we see:

http://i.imgur.com/rP1AivF.png

There is nothing there, but the WORLD_ITEM_REACHABLE flag is set on the slot

[Updated on: Sat, 11 October 2014 23:44] by Moderator

Re: [PATCH] Fix invalid slots in map screen inventory[message #336568] Sat, 11 October 2014 12:23 Go to previous messageGo to next message
silversurfer

 
Messages:2527
Registered:May 2009
I took a quick look at the code and the current implementation of "uiNumberOfSeenItems" doesn't make sense to me for an unloaded sector.

If a sector is loaded "uiNumberOfSeenItems" is 0 first and gets incremented whenever we find an item that we have discovered already. So in the end we have the number of all seen items in this variable. So far so good...

If the sector is not loaded we set "uiNumberOfSeenItems" to the total amount of all items in this sector. However, we never decrease this value for items that are not seen. :headscratch:
So in the end we treat the item pool as if we see all items all the time? I have to debug this to really see what's going on which I cannot do at the moment.

My assumption at the moment is that "pipl->usFlags = 0;" only needs to be set in the final for loop that cleans unused spots and "uiNumberOfSeenItems" needs to be decreased when we detect a hidden item.

[Updated on: Sat, 11 October 2014 12:24] by Moderator


Re: [PATCH] Fix invalid slots in map screen inventory[message #336571] Sat, 11 October 2014 16:18 Go to previous messageGo to next message
Uriens

 
Messages:329
Registered:July 2006
I ran into this problem on my plays. On WF maps in airport sector that I use as a place to store items, it would often happen that I can't combine some items and some magazines would not get merged into boxes/crates when ordered to. I couldn't make heads or tails of when it was happening and couldn't find reliable way to reproduce the problem so I didn't report anything yet. I did manage to make out that certain item slots would cause items to start behaving like that. Sometimes, just moving those items to another slot would 'fix' them but it didn't happen 100% of the time. The bug mentioned here would most likely be the cause of what I was experiencing.
Re: [PATCH] Fix invalid slots in map screen inventory[message #336574] Sat, 11 October 2014 17:39 Go to previous messageGo to next message
silversurfer

 
Messages:2527
Registered:May 2009
After some more code reading I'm not so sure if function "BuildStashForSelectedSector" is the right place to fix it. Why not fix it where the item is placed in sector inventory?

This would be function "MapInvenPoolSlots" (around line 1605).

// Else, try to place here
if ( PlaceObjectInInventoryStash( &( pInventoryPoolList[ ( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter ].object ), gpItemPointer,
				( iCurrentInventoryPoolPage * MAP_INVENTORY_POOL_SLOT_COUNT ) + iCounter, iCurrentlyPickedUpItem ) )
			{
	// HEADROCK HAM 5: A LOT of functions rely on these flags being set. So set them!!
	pInventoryPoolList[(iCurrentInventoryPoolPage*MAP_INVENTORY_POOL_SLOT_COUNT)+iCounter].bVisible = TRUE;
	pInventoryPoolList[(iCurrentInventoryPoolPage*MAP_INVENTORY_POOL_SLOT_COUNT)+iCounter].fExists = TRUE;
+	pInventoryPoolList[(iCurrentInventoryPoolPage*MAP_INVENTORY_POOL_SLOT_COUNT)+iCounter].usFlags &= ~WORLD_ITEM_ARMED_BOMB ; // remove flag from slot
				
	/*if(gGameExternalOptions.fEnableInventoryPoolQ)//dnl ch51 091009

This should fix the problem reliably.

A funny thing that I noticed while testing is that you can arm a bomb and place it in sector inventory in an unloaded sector. This doesn't make much sense to me because you never know where exactly it will be placed. This is not new but maybe we want to prevent that in the future (pampering players even more Wink )? What do you think?

Re: [PATCH] Fix invalid slots in map screen inventory[message #336579] Sat, 11 October 2014 20:55 Go to previous messageGo to next message
Ocular

 
Messages:24
Registered:September 2014
silversurfer
After some more code reading I'm not so sure if function "BuildStashForSelectedSector" is the right place to fix it. Why not fix it where the item is placed in sector inventory?


This is a good idea (and my initial thought), but this approach requires more changes than clearing the bitfield once in BuildStashForSelectedSector.

For instance, this patch to MapInvenPoolSlots only fixes the problem for items dropped manually from the cursor, but not for items placed there via 'W', 'E', Ctrl+Click, or sorting/filtering.

My thinking for fixing this in BuildStashForSelectedSector is this:

  • The reuse of pInventoryPoolList[] slots was introduced in rev 6554 as a performance enhancement. This means that most inventory code was written with the assumption that these structs are clean.
  • If this assumption can no longer be held, all code that touches pInventoryPoolList[] slots must take into account that the underlying data might be dirty.
  • The usFlags bitfield is the most likely place for an unintended data corruption because this field is usually modified via '|=' or '&= ~', instead of wholly replaced by assignment.
  • If the usFlags bitfield is the most problematic field and existing code expects that non-existant pInventoryPoolList[] slots have clear bitfields, then the most minimally invasive change is to also clear the bitfield when the slot is being cleared for reuse.

tl;dr: Restore the assumption that usFlags == 0 in an "empty" pInventoryPoolList[] slot.

Hope that makes sense. Thanks for reviewing this patch!

EDIT:

silversurfer
My assumption at the moment is that "pipl->usFlags = 0;" only needs to be set in the final for loop that cleans unused spots


I think that's right; the other "pipl->usFlags = 0;" is superfluous. I added it because I thought it might be confusing to have two styles of "clearing" a slot in the same function, but that would probably lead to cargo-culting later. A clear comment over the single change would be better.

[Updated on: Sat, 11 October 2014 21:02] by Moderator

Re: [PATCH] Fix invalid slots in map screen inventory[message #336583] Sat, 11 October 2014 22:05 Go to previous messageGo to next message
silversurfer

 
Messages:2527
Registered:May 2009
Ocular

silversurfer
My assumption at the moment is that "pipl->usFlags = 0;" only needs to be set in the final for loop that cleans unused spots


I think that's right; the other "pipl->usFlags = 0;" is superfluous. I added it because I thought it might be confusing to have two styles of "clearing" a slot in the same function, but that would probably lead to cargo-culting later. A clear comment over the single change would be better.

Unfortunately my original assumption is not true. The problem is that "uiNumberOfSeenItems" is not what the name implies. It's not the counter for items that we know of and that are visible. For unloaded sectors it holds the total number of items. The item list of an unloaded sector is not sorted which means that we can have a mix of seen and unseen items in the list. So my intention to decrease "uiNumberOfSeenItems" doesn't work because in this case the final for loop may overwrite seen items which are at the end of the initial list.

Your fix is the best we have at the moment unless we find another solution that also covers all options.

I may research this more.

Re: [PATCH] Fix invalid slots in map screen inventory[message #336607] Sun, 12 October 2014 14:34 Go to previous message
silversurfer

 
Messages:2527
Registered:May 2009
I committed your patch in revision 7552 (dev) and 7553 (stable). After all it's a good patch because it cleans the list of invalid flags and we do not have to fix a messy list in all the other places. Thanks again. Smile

Previous Topic: [PATCH] Fix missing UGL grenades when removing attachments under NAS
Next Topic: [PATCH] Fix ready AP cost and unnecessary lowering of GLs
Goto Forum:
  


Current Time: Fri Apr 26 03:03:34 EEST 2019

Total time taken to generate the page: 0.01109 seconds