fix(D.2b): wield is ContainerId-based — drop the WielderId write (code review)
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) <noreply@anthropic.com>
This commit is contained in:
parent
0c353185c3
commit
c1a84cbe0c
3 changed files with 31 additions and 10 deletions
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
/// </summary>
|
||||
public bool MoveItem(uint itemId, uint newContainerId, int newSlot = -1,
|
||||
EquipMask newEquipLocation = EquipMask.None)
|
||||
|
|
@ -191,14 +193,16 @@ public sealed class ClientObjectTable
|
|||
}
|
||||
|
||||
/// <summary>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.</summary>
|
||||
/// ContainerId = wielderGuid and CurrentlyEquippedLocation = equipMask — EXACTLY what the server's
|
||||
/// WieldObject 0x0023 confirm does via <see cref="MoveItem"/> (acdream models a wielded item as
|
||||
/// contained-by-the-wielder). Neither path writes WielderId, so the optimistic state equals the
|
||||
/// confirmed state and <see cref="RollbackMove"/> 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.</summary>
|
||||
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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue