From 88348f679156edd3d4e0c2e06090a52af57c477a Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 3 Aug 2026 20:13:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(physics):=20#299=20=E2=80=94=20port=20retai?= =?UTF-8?q?l's=20mover-side=20IsImpenetrable=20exemption=20branch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CollisionExemption checked only the TARGET's IsImpenetrable, and the class doc asserted "retail's pseudo-C only checks the target's IsImpenetrable(); acdream follows retail" while blaming ACE for checking both. That was backwards: ACE was retail-faithful and acdream was missing half the check. Retail short-circuits on EITHER the mover's own state & IS_IMPENETRABLE (0x80) OR the target's IsImpenetrable(); either alone exempts. Verified at the byte level rather than from the decompiler's rendering — Binary Ninja shows the mover test as `int16_t state_1 ... if (state_1 < 0)`, which reads like a 0x8000 test, but decoding the PDB-paired binary at the mapped offset gives: 8b 43 04 mov eax,[ebx+4] ; mover object_info.state f6 c4 01 test ah,1 ; 0x100 IsPlayer 84 c0 test al,al ; sign bit of AL = state & 0x80 78 3d js ... ; -> collide `test al, al; js` is a byte-level sign test on AL, i.e. 0x80, not 0x8000. Corroborated downstream in the same block (`test ah,8` = 0x800 IsPK, `test ah,0x10` = 0x1000 IsPKLite) and by OBJECTINFO::init @0x0050cf30 setting state |= 0x80 from the object's own IsImpenetrable(). Also corrected: ACCWeenieObject::IsImpenetrable @0x0058c8c0 returns (_bitfield >> 0x15) & 1 — retail genuinely conflates BF_FREE_PKSTATUS with "impenetrable", so acdream's FromPwdBitfield decode was already right. Both retail arms set collide, so ordering between them is semantically free and a misreading here could only ever produce spurious collisions, never a walk-through. Found while investigating #297; not symptom-causing on its own. No divergence row: this retires a missing port rather than introducing a deviation, and nothing in the register or the collision digest's DO-NOT-RETRY tables covers it. Gates: complete Release solution 10,887 passed / 4 skipped / 0 failed (baseline 10,867/4/0). Adversarial + retail-conformance review PASS on this change specifically. Both new tests discrimination-verified by reverting the branch and confirming failure. Co-Authored-By: Claude Opus 5 --- .../Physics/CollisionExemption.cs | 30 +++++++++++++----- .../Physics/CollisionExemptionTests.cs | 31 +++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/AcDream.Core/Physics/CollisionExemption.cs b/src/AcDream.Core/Physics/CollisionExemption.cs index dc22e7df..58244bd0 100644 --- a/src/AcDream.Core/Physics/CollisionExemption.cs +++ b/src/AcDream.Core/Physics/CollisionExemption.cs @@ -32,10 +32,21 @@ namespace AcDream.Core.Physics; /// /// Cross-checked against ACE /// references/ACE/Source/ACE.Server/Physics/PhysicsObj.cs:381-405 -/// (line-for-line C# port of the same logic). Note: ACE adds -/// state.HasFlag(IsImpenetrable) (mover-impenetrable) to the -/// collide list; retail's pseudo-C only checks the target's -/// IsImpenetrable(). acdream follows retail. +/// (line-for-line C# port of the same logic). #299: retail's pseudo-C +/// (acclient_2013_pseudo_c.txt:276824-276827) checks the MOVER's own +/// state & IS_IMPENETRABLE (0x80) — the sign-bit test on the +/// truncated 16-bit ebx->object_info.state the decompiler renders as +/// if (state_1 < 0), sitting immediately before the target's +/// IsImpenetrable() check at the same nesting level — in addition to +/// the target's own IsImpenetrable(), either alone disqualifying the +/// tentative PvP exemption. ACE's port has both; acdream previously had only +/// the target half. The bit position is confirmed against +/// OBJECTINFO::init@0x0050cf30 (this->state |= 0x80 when +/// weenie_obj->IsImpenetrable()) and ACE's own +/// ObjectInfoState.IsImpenetrable = 0x80 +/// (ACE.Server/Physics/ObjectInfo.cs:17) — the earlier comment here +/// blaming ACE for an addition was itself the divergence; ACE was +/// retail-faithful and acdream was missing the mover half. /// /// public static class CollisionExemption @@ -98,9 +109,14 @@ public static class CollisionExemption // if any of the COLLIDE conditions hold. bool collide = false; - // 4a. Impenetrable target → collide. - // acclient_2013_pseudo_c.txt:276826. - if ((targetFlags & EntityCollisionFlags.IsImpenetrable) != 0) + // 4a. Impenetrable target OR impenetrable mover → collide. + // acclient_2013_pseudo_c.txt:276824-276827 — retail checks + // BOTH: the mover's own state&0x80 (IS_IMPENETRABLE) short- + // circuits first, then the target's IsImpenetrable(). #299: + // acdream previously ported only the target half. + if ((moverState & ObjectInfoState.IsImpenetrable) != 0) + collide = true; + if (!collide && (targetFlags & EntityCollisionFlags.IsImpenetrable) != 0) collide = true; // 4b. Both PK → collide. diff --git a/tests/AcDream.Core.Tests/Physics/CollisionExemptionTests.cs b/tests/AcDream.Core.Tests/Physics/CollisionExemptionTests.cs index 4d20f46d..586c3496 100644 --- a/tests/AcDream.Core.Tests/Physics/CollisionExemptionTests.cs +++ b/tests/AcDream.Core.Tests/Physics/CollisionExemptionTests.cs @@ -170,4 +170,35 @@ public class CollisionExemptionTests targetFlags: EntityCollisionFlags.IsPlayer, moverState: ObjectInfoState.None)); } + + [Fact] + public void ImpenetrableMover_VsOrdinaryPlayer_NotSkipped() + { + // #299: retail's pseudo-C (acclient_2013_pseudo_c.txt:276824-276827) + // disqualifies the tentative PvP exemption when the MOVER itself + // carries state&0x80 (IS_IMPENETRABLE) — not just the target's + // IsImpenetrable(). Without this branch an Impenetrable ("Free" PK + // status) mover would incorrectly walk through an ordinary + // (non-PK, non-Impenetrable) player target, the mirror image of + // ImpenetrableTarget_VsAnyPlayer_NotSkipped above. + Assert.False(CollisionExemption.ShouldSkip( + targetState: 0u, + targetFlags: EntityCollisionFlags.IsPlayer, + moverState: ObjectInfoState.IsPlayer | ObjectInfoState.IsImpenetrable)); + } + + [Fact] + public void ImpenetrableMover_StillCollidesEvenWhenAlsoPk() + { + // The mover-impenetrable disqualifier (4a) is checked before the + // PK/PKLite arms (4b/4c) but must not depend on them — an + // Impenetrable mover collides with a plain player target even + // though neither side is PK/PKLite. + Assert.False(CollisionExemption.ShouldSkip( + targetState: 0u, + targetFlags: EntityCollisionFlags.IsPlayer, + moverState: ObjectInfoState.IsPlayer + | ObjectInfoState.IsImpenetrable + | ObjectInfoState.IsPK)); + } }