fix(D.2b): PaperdollController review fixes — player-scope Concerns, loop clarity
Code-quality review on Task 5: - I1: Concerns was unscoped (CurrentlyEquippedLocation != None) → an NPC's wielded item (which also carries that wire field) triggered spurious full repaints. Narrowed to (WielderId==p || ContainerId==p), matching InventoryController; OnObjectMoved's from/to-player backstop still catches unwield-into-a-side-bag. Populate's own scope already prevented wrong data; this kills the wasted repaints. - I2: replaced the dual-`index++` (assign-vs-skip) with a for-i loop; SlotIndex = SlotMap position (= the drag payload's SourceSlot on unwield). - M1/M2: comment that the cell's SpriteResolve + the discrete-slot accept/ reject ring (0x060011F9/F8, not the grid insert-arrow) are factory-provided. - Added two behavioral tests: a live player wield repaints the slot (ObjectMoved → Concerns → Populate); an NPC's wielded item never appears on the doll (player-scoping). - Synced the stale spec §4b/§6c/§8 to the Task-3 Option-1 reality (the optimistic wield is ContainerId-based and does NOT write WielderId). App suite 580 passed / 0 failed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
9f187c3e31
commit
3b8a39c49e
3 changed files with 54 additions and 15 deletions
|
|
@ -155,20 +155,22 @@ This makes rollback faithful in **both** directions (today an unwield-reject los
|
||||||
|
|
||||||
```csharp
|
```csharp
|
||||||
/// <summary>Optimistic (instant) wield: snapshot the pre-wield (container, slot, equip), then set
|
/// <summary>Optimistic (instant) wield: snapshot the pre-wield (container, slot, equip), then set
|
||||||
/// ContainerId = WielderId = wielderGuid and CurrentlyEquippedLocation = equipMask (matching the
|
/// ContainerId = wielderGuid and CurrentlyEquippedLocation = equipMask (matching the
|
||||||
/// server's WieldObject 0x0023 confirm), firing ObjectMoved for an immediate repaint. The caller
|
/// 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>
|
/// sends GetAndWieldItem; ConfirmMove (on the 0x0023 echo) / RollbackMove (on 0x00A0) reconcile.</summary>
|
||||||
public bool WieldItemOptimistic(uint itemId, uint wielderGuid, EquipMask equipMask)
|
public bool WieldItemOptimistic(uint itemId, uint wielderGuid, EquipMask equipMask)
|
||||||
{
|
{
|
||||||
if (!_objects.TryGetValue(itemId, out var item)) return false;
|
if (!_objects.TryGetValue(itemId, out var item)) return false;
|
||||||
RecordPending(itemId, item);
|
RecordPending(itemId, item);
|
||||||
item.WielderId = wielderGuid; // unambiguously the player's gear during the optimistic window
|
|
||||||
return MoveItem(itemId, wielderGuid, newSlot: -1, newEquipLocation: equipMask);
|
return MoveItem(itemId, wielderGuid, newSlot: -1, newEquipLocation: equipMask);
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
`MoveItem` already sets `ContainerId` + `CurrentlyEquippedLocation` + reindexes + fires `ObjectMoved`.
|
`MoveItem` already sets `ContainerId` + `CurrentlyEquippedLocation` + reindexes + fires `ObjectMoved`.
|
||||||
Setting `WielderId` here matches the steady state (the item's own CreateObject carries `Wielder`).
|
It does **not** write `WielderId` — acdream's `WieldObject 0x0023` confirm is also ContainerId-based (it
|
||||||
|
never sets `WielderId`), so the optimistic state equals the confirmed state and rollback fully restores
|
||||||
|
through `MoveItem` alone. (An equipped item is detected as the player's via `ContainerId == player`;
|
||||||
|
login-equipped items match via `WielderId` from their own CreateObject. Decided at the Task-3 code review.)
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
@ -239,9 +241,10 @@ jewelry — a single equip bit intersects exactly one slot mask). If found, `cel
|
||||||
iconIds(item.Type, item.IconId, item.IconUnderlayId, item.IconOverlayId, item.Effects))`; else
|
iconIds(item.Type, item.IconId, item.IconUnderlayId, item.IconOverlayId, item.Effects))`; else
|
||||||
`cell.Clear()` (transparent — nothing drawn).
|
`cell.Clear()` (transparent — nothing drawn).
|
||||||
|
|
||||||
`Concerns(o)`: repaint when `o.CurrentlyEquippedLocation != None || o.WielderId == player || o.ContainerId == player`;
|
`Concerns(o)`: repaint when `o.WielderId == player || o.ContainerId == player` (player-scoped — an NPC's
|
||||||
`OnObjectMoved` also repaints when from/to is the player (an item being wielded/unwielded). Mirror
|
wielded item carries `CurrentlyEquippedLocation` too and must NOT leak onto the doll; a player-equipped item
|
||||||
`InventoryController`'s debounce.
|
always has `WielderId==player` or `ContainerId==player`). `OnObjectMoved` also repaints when from/to is the
|
||||||
|
player (an item being wielded/unwielded into a side bag). Mirror `InventoryController`'s debounce.
|
||||||
|
|
||||||
### 6d. `IItemListDragHandler`
|
### 6d. `IItemListDragHandler`
|
||||||
|
|
||||||
|
|
@ -295,8 +298,9 @@ _paperdollController = AcDream.App.UI.Layout.PaperdollController.Bind(
|
||||||
confidence (the contents grid proves the same `0x2100003D` base path), but load-bearing — if it fails,
|
confidence (the contents grid proves the same `0x2100003D` base path), but load-bearing — if it fails,
|
||||||
fix the importer/factory before continuing.
|
fix the importer/factory before continuing.
|
||||||
- **Core — `EquipMaskTests`:** numeric pin of every member vs `acclient.h:3193`.
|
- **Core — `EquipMaskTests`:** numeric pin of every member vs `acclient.h:3193`.
|
||||||
- **Core — `ClientObjectTableTests`:** `WieldItemOptimistic` sets equip + WielderId + container; `RollbackMove`
|
- **Core — `ClientObjectTableTests`:** `WieldItemOptimistic` sets equip + container (ContainerId-based; does
|
||||||
restores the pre-wield equip mask (the new snapshot field); outstanding-count across wield+move of the same item.
|
NOT write WielderId); `RollbackMove` restores the pre-wield equip mask (the new snapshot field);
|
||||||
|
outstanding-count across wield+move of the same item.
|
||||||
- **Core.Net — `GameEventWiringTests`:** `WieldObject 0x0023` now calls `ConfirmMove` (an optimistic wield's
|
- **Core.Net — `GameEventWiringTests`:** `WieldObject 0x0023` now calls `ConfirmMove` (an optimistic wield's
|
||||||
snapshot clears on the echo).
|
snapshot clears on the echo).
|
||||||
- **App — `PaperdollControllerTests`:** element-id→mask map matches the dump; populate shows the equipped item
|
- **App — `PaperdollControllerTests`:** element-id→mask map matches the dump; populate shows the equipped item
|
||||||
|
|
|
||||||
|
|
@ -57,14 +57,18 @@ public sealed class PaperdollController : IItemListDragHandler
|
||||||
{
|
{
|
||||||
_objects = objects; _playerGuid = playerGuid; _iconIds = iconIds; _sendWield = sendWield;
|
_objects = objects; _playerGuid = playerGuid; _iconIds = iconIds; _sendWield = sendWield;
|
||||||
|
|
||||||
int index = 0;
|
for (int i = 0; i < SlotMap.Length; i++)
|
||||||
foreach (var (element, mask) in SlotMap)
|
|
||||||
{
|
{
|
||||||
if (layout.FindElement(element) is not UiItemList list) { index++; continue; }
|
var (element, mask) = SlotMap[i];
|
||||||
|
if (layout.FindElement(element) is not UiItemList list) continue;
|
||||||
list.RegisterDragHandler(this);
|
list.RegisterDragHandler(this);
|
||||||
list.Cell.SourceKind = ItemDragSource.Equipment;
|
list.Cell.SourceKind = ItemDragSource.Equipment;
|
||||||
list.Cell.SlotIndex = index++; // a stable per-slot index (routing uses the list, not this)
|
list.Cell.SlotIndex = i; // SlotMap position = this equipped item's drag-payload SourceSlot when dragged out to unwield
|
||||||
list.Cell.EmptySprite = 0u; // TRANSPARENT empty slot (brainstorm decision; no doll behind it)
|
list.Cell.EmptySprite = 0u; // TRANSPARENT empty slot (brainstorm decision; no doll behind it in Slice 1)
|
||||||
|
// Cell.SpriteResolve + the default accept/reject sprites (ItemSlot_DragOver_Accept ring
|
||||||
|
// 0x060011F9 / reject circle 0x060011F8 — the discrete-slot frames, NOT the inventory grid's
|
||||||
|
// insert-arrow 0x060011F7) are already wired by DatWidgetFactory when it built the UiItemList;
|
||||||
|
// no need to re-set them here.
|
||||||
_slots.Add((mask, list));
|
_slots.Add((mask, list));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -85,11 +89,16 @@ public sealed class PaperdollController : IItemListDragHandler
|
||||||
private void OnObjectMoved(ClientObject o, uint from, uint to)
|
private void OnObjectMoved(ClientObject o, uint from, uint to)
|
||||||
{ if (Concerns(o) || from == _playerGuid() || to == _playerGuid()) Populate(); }
|
{ if (Concerns(o) || from == _playerGuid() || to == _playerGuid()) Populate(); }
|
||||||
|
|
||||||
/// <summary>The object is (or just became / ceased to be) the player's equipped gear.</summary>
|
/// <summary>The object belongs to the player (wielded gear or pack contents) — so a change to it may
|
||||||
|
/// add/remove/repaint a doll slot. Player-scoped: an NPC's or vendor's wielded item (which also carries
|
||||||
|
/// CurrentlyEquippedLocation from the wire) must NOT trigger a repaint. A player-equipped item always
|
||||||
|
/// has WielderId==p (login, from CreateObject) or ContainerId==p (live/optimistic wield, set by
|
||||||
|
/// WieldItemOptimistic), so the equip-location need not be tested here; OnObjectMoved adds the from/to
|
||||||
|
/// player backstop for moves that transiently satisfy neither (e.g. unwield into a side bag).</summary>
|
||||||
private bool Concerns(ClientObject o)
|
private bool Concerns(ClientObject o)
|
||||||
{
|
{
|
||||||
uint p = _playerGuid();
|
uint p = _playerGuid();
|
||||||
return o.CurrentlyEquippedLocation != EquipMask.None || o.WielderId == p || o.ContainerId == p;
|
return o.WielderId == p || o.ContainerId == p;
|
||||||
}
|
}
|
||||||
|
|
||||||
public void Populate()
|
public void Populate()
|
||||||
|
|
|
||||||
|
|
@ -118,4 +118,30 @@ public class PaperdollControllerTests
|
||||||
Bind(layout, new ClientObjectTable());
|
Bind(layout, new ClientObjectTable());
|
||||||
Assert.Equal(0u, lists[HeadSlot].Cell.EmptySprite);
|
Assert.Equal(0u, lists[HeadSlot].Cell.EmptySprite);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Live_player_wield_repaints_the_slot() // event-driven: ObjectMoved → Concerns → Populate
|
||||||
|
{
|
||||||
|
var (layout, lists) = BuildLayout();
|
||||||
|
var objects = new ClientObjectTable();
|
||||||
|
Bind(layout, objects); // slots empty at bind
|
||||||
|
SeedPackItem(objects, 0xF01u, EquipMask.HeadWear); // a helm appears in the pack (not on the doll)
|
||||||
|
Assert.Equal(0u, lists[HeadSlot].Cell.ItemId);
|
||||||
|
objects.WieldItemOptimistic(0xF01u, Player, EquipMask.HeadWear); // player wields it → ObjectMoved(to=player)
|
||||||
|
Assert.Equal(0xF01u, lists[HeadSlot].Cell.ItemId); // the head slot repainted with the helm
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Live_npc_equip_does_not_appear_on_the_doll() // player-scoped: a remote creature's wielded item never leaks
|
||||||
|
{
|
||||||
|
var (layout, lists) = BuildLayout();
|
||||||
|
var objects = new ClientObjectTable();
|
||||||
|
Bind(layout, objects);
|
||||||
|
const uint npc = 0x60000001u;
|
||||||
|
objects.AddOrUpdate(new ClientObject
|
||||||
|
{
|
||||||
|
ObjectId = 0xF02u, WielderId = npc, CurrentlyEquippedLocation = EquipMask.HeadWear,
|
||||||
|
});
|
||||||
|
Assert.Equal(0u, lists[HeadSlot].Cell.ItemId); // the NPC's helm is NOT on the player's doll
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue