Fix SSC (Server Side Characters) for Terraria 1.4.5

[Reisenbug]

ssc mode broken in 1.4.5
/uploadssc throwing IndexOutOfRangeException in log
ssc failed. Players can join an ssc-enabled server without being reset, but when they click on items in their inventory, those items turn into whatever ssc already has in that slot. Also, the items can't actually be picked up.
etc.

Root Cause:
1.4.5 source code:

1.4.4.9 source code:

They allocate 200 slots per bank instead of 40.

Changes:
Use PlayerItemSlotID constants instead of sequential slot counter in RestoreCharacter
Add try-finally to ensure IgnoreSSCPackets is reset even if exception occurs
Update HandlePlayerSlot slot validation to use PlayerItemSlotID.Count (990) instead of NetItem.MaxInventory (350)

[Reisenbug]

conflicts solved

[Reisenbug]

These changes successfully solved at least the issue that players lose all of their items in Void Vault and Defender's Forge

[hakusaro]

Can we derive this somehow in a way that makes it clear what -1 is?

[Reisenbug]

PlayerItemSlotID.Count (990) is the total number of slot IDs, slots range from 0 to 989, so Count - 1 is the last valid slot index. Would a descriptive comment or a local variable like const int lastSyncSlot = PlayerItemSlotID.Count - 1 be clearer

[hakusaro]

Either, yes

[bartico6]

random thoughts screamed into the ether on the ssc changes in 1.4.5.0+ (not related to this PR per se, just something to note for future reference if anyone wants to make changes)

• packet 5 no longer requires an Item instance to send, so all senditem methods (or any SendData(5, ...) calls for that matter) could be collapsed into array-less overloads like so:

  • SendItem(int player, int index);
  • SendItems(int player, int index, int count); => count * SendItem(player, index + i);

• be careful around personal banks. you want to use the offsets from PlayerItemSlotID (because they're correct) but use array lengths for the bounds of each bank (because PlayerItemSlotID is incorrect) - once the 40-200 discrepancy is resolved, PlayerItemSlotID is Terraria's internal version of what you currently have in NetItem, and the one in NetItem should be deprecated/removed.

• it could be worthwhile to import these values from PlayerItemSlotID at runtime instead of hardcoding them, as otherwise you'll be returning to this code and reasoning about it every update - though this is guaranteed to temporarily happen anyway due to the bad indexes issue with personal banks

• the massive loop sending everything from the server in RestorePlayer could be simplified: instead of manually deciding which slots to broadcast and which slots to send only to the player being restored, you can use PlayerItemSlotID.CanRelay[int] with the global index of that item slot. if it's false, send to owner only, if it's true, send to everyone including owner. You could also make it into one loop going 0-990 since you're using NetMessage.SendData() which will automatically convert the index back to the correct Item array anyway

overall fix looks okay from experience making similar changes elsewhere but I cannot personally test tshock's version anytime soon. 👍

[hakusaro]

Most likely this can be merged if you wanna make a small comment or change on the magic number. Otherwise I’m reasonably satisfied!

[hakusaro]

seems legit on paper

[hakusaro]

@ACaiCat can you re-review and approve if you're good?

[ACaiCat]

Looks good to me

[ACaiCat]

all slots are tracked normally

[hakusaro]

@Nitori-Lab if you can review #3164 I would really appreciate it, but if you can't, nbd!