From c1a84cbe0c1727cd711e9c0278d7ad53b13d1e46 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 22 Jun 2026 22:15:18 +0200 Subject: [PATCH] =?UTF-8?q?fix(D.2b):=20wield=20is=20ContainerId-based=20?= =?UTF-8?q?=E2=80=94=20drop=20the=20WielderId=20write=20(code=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-quality review (needs-changes) on Task 3: WieldItemOptimistic wrote item.WielderId directly, but RollbackMove (via MoveItem) never cleared it → a wield-rollback left a pack item with a stale WielderId=player. Root-cause fix (vs the reviewer's snapshot-WielderId suggestion): acdream's existing WieldObject 0x0023 confirm models a wielded item as ContainerId= wielder + equip=mask and does NOT touch WielderId. So the optimistic path must match — drop the WielderId write entirely. Optimistic state now equals the confirmed state, rollback fully restores through MoveItem alone, and the stale-state class is structurally eliminated. The paperdoll's (WielderId==p || ContainerId==p) filter still matches optimistic wields via ContainerId (login-equipped items match via WielderId from their CreateObject). Also: + the wield+move outstanding-count combo test (spec §8), MoveItem doc note that it doesn't manage WielderId, and the test pins WielderId==0 post- optimistic-wield to document the model. Core 74/74 green. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-06-22-d2b-paperdoll-slice1-equip-slots.md | 2 +- src/AcDream.Core/Items/ClientObjectTable.cs | 14 +++++++---- .../Items/ClientObjectTableTests.cs | 25 ++++++++++++++++--- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/docs/superpowers/plans/2026-06-22-d2b-paperdoll-slice1-equip-slots.md b/docs/superpowers/plans/2026-06-22-d2b-paperdoll-slice1-equip-slots.md index b4f7d92e..1a4d769e 100644 --- a/docs/superpowers/plans/2026-06-22-d2b-paperdoll-slice1-equip-slots.md +++ b/docs/superpowers/plans/2026-06-22-d2b-paperdoll-slice1-equip-slots.md @@ -566,7 +566,7 @@ public class PaperdollControllerTests var payload = new ItemDragPayload(0xD01u, ItemDragSource.Inventory, 0, lists[HeadSlot].Cell); ctrl.HandleDropRelease(lists[HeadSlot], lists[HeadSlot].Cell, payload); Assert.Equal(EquipMask.HeadWear, objects.Get(0xD01u)!.CurrentlyEquippedLocation); // equipped instantly - Assert.Equal(Player, objects.Get(0xD01u)!.WielderId); + Assert.Equal(Player, objects.Get(0xD01u)!.ContainerId); // contained-by-wielder (the optimistic wield is ContainerId-based; it does NOT write WielderId) Assert.Single(wields); Assert.Equal((0xD01u, (uint)EquipMask.HeadWear), wields[0]); // GetAndWieldItem wire } diff --git a/src/AcDream.Core/Items/ClientObjectTable.cs b/src/AcDream.Core/Items/ClientObjectTable.cs index 2a310ea4..48893182 100644 --- a/src/AcDream.Core/Items/ClientObjectTable.cs +++ b/src/AcDream.Core/Items/ClientObjectTable.cs @@ -119,7 +119,9 @@ public sealed class ClientObjectTable /// Handle a server-driven move — called from /// InventoryPutObjInContainer (0x0022) and WieldObject (0x0023) /// handlers. Updates ContainerId / ContainerSlot / CurrentlyEquippedLocation - /// and fires ObjectMoved. + /// and fires ObjectMoved. Does NOT touch WielderId — neither the wield nor the + /// unwield path manages it (a wielded item is modeled as contained-by-the-wielder), + /// so RollbackMove restores full pre-move state through this method alone. /// public bool MoveItem(uint itemId, uint newContainerId, int newSlot = -1, EquipMask newEquipLocation = EquipMask.None) @@ -191,14 +193,16 @@ public sealed class ClientObjectTable } /// Optimistic (instant) wield: snapshot the pre-wield (container, slot, equip), then set - /// ContainerId = WielderId = wielderGuid and CurrentlyEquippedLocation = equipMask (matching the - /// server's WieldObject 0x0023 confirm), firing ObjectMoved for an immediate repaint. The caller - /// sends GetAndWieldItem; ConfirmMove (on the 0x0023 echo) / RollbackMove (on 0x00A0) reconcile. + /// ContainerId = wielderGuid and CurrentlyEquippedLocation = equipMask — EXACTLY what the server's + /// WieldObject 0x0023 confirm does via (acdream models a wielded item as + /// contained-by-the-wielder). Neither path writes WielderId, so the optimistic state equals the + /// confirmed state and via MoveItem fully restores pre-move state. Fires + /// ObjectMoved for an immediate repaint. The caller sends GetAndWieldItem; ConfirmMove (on the 0x0023 + /// echo) / RollbackMove (on 0x00A0) reconcile. public bool WieldItemOptimistic(uint itemId, uint wielderGuid, EquipMask equipMask) { if (!_objects.TryGetValue(itemId, out var item)) return false; RecordPending(itemId, item); - item.WielderId = wielderGuid; // unambiguously the player's gear during the optimistic window return MoveItem(itemId, wielderGuid, newSlot: -1, newEquipLocation: equipMask); } diff --git a/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs b/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs index 758e641a..4c8f524e 100644 --- a/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs +++ b/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs @@ -455,17 +455,17 @@ public sealed class ClientObjectTableTests } [Fact] - public void WieldItemOptimistic_equipsInstantly_andSetsWielderAndContainer() + public void WieldItemOptimistic_equipsInstantly_setsContainerAndEquip() { var table = new ClientObjectTable(); const uint player = 0x50000001u, pack = 0x40000005u; table.AddOrUpdate(new ClientObject { ObjectId = 0x940u }); - table.MoveItem(0x940u, pack, newSlot: 2); // a pack item + table.MoveItem(0x940u, pack, newSlot: 2); // a pack item (WielderId stays 0) table.WieldItemOptimistic(0x940u, player, EquipMask.HeadWear); var o = table.Get(0x940u)!; Assert.Equal(EquipMask.HeadWear, o.CurrentlyEquippedLocation); // equipped now (instant) - Assert.Equal(player, o.WielderId); - Assert.Equal(player, o.ContainerId); + Assert.Equal(player, o.ContainerId); // contained-by-wielder (matches the WieldObject 0x0023 confirm) + Assert.Equal(0u, o.WielderId); // optimistic wield does NOT write WielderId (ContainerId-based model) } [Fact] @@ -499,4 +499,21 @@ public sealed class ClientObjectTableTests [Fact] public void WieldItemOptimistic_unknownItem_false() => Assert.False(new ClientObjectTable().WieldItemOptimistic(0xDEADu, 0x1u, EquipMask.HeadWear)); + + [Fact] + public void WieldThenMove_oneConfirm_rollsBackToPreWield() // outstanding-count across wield+move (shared RecordPending) + { + var table = new ClientObjectTable(); + const uint player = 0x50000001u, pack = 0x40000005u, pack2 = 0x40000006u; + table.AddOrUpdate(new ClientObject { ObjectId = 0x950u }); + table.MoveItem(0x950u, pack, newSlot: 2); // pre-wield: pack slot 2 + table.WieldItemOptimistic(0x950u, player, EquipMask.HeadWear); // outstanding 1, snapshot (pack, 2, None) + table.MoveItemOptimistic(0x950u, pack2, 0); // outstanding 2 (same snapshot) + table.ConfirmMove(0x950u); // confirm the first → outstanding 1, still pending + Assert.True(table.RollbackMove(0x950u)); // the second rejected → roll back + var o = table.Get(0x950u)!; + Assert.Equal(pack, o.ContainerId); // to the ORIGINAL pre-wield container + Assert.Equal(2, o.ContainerSlot); // and slot + Assert.Equal(EquipMask.None, o.CurrentlyEquippedLocation); // pre-wield was un-equipped + } }