Both review lenses PASSED; this is the cleanup, not a rescue. Evidence:
docs/research/2026-08-06-ap156-review-closure.md (the review itself is
committed alongside it as the received artifact).
R1 — the load-bearing containment test could not fail. Its truth and flood
values were two hand-copies of the same expression over the same part set,
so the shortfall was algebraically identically zero for any DAT input. The
oracle is now PHYSICS-POLYGON VERTICES — a different DAT field from the
bounding sphere the builder emits, so the two sides can genuinely disagree.
Sabotage-verified three ways after full cleans: dropping the bounds centre
in production reddens it (428 Setups, worst 35.869 m on 0x0200129A, matching
an independent out-of-repo sweep exactly); dropping only the scale on the
centre reddens it (326); and corrupting the TEST's own bounds oracle reddens
it (467) where under the shipped oracle that same corruption was invisible
by algebra. Renamed accordingly. A6's stale "cap control" comment corrected:
that loop is the test's own uncapped re-implementation and cannot observe a
cap regression — the cap is covered in Core.
R2 — the population was understated. 172 is AP-152's DISPATCH population;
AP-156's is 530 BSP-bearing Setups, of which 525 have a flood sphere move
and 428 fail vertex containment before the fix (412 at a 1 cm tolerance —
the review's figure; the gap is 16 Setups between 1.4 mm and 10 mm, real
geometry). 0 fail after, at any tolerance down to zero. Corrected in the
AP-156 row, the section-3 header, the C5c handoff and two test docstrings.
Dated review artifacts are left as written — "170 of 172" was correct for
what they measured, and rewriting evidence to match a later measurement
loses provenance.
A1 — BoundsCenter = default reopened at the type what the commit closed at
the seam. Dropping the default alone would NOT have closed the review's own
scenario (a copied Cylinder call site would write Vector3.Zero explicitly
and stay green), so ShadowShape's constructor is now private and BSP shapes
are built only through ShadowShape.Bsp(..., FlatCollisionSphere localBounds),
which takes radius and centre as ONE value and scales them together. There
is no expression a caller can write that carries one and drops the other.
22 construction sites converted; the same sabotage now reddens 5 Core tests
where the review's sabotage A reached 4, because both BSP producers share
one scaling path.
A2 — #333 is real and bigger than filed, and its retail question is
answered. I disassembled CObjCell::find_obj_collisions @0x0052b750 from the
PDB-paired binary myself (check_exe_pdb.py MATCH) rather than inheriting the
claim: its only early-out is sphere_path.insert_type == INITIAL_PLACEMENT_
INSERT, then it calls FindObjCollisions on every unparented non-self shadow
object UNCONDITIONALLY. Retail has NO distance pre-filter, so acdream's
"+ movement + 2f" reach filter is an invention with no register row — filed
as AP-158, carrying the disassembly, the F_EPSILON = 0.0002 m contrast, and
the measured blast radius (118 of 477 unique installed physics-BSP GfxObjs
exceed its ~2.5 m budget, 46 exceed 5 m). Active AP rows 109 -> 110.
Recorded prominently in three places a reader will hit: TALL PROPS MAY SHOW
NO VISIBLE CHANGE UNTIL #333 LANDS, and a null result at the connected gate
is EXPECTED, not evidence against AP-156.
LOW items. R3: the comment claiming the cited evidence justified the whole
cap line is corrected, but int.MaxValue on the sorting-sphere branch stays —
capping at 1 would take Spheres[0], and retail's one sphere is
CSetup::sorting_sphere, a different DAT field; capping keeps the wrong field
AND flips the substitution under-inclusive (#98/#168 direction). AP-157
already owns it. R4: acdream scales the flood sphere where retail's
find_transit_cells never reads gfxobj_scale — added as a second residual on
AP-156. R5: retail's slack constant carried into AP-158 and #333. A3: the
per-call delegate allocation is back to a cached field, still derived from
the single bounds resolver. A5: noted; b52967de's message cannot be amended.
Gates: all 44 bin/obj deleted before every verdict-deciding build, each test
run gated on a verified "Build succeeded" in the same invocation. Release
build 0 errors / 21 pre-existing warnings. Complete suite 11,208 passed /
4 skipped / 0 failed — reconciles exactly with the e2b2d04c baseline; one
test renamed, none added, removed or skipped. Nothing conflated with the
known load-sensitive flakes #302 / #308 / #321.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
208 lines
9.8 KiB
C#
208 lines
9.8 KiB
C#
using System.Collections.Generic;
|
||
using System.Collections.Immutable;
|
||
using System.Linq;
|
||
using System.Numerics;
|
||
using AcDream.Core.Physics;
|
||
using AcDream.Core.World;
|
||
using DatReaderWriter.Enums;
|
||
using DatReaderWriter.Types;
|
||
using Xunit;
|
||
|
||
namespace AcDream.Core.Tests.Physics;
|
||
|
||
/// <summary>
|
||
/// #185 (2026-07-08) — the outdoor-stairs "invisible wall" root cause: the
|
||
/// former per-part landblock shadow registration used a synthetic part-id
|
||
/// <c>entity.Id * 256u + partIndex</c> that OVERFLOWED uint32 for class-prefixed
|
||
/// landblock ids (<c>0x40</c>/<c>0x80</c>/<c>0xC0</c>…). The <c><< 8</c> dropped the
|
||
/// prefix byte, so different-class entities sharing the low 24 bits collided on
|
||
/// one shadow part-id and <c>Register</c>'s deregister-then-insert silently
|
||
/// overwrote one entity's collision geometry — rendered stair steps with NO
|
||
/// collision. The fix registers each multi-part entity via
|
||
/// <see cref="ShadowObjectRegistry.RegisterMultiPart"/> under its UNIQUE 32-bit
|
||
/// <c>entity.Id</c> (retail <c>add_shadows_to_cells</c> / <c>CPartArray::AddPartsShadow</c>).
|
||
/// </summary>
|
||
public class ShadowRegistrationOverflowTests
|
||
{
|
||
// Two real stair-entity ids from the #185 capture that share the low 24 bits
|
||
// (0xF68221) but differ in the class-prefix byte.
|
||
private const uint EntityA = 0x40F68221u;
|
||
private const uint EntityB = 0xC0F68221u;
|
||
|
||
private const uint LbId = 0xF6820000u;
|
||
private const float OffX = 0f, OffY = 0f;
|
||
|
||
// ── The root cause, as pure arithmetic ────────────────────────────────
|
||
|
||
[Fact]
|
||
public void OldPartIdScheme_OverflowsUint32_AndCollides()
|
||
{
|
||
// entity.Id * 256u == entity.Id << 8, truncated to 32 bits → the prefix
|
||
// byte falls off the top and the two distinct entities alias.
|
||
uint oldPartA = unchecked(EntityA * 256u); // 0x40F68221 << 8 → 0xF6822100
|
||
uint oldPartB = unchecked(EntityB * 256u); // 0xC0F68221 << 8 → 0xF6822100
|
||
Assert.Equal(0xF6822100u, oldPartA);
|
||
Assert.Equal(oldPartA, oldPartB); // COLLISION = the #185 bug
|
||
Assert.NotEqual(EntityA, EntityB); // …yet the entities ARE distinct
|
||
}
|
||
|
||
// ── The bug: old per-part Register loses one registration ─────────────
|
||
|
||
private static ShadowShape Cyl(Vector3 local) => ShadowShape.Cylinder(
|
||
gfxObjId: 0u, localPosition: local, localRotation: Quaternion.Identity,
|
||
scale: 1f, radius: 1f, cylHeight: 2f);
|
||
|
||
[Fact]
|
||
public void OldPerPartRegister_CollidingIds_SecondSilentlyOverwritesFirst()
|
||
{
|
||
var reg = new ShadowObjectRegistry();
|
||
|
||
// Two DIFFERENT physical objects at DIFFERENT cells, registered the OLD way
|
||
// (Register with the synthetic overflowing part-id). EntityA at cell (0,0),
|
||
// EntityB at cell (1,0) — 30 m apart in X.
|
||
var posA = new Vector3(12f, 12f, 50f); // → cell LbId|1
|
||
var posB = new Vector3(42f, 12f, 50f); // → cell LbId|9
|
||
|
||
reg.Register(unchecked(EntityA * 256u), 0u, posA, Quaternion.Identity, 1f,
|
||
OffX, OffY, LbId, ShadowCollisionType.Cylinder, 2f);
|
||
reg.Register(unchecked(EntityB * 256u), 0u, posB, Quaternion.Identity, 1f,
|
||
OffX, OffY, LbId, ShadowCollisionType.Cylinder, 2f);
|
||
|
||
// EntityA's registration is GONE (its part-id was reused by EntityB): its
|
||
// cell is empty. This is exactly the missing stair-step collision.
|
||
Assert.Empty(reg.GetObjectsInCell(LbId | 1u));
|
||
Assert.NotEmpty(reg.GetObjectsInCell(LbId | 9u));
|
||
Assert.Equal(1, reg.TotalRegistered); // one silently lost
|
||
}
|
||
|
||
// ── The fix: RegisterMultiPart keys on the unique entity.Id ───────────
|
||
|
||
[Fact]
|
||
public void RegisterMultiPart_CollidingLowBitsIds_BothSurvive()
|
||
{
|
||
var reg = new ShadowObjectRegistry();
|
||
|
||
var posA = new Vector3(12f, 12f, 50f); // → cell LbId|1
|
||
var posB = new Vector3(42f, 12f, 50f); // → cell LbId|9
|
||
|
||
reg.RegisterMultiPart(EntityA, posA, Quaternion.Identity,
|
||
new[] { Cyl(Vector3.Zero) }, 0u, EntityCollisionFlags.None,
|
||
OffX, OffY, LbId, isStatic: true);
|
||
reg.RegisterMultiPart(EntityB, posB, Quaternion.Identity,
|
||
new[] { Cyl(Vector3.Zero) }, 0u, EntityCollisionFlags.None,
|
||
OffX, OffY, LbId, isStatic: true);
|
||
|
||
// Both distinct entities survive at their own cells — no overflow collision.
|
||
Assert.Contains(reg.GetObjectsInCell(LbId | 1u), e => e.EntityId == EntityA);
|
||
Assert.Contains(reg.GetObjectsInCell(LbId | 9u), e => e.EntityId == EntityB);
|
||
Assert.Equal(2, reg.TotalRegistered);
|
||
}
|
||
|
||
// ── The builder: one BSP shape per BSP part; shells + no-BSP excluded ──
|
||
|
||
/// <summary>
|
||
/// Graph-form fixture. The bounding sphere is OFF-CENTRE by default
|
||
/// because that is the DAT-real case — a GfxObj's physics BSP is authored
|
||
/// in the GfxObj's own coordinates and 376 of the 973 installed
|
||
/// physics-BSP parts have a root sphere further from the part origin than
|
||
/// half their radius.
|
||
/// </summary>
|
||
private static GfxObjPhysics BspGfx(float radius, float centerZ = 0.75f)
|
||
{
|
||
var leaf = new PhysicsBSPNode { Type = BSPNodeType.Leaf };
|
||
return new GfxObjPhysics
|
||
{
|
||
BSP = new PhysicsBSPTree { Root = leaf },
|
||
BoundingSphere = new Sphere
|
||
{ Origin = new Vector3(0f, 0f, centerZ), Radius = radius },
|
||
Resolved = new Dictionary<ushort, ResolvedPolygon>(),
|
||
PhysicsPolygons = new Dictionary<ushort, Polygon>(),
|
||
Vertices = new VertexArray(),
|
||
};
|
||
}
|
||
|
||
/// <summary>Flat-form fixture — the production storage since I6/I7.</summary>
|
||
private static GfxObjPhysics FlatBspGfx(float radius, float centerZ)
|
||
{
|
||
var node = new FlatPhysicsBspNode(
|
||
BSPNodeType.Leaf, default, -1, -1, 0, 0,
|
||
new FlatCollisionSphere(new Vector3(0f, 0f, centerZ), radius),
|
||
new FlatIndexRange(0, 0));
|
||
GfxObjPhysics phys = BspGfx(radius, centerZ);
|
||
phys.FlatPhysicsBsp = new FlatPhysicsBsp(
|
||
0,
|
||
ImmutableArray.Create(node),
|
||
ImmutableArray<int>.Empty,
|
||
FlatPolygonTable.Empty);
|
||
return phys;
|
||
}
|
||
|
||
[Fact]
|
||
public void FromLandblockBspParts_OneShapePerBspPart_LocalTransformPreserved()
|
||
{
|
||
var meshRefs = new[]
|
||
{
|
||
new MeshRef(0x01000AC5u, Matrix4x4.CreateTranslation(0f, 0.5f, 0.4f)),
|
||
new MeshRef(0x01000AC5u, Matrix4x4.CreateTranslation(0f, 1.0f, 0.8f)),
|
||
new MeshRef(0x0BADBADu, Matrix4x4.Identity), // no physics BSP → skipped
|
||
};
|
||
|
||
var shapes = ShadowShapeBuilder.FromLandblockBspParts(
|
||
meshRefs, isBuildingShell: false,
|
||
getGfxObj: id => id == 0x01000AC5u ? BspGfx(1.05f) : null);
|
||
|
||
Assert.Equal(2, shapes.Count); // only the two BSP-bearing parts
|
||
Assert.All(shapes, s => Assert.Equal(ShadowCollisionType.BSP, s.CollisionType));
|
||
Assert.All(shapes, s => Assert.Equal(0x01000AC5u, s.GfxObjId));
|
||
// Local part offsets survive (decomposed from PartTransform).
|
||
Assert.Contains(shapes, s => Vector3.Distance(s.LocalPosition, new Vector3(0f, 0.5f, 0.4f)) < 1e-4f);
|
||
Assert.Contains(shapes, s => Vector3.Distance(s.LocalPosition, new Vector3(0f, 1.0f, 0.8f)) < 1e-4f);
|
||
// Radius = local BoundingSphere radius × part scale (1.0).
|
||
Assert.All(shapes, s => Assert.Equal(1.05f, s.Radius, 3));
|
||
}
|
||
|
||
/// <summary>
|
||
/// AP-156, landblock half. A landblock-baked multi-part entity is the
|
||
/// same <c>CPartArray</c> walk as a live Setup
|
||
/// (<c>CPhysicsObj::find_bbox_cell_list</c> @0x00510fc0 →
|
||
/// <c>CPartArray::calc_cross_cells_static</c> @0x00518160 →
|
||
/// <c>CEnvCell::find_transit_cells</c> @0x0052cae0), which transforms each
|
||
/// part's <c>CGfxObj::physics_sphere</c> CENTRE through the part's own
|
||
/// Position before reading its radius. Carrying the radius alone puts a
|
||
/// stair run's or fence's flood sphere at the part origin instead of on
|
||
/// its geometry. Both storage forms — the flat BSP that production uses
|
||
/// and the graph fallback — must supply the centre, SCALED with the part.
|
||
/// </summary>
|
||
[Fact]
|
||
public void FromLandblockBspParts_CarriesTheScaledRootSphereCentre()
|
||
{
|
||
Matrix4x4 halfScale = Matrix4x4.CreateScale(0.5f)
|
||
* Matrix4x4.CreateTranslation(0f, 2f, 0f);
|
||
|
||
var flat = ShadowShapeBuilder.FromLandblockBspParts(
|
||
[new MeshRef(0x01000AC5u, halfScale)],
|
||
isBuildingShell: false,
|
||
getGfxObj: _ => FlatBspGfx(4f, centerZ: 3f));
|
||
ShadowShape flatShape = Assert.Single(flat);
|
||
Assert.Equal(0.5f, flatShape.Scale, 3);
|
||
Assert.Equal(2f, flatShape.Radius, 3); // 4 * 0.5
|
||
Assert.Equal(new Vector3(0f, 0f, 1.5f), flatShape.BoundsCenter); // 3 * 0.5
|
||
|
||
// Graph fallback (fixtures without a flat BSP) takes the same path.
|
||
var graph = ShadowShapeBuilder.FromLandblockBspParts(
|
||
[new MeshRef(0x01000AC5u, halfScale)],
|
||
isBuildingShell: false,
|
||
getGfxObj: _ => BspGfx(4f, centerZ: 3f));
|
||
ShadowShape graphShape = Assert.Single(graph);
|
||
Assert.Equal(new Vector3(0f, 0f, 1.5f), graphShape.BoundsCenter);
|
||
}
|
||
|
||
[Fact]
|
||
public void FromLandblockBspParts_BuildingShell_ReturnsEmpty()
|
||
{
|
||
var meshRefs = new[] { new MeshRef(0x01000AC5u, Matrix4x4.Identity) };
|
||
var shapes = ShadowShapeBuilder.FromLandblockBspParts(
|
||
meshRefs, isBuildingShell: true, getGfxObj: _ => BspGfx(1f));
|
||
Assert.Empty(shapes); // building shells collide via the building channel
|
||
}
|
||
}
|