Changed ITEM:Remove() fallback code to work and (hopefully) be more reliable

[DoopieWop]

Problems with the current implementation

In the Item meta function "Remove" there is some code that runs if the previous loop (Line 424-436) couldn't find the to-be-deleted item in all the slots, that its expected to be at. E.g. an item with x = 1, y = 3, w = 2, h = 1, should be in the slots, x = 1 & y = 3; x = 2 & y = 3. If, for example, the item object is not in x = 2 & y = 3, this "fallback" code is run:

helix/gamemode/core/meta/sh_item.lua

Lines 438 to 455 in 418947a

	if (failed) then
	inv.slots = {}
	for k, _ in inv:Iter() do
	if (k.invID == inv:GetID()) then
	for x = self.gridX, self.gridX + (self.width - 1) do
	for y = self.gridY, self.gridY + (self.height - 1) do
	inv.slots[x][y] = k.id
	end
	end
	end
	end
	if (IsValid(inv.owner) and inv.owner:IsPlayer()) then
	inv:Sync(inv.owner, true)
	end
	return false
	end

However, the current fallback code doesn't actually work, for multiple reasons.
The way it was supposed to work is, that it caches all the items currently in the inventory (Version 4e49199 Line 439) then it empties the inventory.slots table (Line 439).

The inventory.slots table stores all the items associated with the inventory, using their x and y position as the key. It looks something like this: inv.slot[3][2] = item[51] (item object with id 51 at x = 3 and y = 2)

However because of a recent commit #468 the items wouldnt be cached before clearing inv.slots, so the following loop wouldn't do anything.
Said loop (Line 440-448) was meant to repopulate the slots table with all the items. Essentially to ensure that all the items associated with the inventory are actually all entered into the slots table correctly.
Additionally, the loop seemingly only uses the to-be-removed item's grid x and y positions and width and height (Line 442f), instead of the currently relevant item's x, y, w and h values.
Lastly the code would then sync this new slots table to the owner, however the code was treating inv.owner as a player object, instead of a character id, so it would error. (Line 450-452)

Here is what I changed

Firstly I made sure the function would also be marked as "failed" if it cant even find the items grid x position in the slots table, so if inv.slots[x] = nil. (Commit Line 434-435)

Next I changed which items the fallback code uses to reconstruct the inv.slots table. My thinking was, that an entirely missing item wouldn't be reconstructed here as INVENTORY:GetItems() uses the inv.slots table as well. I opted to loop through ix.item.instances. Even though this table is likely very large and we can only loop through with pairs, which is a hair slower than ipairs, it felt worth it, as this table most definitely contains every active item instance. While we are looping through, we are also making sure to leave out the item that was meant to be removed, so the fallback code ensures that removing the item will actually happen even if it failed first time around. Also a small side-note, this remove function basically never fails, at least I've never seen it fail, so if it fails, it wouldn't be the worst if ix.item.instances is looped through once. (Commit Line 440-445)

Next the loop responsible for reconstructing the inv.slots table has been fixed. Nothing much to say here. (Commit Line 448-445)

Lastly I changed the way the fallback code syncs the change to the player. For one, it now respects the bNoReplication function argument. Additionally, since the fallback code now doesnt count as "failed", the rest of the code will also run, so to ensure that there wouldn't be double syncs (once to sync the new inv.slots and once for the "ixInventoryRemove" net message) i opted to move the sync function down to the net message. (Commit Line 476-483)

Why does this fallback code exist?

Despite all these changes I still dont know if this fallback code is actually important at all. Was it a bandaid fix from nutscript devs to fix some niche issue that popped up once in a while? Probably. Does this issue still persist today? Who knows. Would everything break without this fallback code? No idea.
This function definitely needs to be revisited in the future, as ultimately this entire fallback logic is just a bandaid fix.