[CPP] Send inventory update after equipment change

[MowFord]

I affirm:

• [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• [x] I have read and understood the Contributing Guide and the Code of Conduct.
• [x] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Temporary items disappear from item menu after changing equipment. Come back after sorting inventory or moving items between inventory and satchel/wardrobes.

Traced it down to PChar->pushPacket(new CInventoryFinishPacket()); needing to be sent after CEquipPacket.

Note that the changes here are do the charutils::EquipItem function so the catch equipment changes via both 0x050 and 0x051

Steps to test these changes

Before this change, replicate the issue via:

• !exec player:addTempItem(5393)
• open quick item menu to see temp item available
• Then equip any pieced of gear and see that the temp item disappears from quick item menu

same process for unequipping an item.

After change see that the item stays visible after equipping and unequipping gear

[zach2good]

During a full equip set swap, or macros, will this send out multiple finish packets? What does retail do in this scenario, and the one I described?

[MowFord]

During a full equip set swap, or macros, will this send out multiple finish packets? What does retail do in this scenario, and the one I described?

No idea. I'll say however that equipping an item was already sending this packet, just at the wrong time.

Edit: also ashitacast sends individual equp change packets, so this would of course be sending individual inventory finish packets. Equipsets loop over each piece of gear and run EquipItem. This makes both methods of changing gear send the full gambit of packets for every piece of gear

[zach2good]

I'll say however that equipping an item was already sending this packet, just at the wrong time.

You've moved one packet send statement, yes. You've also introduced another one. Please go and observe before/after these changes and see what the packet volume is for:

• Equipping single items
• De-quipping single items
• Going through equipsets
• Going through macros to change gear
• Using whatever mechanism ashitacast uses

It would also be extremely useful to confirm that we (both before and/or after) are doing what retail does.

[WinterSolstice8]

Equip single item (overwriting a slot that already had something in it): the last 0x1D has flag set to 0x01, the rest are 0x00

First 0x01F was the item that used to be equipped, second is the new item.

[WinterSolstice8]

Equip single item with the slot empty (behavior is identical with unequipping item): Same thing with flag being 0x01 on the last 0x1D

[WinterSolstice8]

Equipset of 5 items when naked (this is what ashitacast uses)

Equipset of 5 items overwriting items that already exist (this is what ashitacast uses) It first unequips the items in the slot then equips the new ones (0x01F)

[WinterSolstice8]

Macro that overrides slots already equipped: Oddly, the behavior here with 0x01F is to equip the new items first then unequip (0x01F)

[WinterSolstice8]

Looks like the flow is

1. Send equip slot & model change
2. Assign or unassign items in either order (but in batches of assign or unassign)
3. Finish with a flag 0x0 finish inventory and flag 0x1 finish inventory

There's some slight discrepancies, such as finish inventory being called three on a single item equip through the menu instead of twice (not sure what the point of that was)

Edit: We can probably optimize this to only send one model change packet since it includes all the models in a single packet.

[MowFord]

FWIW we went live with this on cexi yesterday and had massive performance issues that night, including some zone crashes:

My current theory is the game client sends an extra request if it doesn't get the "inventory finish" with flag 0 before the final "inventory finish" with flag 1 (as shown in winter's captures), I've updated my PR to reflect this. Still resolves the issue with temporary items, but we've not tested it with a load of players

[MowFord]

We've been running cexi with this commit for a while now. Works great.

Just to be clear, the PR adds the inventory finish packets exactly like winter's capture showed above

[zach2good]

What changed between March 12th's we went live with this on cexi yesterday and had massive performance issues ... including crashes and We've been running cexi with this commit for a while now. Works great.?

I wouldn't be surprised if GitHub's commit/comment tracking hasn't captured any changes correctly, but to an onlooker it looks like there have been no additional changes.

[zach2good]

If Winter has shown a bunch of PVLV/VieweD captures, it'll probably be useful for you to provide the same for your changes. It's very easy, takes 5 minutes.

Edit: We can probably optimize this to only send one model change packet since it includes all the models in a single packet.

Not against non-retail packet optimisations, but it would need to be behind a default-off setting. Something something retail emulator something something darkside.

[MowFord]

What changed between March 12th's we went live with this on cexi yesterday and had massive performance issues ... including crashes and We've been running cexi with this commit for a while now. Works great.?

I wouldn't be surprised if GitHub's commit/comment tracking hasn't captured any changes correctly, but to an onlooker it looks like there have been no additional changes.

The extra inventory finish was added the same time as my comment.

Also I doubt that it was even related. People were frustrated and blamed that change but there was no Tracy logging or anything scientific done (we had also enabled a few other big things that update)

I'll see about getting someone to capture a gear change on cexi

[MowFord]

^^ This is an equipset with two items

^^ this is an ashitacast gearset with 5 items

Looks like EquipItem( function needs to include a bool to pass the finish inventory packet so it's not sent on every item

Edit: rebased so i can start looking into it

[MowFord]

OK I give up everything I can think of would probably be considered a hacky solution.

Problem: We need to respond to a sequence of 0x050 packets by only sending finish inventory on the final one. AFAIK there's no way to know which is the final.

Potential Solution 1: in place of this PR's two calls of

                    PChar->pushPacket(new CInventoryFinishPacket(static_cast<CONTAINER_ID>(containerID)));
                    PChar->pushPacket(new CInventoryFinishPacket());

we put some kind of timer for 100ms that gets refreshed if the timer is already present. This would push back the finish inventory pair until the last 0x050 and solve the issue of an equipset 0x051 packet sending a string of EquipItem( calls (which btw ashitacast doesn't use, it sends individual 0x050 packets, unless aclegacy is different than luashitacast, which I don't use)

Potenial Solution 2: add a flag to EquipItem to define if the finish inventory packet should be sent with a valid change of equipment and somehow know which is the last 0x050 packet.

Separately, we'd need to just put a manual set of finish inventory packets after this loop in 0x051 (or get very clever with the logic to retroactively check if it should send that flag before calling EquipItem() :