fix(physics): #297 — keep the PWD bitfield live so PK status reaches the client

The user typed @pklite and then walked straight through other PKLite players.

Root cause: ClientObject.PublicWeenieBitfield was written exactly once, from the
0xF745 CreateObject parse, and never refreshed. ACE's only PK-change message is
PropertyInt.PlayerKillerStatus (134) over 0x02CE/0x02CD, which we parsed and
stored into Properties.Ints[134] but never translated back into the bitfield —
and ACE never re-sends a PublicWeenieDesc at all (EnqueueBroadcastUpdateObject
has zero live callers), so that property is the ONLY signal a client can learn
from. Both sides of the collision test read the frozen value, so
CollisionExemption's "4c. both PKLite -> collide" rule could never fire.

Retail's missing port: PublicWeenieDesc::SetPlayerKillerStatus @0x005AC7C0
rewrites _bitfield in place — PK(4) -> (b & 0xfddfffff) | 0x20; PKLite(0x40) ->
(b & 0xffdfffdf) | 0x2000000; Free(0x20) -> (b & 0xfdffffdf) | 0x200000; else
b &= 0xfddfffdf. Mutually exclusive, verified byte-for-byte, with input values
confirmed against retail's own PKStatusEnum (acclient.h:6412-6427), not just
ACE's. Driven from ACCWeenieObject::OnStatUpdated @0x0058DF20 case 0x86.

The fix rewrites the value at its source rather than patching consumers. Two
review rounds were needed because the first pass missed that there are TWO
snapshot stores: InboundPhysicsStateController keeps its own private _snapshots
dictionary, and every untimestamped-field merge (ApplyAcceptedObjDesc and
friends) reads `old` from THAT store, not from RuntimeEntityRecord.Snapshot.
Refreshing only the active record left the target-side shadow flags correct
until the remote's next equip or unequip — ACE broadcasts an ObjDesc on every
one — at which point the appearance path rebuilt the registration from the
frozen spawn and dropped the bit permanently. The regression test demanded by
review is what surfaced that; it is verified discriminating (reverting gives
Actual: 8 instead of 33554440).

Five stores now hold this value, kept coherent from one source by two
ObjectUpdated subscribers plus the appearance-rebuild path. The two shadow-flag
writers are the same invalidation applied at the two edges that can invalidate
it, not competing authorities — review enumerated every drift path and closed
each. That coherence invariant is new as of this commit and is recorded as
register row AP-134, with AP-133 as the precedent for filing a row when the
danger is a future writer rather than current behaviour.

Also corrects TS-23's retirement narrative, which claimed every mover-flags call
site read the mover's "real" PK bits from 2026-07-30. The bits existed but their
source was frozen, so that only became true here; the site enumeration also
missed RuntimeSetPositionMoverPreparation, a seventh site that decodes the
snapshot directly.

Unblocks #298 (melee/missile admission needs the local player's own PKLite bit).
Follow-ups filed: #300 (Properties.Ints[134] vs bitfield mirror gap), #301 (same
defect class for radar blip colour and radar behaviour), #302 (a pre-existing
PortalProjection allocation-assertion flake, 1 in 6, found while verifying this
gate), #303 (LiveEntityPvpBitfieldSync is App-resident but Runtime-owned-state).

