From 702b30a63edee455d9e20520b5dc8ea36f67dd78 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 19 May 2026 18:01:44 +0200 Subject: [PATCH] =?UTF-8?q?refactor(physics):=20Phase=202=20=E2=80=94=20co?= =?UTF-8?q?de-review=20polish=20on=20BuildingPhysics=20commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five reviewer-flagged items addressed: - Fix #1: GameWindow building-loop now reuses TerrainSurface.ComputeOutdoorCellId instead of re-deriving the row-major cell-index formula. DRY win; no risk of the two formulas drifting. - Fix #2: BuildingPhysics.ExactMatch decoder now references DatReaderWriter.Enums.PortalFlags.ExactMatch instead of magic 0x0001. - Fix #3: ExactMatch XML doc clarified as "reserved per retail's CBldPortal::exact_match; not currently consumed by CheckBuildingTransit". - Fix #4: CheckBuildingTransit docstring now explicitly documents the retail divergence — retail's sphere_intersects_cell (radius-aware) vs. our PointInsideCellBsp (radius-less). The sphereRadius parameter is reserved for the future sphere_intersects_cell port. Practical effect noted: entry fires ~sphereRadius (~0.48m) deeper than retail. - Fix #5: Test method `SphereInsideBuildingPortalDestination_AddsInteriorCell` renamed to `BuildingPortalWithUnloadedCellBSP_NoCandidateAdded` — the test asserts Empty(candidates), not that the cell is added. Comment updated. Spec: docs/superpowers/specs/2026-05-19-indoor-portal-cell-tracking-design.md Plan: docs/superpowers/plans/2026-05-19-indoor-portal-cell-tracking.md Co-Authored-By: Claude Opus 4.7 (1M context) --- src/AcDream.App/Rendering/GameWindow.cs | 15 ++++++--------- src/AcDream.Core/Physics/BuildingPhysics.cs | 15 +++++++++++++-- src/AcDream.Core/Physics/CellTransit.cs | 17 +++++++++++++++++ .../CellTransitCheckBuildingTransitTests.cs | 11 ++++++++++- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 620c931..302a124 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -5741,15 +5741,12 @@ public sealed class GameWindow : IDisposable * System.Numerics.Matrix4x4.CreateTranslation(bldOriginWorld); // Derive the outdoor landcell id containing this building. - // Retail's cell index: row-major (gridX * 8 + gridY + 1) within - // the 8×8 grid of 24m cells in a landblock. - int bldGridX = (int)(building.Frame.Origin.X / 24f); - int bldGridY = (int)(building.Frame.Origin.Y / 24f); - if (bldGridX < 0) bldGridX = 0; - if (bldGridX >= 8) bldGridX = 7; - if (bldGridY < 0) bldGridY = 0; - if (bldGridY >= 8) bldGridY = 7; - uint landcellLow = (uint)(bldGridX * 8 + bldGridY + 1); + // Reuse TerrainSurface.ComputeOutdoorCellId rather than + // re-deriving the row-major (gridX * 8 + gridY + 1) formula here. + // Frame.Origin is landblock-relative, same coordinate space as + // ComputeOutdoorCellId expects (local X/Y within the 192m block). + uint landcellLow = terrainSurface.ComputeOutdoorCellId( + building.Frame.Origin.X, building.Frame.Origin.Y); uint landcellId = lbPrefix | landcellLow; _physicsDataCache.CacheBuilding(landcellId, bldPortals, buildingTransform); diff --git a/src/AcDream.Core/Physics/BuildingPhysics.cs b/src/AcDream.Core/Physics/BuildingPhysics.cs index c05cd66..f717a58 100644 --- a/src/AcDream.Core/Physics/BuildingPhysics.cs +++ b/src/AcDream.Core/Physics/BuildingPhysics.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using System.Numerics; +using DatReaderWriter.Enums; namespace AcDream.Core.Physics; @@ -36,6 +37,16 @@ public readonly struct BldPortalInfo public ushort OtherPortalId { get; } public ushort Flags { get; } - /// Bit 0 of Flags (PortalFlags.ExactMatch = 0x0001). - public bool ExactMatch => (Flags & 0x0001) != 0; + /// + /// Bit 0 of (DatReaderWriter.Enums.PortalFlags.ExactMatch). + /// + /// + /// Reserved per retail's CBldPortal::exact_match. NOT currently + /// consumed by — every + /// portal overlap is treated as a valid entry trigger. If a future + /// regression surfaces (e.g., a building entered by overlapping a + /// non-exact-match portal), wire this into the entry test. + /// + /// + public bool ExactMatch => (Flags & (ushort)PortalFlags.ExactMatch) != 0; } diff --git a/src/AcDream.Core/Physics/CellTransit.cs b/src/AcDream.Core/Physics/CellTransit.cs index 0e3e566..12bfa0d 100644 --- a/src/AcDream.Core/Physics/CellTransit.cs +++ b/src/AcDream.Core/Physics/CellTransit.cs @@ -158,6 +158,23 @@ public static class CellTransit /// whether the sphere center is inside it via /// . If so, add the interior /// cell to . + /// + /// + /// Retail divergence: retail's check_building_transit + /// uses CCellStruct::sphere_intersects_cell (radius-aware + /// BSP-vs-sphere test) which fires the moment ANY part of the sphere + /// overlaps the destination cell. Our port uses + /// (radius-less, tests only + /// the sphere CENTER). Practical effect: entry into a building fires + /// when the player's foot-sphere center crosses the destination cell + /// boundary — roughly (~0.48m) DEEPER + /// into the doorway than retail. If visual verification at the cottage + /// door shows a noticeable "late entry" effect (player visually inside + /// the building before walls switch from outdoor-stab to indoor-cell), + /// port sphere_intersects_cell in a follow-up. + /// is plumbed through for that future + /// upgrade; currently unused. + /// /// public static void CheckBuildingTransit( PhysicsDataCache cache, diff --git a/tests/AcDream.Core.Tests/Physics/CellTransitCheckBuildingTransitTests.cs b/tests/AcDream.Core.Tests/Physics/CellTransitCheckBuildingTransitTests.cs index e6cb512..6ea51dc 100644 --- a/tests/AcDream.Core.Tests/Physics/CellTransitCheckBuildingTransitTests.cs +++ b/tests/AcDream.Core.Tests/Physics/CellTransitCheckBuildingTransitTests.cs @@ -8,8 +8,17 @@ namespace AcDream.Core.Tests.Physics; public class CellTransitCheckBuildingTransitTests { [Fact] - public void SphereInsideBuildingPortalDestination_AddsInteriorCell() + public void BuildingPortalWithUnloadedCellBSP_NoCandidateAdded() { + // Verifies the null-CellBSP guard: when the destination interior cell + // is cached but its CellBSP isn't yet loaded (or is structurally absent), + // CheckBuildingTransit must NOT add the cell to candidates — even though + // PointInsideCellBsp(null, _) returns true. + // + // Happy-path (CellBSP present, sphere inside) requires a synthetic + // CellBSPTree which is non-trivial to construct from DatReaderWriter + // types. Deferred to visual verification. + // Building at world origin. One portal to interior cell 0xA9B40100. var building = new BuildingPhysics {