[Feature] Implement Move Multiple Items #4259
Conversation
|
Do not merge this. Found some bugs. |
|
No, it is because the swap contains redundant two-way operations. |
|
This works according to my testing. It implements all 3 ctrl-click modes exactly replicating client behavior.
left and right click logic needs to be bifurcated because left click handles things a lot like proper slot swaps or autoinv in terms of stack combining, etc, so we want to leverage the existing logic of SwapItem. right click just bluntly exchanges contents, but we need to respect the new targets instead of just explicitly exchanging the bag contents slot for slot. |
Kinglykrab
changed the title
|
This has been running on my servers for a while now without issues (~40 users total). I haven't been able to break it personally. I'm reasonably confident in this implementation. |
Can confirm this has been working as expected. Have not ran into any issues while testing on Cata's servers. |
| enum MoveMultiType: int16 { | ||
| inventoryPersonal = 0, | ||
| inventoryBank = 1, | ||
| inventorySharedBank = 2 | ||
| }; | ||
|
|
| /*000*/ int16 Type; // Worn and Normal inventory = 0, Bank = 1, Shared Bank = 2, Delete Item = -1 | ||
| /*002*/ int16 Unknown02; | ||
| /*004*/ int16 Slot; | ||
| /*006*/ int16 SubIndex; | ||
| /*008*/ int16 AugIndex; // Guessing - Seen 0xffff | ||
| /*010*/ int16 Unknown01; // Normally 0 - Seen 13262 when deleting an item, but didn't match item ID |
There was a problem hiding this comment.
This exists already in rof_structs.h, I just copied it. Would it be more correct to adjust this in rof_structs.h and reference it from there?
| Kick("Unimplemented move multiple items"); // TODO: lets not desync though | ||
| // This packet is only sent from the client if we ctrl click items in inventory | ||
| if (m_ClientVersionBit & EQ::versions::maskRoF2AndLater) { | ||
|
|
| bool error = false; | ||
| SwapItemResync(mi); | ||
| InterrogateInventory(this, false, true, false, error, false); | ||
| if (error) |
| from_slot = m_inv.CalcSlotId(multi_move->moves[i].from_slot.Slot + EQ::invslot::SHARED_BANK_BEGIN, multi_move->moves[i].from_slot.SubIndex); | ||
| } | ||
|
|
||
| auto to_slot = m_inv.CalcSlotId(multi_move->moves[i].to_slot.Slot, multi_move->moves[i].to_slot.SubIndex); |
There was a problem hiding this comment.
Can use actual type here since it's a simple type.
|
|
||
| for (int i = 0; i < multi_move->count; i++) { | ||
| // These are always bags, so we don't need to worry about raw items in slotCursor | ||
| auto from_slot = m_inv.CalcSlotId(multi_move->moves[i].from_slot.Slot, multi_move->moves[i].from_slot.SubIndex); |
There was a problem hiding this comment.
Can use actual type here since it's a simple type.
| const auto from_parent = multi_move->moves[0].from_slot.Slot; | ||
| const auto to_parent = multi_move->moves[0].to_slot.Slot; |
There was a problem hiding this comment.
Can use actual types here since they're simple types.
| return; // Not enough data to be a valid packet | ||
| } | ||
|
|
||
| const MultiMoveItem_Struct* multi_move = reinterpret_cast<const MultiMoveItem_Struct*>(app->pBuffer); |
There was a problem hiding this comment.
Could use const auto if you'd prefer since it's a complex type.
| // This is a left click which is purely additive. This should always be cursor object or cursor bag contents into general\bank\whatever bag | ||
| if (left_click) { | ||
| for (int i = 0; i < multi_move->count; i++) { | ||
| MoveItem_Struct* mi = new MoveItem_Struct(); |
There was a problem hiding this comment.
Could use auto if you'd prefer since it's a complex type.
| } | ||
| } | ||
|
|
||
| for (const MoveInfo& move : items) { |
There was a problem hiding this comment.
Could use const auto& if you'd prefer since it's a complex type.
| PutItemInInventory(move.to_slot, *move.item); // This saves inventory too | ||
| } | ||
| } | ||
|
|
| MoveInfo move; | ||
| move.item = m_inv.PopItem(from_slot); // Don't delete the instance here | ||
| move.to_slot = to_slot; |
There was a problem hiding this comment.
Can do this.
MoveInfo move{
.item = m_inv.PopItem(from_slot), // Don't delete the instance here
.to_slot = to_slot
};
if (move.item) {
|
@catapultam-habeo can you attach some demo videos? Do you have any concerns of item dupes or item loss? |
Sorry for the delay in having KK's comments addressesed. I'll get some demo videos when I make those adjustments and have a new build. As for concerns about item dupes or loss; I don't see how either could happen unless there is a flaw with the existing inventory manipulation methods. That said, I think that there is a flaw in existing inventory manipulation somewhere, but I haven't been able to locate it. Unrelated to this PR (I have observed this behavior prior to working on this, on a totally stock release server), items can dup into ostensibly unaddressable inventory Slot IDs. There doesn't seem to be any real consequence other than extraneous |
Description
This implements ctrl-click movement of bag contents for ROF+ clients.
Type of change
Testing
There are three behaviors that are introduced here;
I don't know how to attach images to demonstrate this.
I would furthermore like some assistance testing this.
Clients tested:
Checklist