Gates: complete Release solution 10,895 passed / 4 skipped / 0 failed (baseline
10,887 including #299). Adversarial + retail-conformance review PASS after one
FAIL round. Every new test discrimination-verified by reverting the fix.
Connected acceptance NOT run — needs a live two-client PKLite session.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-03 20:59:01 +02:00
parent 88348f6791
commit 9b1e6fc637
19 changed files with 1329 additions and 5 deletions

View file

@ -1178,6 +1178,42 @@ public sealed class InboundPhysicsStateController
return true;
}
/// <summary>
/// #297 review round 2 (F1/F2): rewrites <c>ObjectDescriptionFlags</c> on
/// THIS store's retained snapshot — the same base every ObjDesc/Pickup/
/// Parent/etc. merge in this class reads as <c>old</c>/<c>retained</c>
/// (see <see cref="ApplyAcceptedObjDesc"/>,
/// <see cref="ApplyAcceptedWeenieDescriptionSnapshot"/>). Without this, a
/// live PK-status rewrite applied only to
/// <c>RuntimeEntityRecord.Snapshot</c> (the ACTIVE record's copy) would
/// still be reverted by the NEXT untimestamped-field merge, because every
/// such merge starts from <c>_snapshots[guid]</c>, not the active
/// record — the two stores are related but distinct, exactly the
/// hazard the Round 3 A1 seam (<see cref="ApplyAcceptedObjDescSnapshot"/>)
/// was introduced to close for OTHER fields. No-op (returns false) if no
/// CreateObject has ever seeded this guid, or if the bitfield is
/// unchanged.
/// </summary>
internal bool TryRefreshObjectDescriptionFlags(
uint guid,
uint bitfield,
out WorldSession.EntitySpawn merged)
{
if (!_snapshots.TryGetValue(guid, out WorldSession.EntitySpawn retained))
{
merged = default;
return false;
}
if (retained.ObjectDescriptionFlags == bitfield)
{
merged = retained;
return false;
}
merged = retained with { ObjectDescriptionFlags = bitfield };
_snapshots[guid] = merged;
return true;
}
private static SameGenerationCreateObjectEvents BuildSameGenerationEvents(
WorldSession.EntitySpawn incoming)
{

View file

@ -642,6 +642,16 @@ public sealed class RuntimeEntityDirectory
out WorldSession.EntitySpawn merged) =>
_inbound.ApplyAcceptedWeenieDescriptionSnapshot(guid, incoming, out merged);
/// <summary>
/// #297 review round 2: see
/// <see cref="InboundPhysicsStateController.TryRefreshObjectDescriptionFlags"/>.
/// </summary>
internal bool TryRefreshObjectDescriptionFlags(
uint guid,
uint bitfield,
out WorldSession.EntitySpawn merged) =>
_inbound.TryRefreshObjectDescriptionFlags(guid, bitfield, out merged);
private bool IsKnown(RuntimeEntityRecord record)
{
if (IsCurrent(record))

View file

@ -163,6 +163,14 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
private readonly List<Func<int>> _firstEntryDriveOwnership = [];
/// <summary>C4 route 2: see <see cref="RegisterAcceptedPositionDriveOwnership"/>.</summary>
private readonly List<Func<int>> _acceptedPositionDriveOwnership = [];
/// <summary>
/// #297 (review round 2, preferred fix): keeps every canonical
/// snapshot's <c>ObjectDescriptionFlags</c> live against
/// <c>ClientObjectTable.PublicWeenieBitfield</c> — see
/// <see cref="RuntimeEntityPvpBitfieldSnapshotSync"/> for the full
/// rationale.
/// </summary>
private readonly RuntimeEntityPvpBitfieldSnapshotSync _pvpBitfieldSync;
public RuntimeEntityObjectLifetime(
uint firstLocalEntityId = RuntimeEntityDirectory.FirstLocalEntityId,
@ -175,6 +183,7 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
timeProvider: timeProvider,
gameClock: gameClock);
Objects = new ClientObjectTable();
_pvpBitfieldSync = new RuntimeEntityPvpBitfieldSnapshotSync(Entities, Objects);
// AP-129 (Campaign P Slice P4 review fix, 2026-07-30): the physics
// entry-restriction gate (ObjectInfo.CheckEntryRestrictions) resolves
// a restricted cell's owner/guest list through the SAME live
@ -263,6 +272,7 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
timeProvider,
gameClock);
Objects = new ClientObjectTable();
_pvpBitfieldSync = new RuntimeEntityPvpBitfieldSnapshotSync(Entities, Objects);
Physics.Engine.Objects = Objects;
var views = new RuntimeEntityObjectViews(Entities, Objects);
EntityView = views.Entities;
@ -345,6 +355,7 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
timeProvider,
gameClock);
Objects = new ClientObjectTable();
_pvpBitfieldSync = new RuntimeEntityPvpBitfieldSnapshotSync(Entities, Objects);
Physics.Engine.Objects = Objects;
var views = new RuntimeEntityObjectViews(Entities, Objects);
EntityView = views.Entities;
@ -2009,6 +2020,7 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
finally
{
_disposed = true;
_pvpBitfieldSync.Dispose();
Events.Dispose();
Physics.Dispose();
}

View file

@ -0,0 +1,96 @@
using AcDream.Core.Items;
using AcDream.Core.Net;
namespace AcDream.Runtime.Entities;
/// <summary>
/// #297 (preferred fix, review round 2): rewrites the canonical Runtime
/// snapshot's <c>ObjectDescriptionFlags</c> (retail
/// <c>PublicWeenieDesc._bitfield</c>) in place whenever a live
/// <c>PropertyInt.PlayerKillerStatus</c> update lands on
/// <see cref="ClientObjectTable"/>, mirroring the same
/// <c>PlayerKillerStatusBitfield</c> rewrite
/// <c>ClientObjectTable.UpdateIntProperty</c> already applies to
/// <see cref="ClientObject.PublicWeenieBitfield"/>.
///
/// <para>
/// Fixing it at the SOURCE — the one <see cref="RuntimeEntityRecord.Snapshot"/>
/// every subsystem reads — means every downstream consumer inherits the live
/// value by construction, with no second writer:
/// </para>
/// <list type="bullet">
/// <item>ObjDesc/appearance rebuild
/// (<c>LiveEntityCollisionBuilder.Build</c> reads
/// <c>spawn.ObjectDescriptionFlags</c> from the very snapshot this class
/// keeps live) no longer reverts a live PK-status change on the next
/// equip/dequip — the F1 defect from review round 2.</item>
/// <item>Placement/teleport/spawn-settle mover-flags resolution
/// (<c>RuntimeSetPositionMoverPreparation</c> reads
/// <c>record.Snapshot.ObjectDescriptionFlags</c> directly, not the live
/// table) sees the live value too — F2.</item>
/// <item>The vivid target indicator
/// (<c>WorldSelectionQuery.ResolveVividTargetInfo</c> reads
/// <c>LiveEntityRuntime.TryGetSnapshot</c>, the App-side mirror of this
/// same Runtime snapshot) is fixed for free — F4.</item>
/// </list>
///
/// <para>
/// Every ClientObjectTable update — not just PropertyInt 134 — fires
/// <see cref="ClientObjectTable.ObjectUpdated"/>, so the idempotency guard
/// inside <see cref="RuntimeEntityDirectory.TryRefreshObjectDescriptionFlags"/>
/// is load-bearing: without it, an unrelated property change (e.g. an
/// inventory EncumbranceVal move) would still allocate a new snapshot record
/// for no reason. This mirrors the review's F3 guidance for the target-side
/// shadow-registry sync.
/// </para>
///
/// <para>
/// There are TWO related-but-distinct snapshot stores in Runtime: the ACTIVE
/// <see cref="RuntimeEntityRecord.Snapshot"/> every subsystem reads, and
/// <c>InboundPhysicsStateController</c>'s own retained copy that every
/// untimestamped-field merge (ObjDesc, same-generation CreateObject, ...)
/// uses as its <c>old</c>/<c>retained</c> base. Rewriting only the active
/// record's copy is not enough — the NEXT such merge would revert it, because
/// it starts from the OTHER store. This is exactly the hazard the Round 3 A1
/// seam (<c>ApplyAcceptedObjDescSnapshot</c>) was introduced to close for
/// other fields; <see cref="RuntimeEntityDirectory.TryRefreshObjectDescriptionFlags"/>
/// keeps both stores in lockstep for this one.
/// </para>
/// </summary>
internal sealed class RuntimeEntityPvpBitfieldSnapshotSync : IDisposable
{
private readonly RuntimeEntityDirectory _entities;
private readonly ClientObjectTable _objects;
private bool _disposed;
public RuntimeEntityPvpBitfieldSnapshotSync(
RuntimeEntityDirectory entities,
ClientObjectTable objects)
{
_entities = entities ?? throw new ArgumentNullException(nameof(entities));
_objects = objects ?? throw new ArgumentNullException(nameof(objects));
_objects.ObjectUpdated += OnObjectUpdated;
}
private void OnObjectUpdated(ClientObject item)
{
if (item.PublicWeenieBitfield is not { } bitfield)
return;
if (!_entities.TryRefreshObjectDescriptionFlags(
item.ObjectId,
bitfield,
out WorldSession.EntitySpawn merged))
{
return; // unchanged, or no CreateObject snapshot exists for this guid
}
if (_entities.TryGetActive(item.ObjectId, out RuntimeEntityRecord record))
_entities.RefreshSnapshot(record, merged, refreshPosition: false);
}
public void Dispose()
{
if (_disposed) return;
_disposed = true;
_objects.ObjectUpdated -= OnObjectUpdated;
}
}